code review 5321061: errors: new package

errors: new package

The only function is errors.New, at least for now.

[rsc, 2011-10-28 04:04:11]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-10-28 04:13:54]

sorry, i don't understand the New thing

http://codereview.appspot.com/5321061/diff/4001/src/pkg/errors/errors.go
File src/pkg/errors/errors.go (right):

http://codereview.appspot.com/5321061/diff/4001/src/pkg/errors/errors.go#newc...
src/pkg/errors/errors.go:5: // Package errors implements functions to manipulate
errors.
s/errors/error values/ would read better and be less confusing.

http://codereview.appspot.com/5321061/diff/4001/src/pkg/errors/errors.go#newc...
src/pkg/errors/errors.go:9: // New(text) == New(text) even for different calls
to New.
i don't understand what you're trying to say.

http://codereview.appspot.com/5321061/diff/4001/src/pkg/errors/errors_test.go
File src/pkg/errors/errors_test.go (right):

http://codereview.appspot.com/5321061/diff/4001/src/pkg/errors/errors_test.go...
src/pkg/errors/errors_test.go:13: if New("abc") != New("abc") {
what??

[rsc, 2011-10-28 04:14:46]

On Thu, Oct 27, 2011 at 21:13,  <r@golang.org> wrote:
> i don't understand what you're trying to say.

if you call New twice with the same string,
the two different results are == to each other

[r, 2011-10-28 06:08:29]

On Thu, Oct 27, 2011 at 9:14 PM, Russ Cox <rsc@golang.org> wrote:
> On Thu, Oct 27, 2011 at 21:13,  <r@golang.org> wrote:
>> i don't understand what you're trying to say.
>
> if you call New twice with the same string,
> the two different results are == to each other

yes, but so what? it seems such an odd thing to single out. upon
reflection i think i see what you're after, but the message is
cryptic. you mean, i believe, that although the function is called
New, it doesn't work like other New functions in that the return
values compare. but is that worth saying? is code likely to depend on
multiple calls being unequal?

anyway, a statement of intent rather than a bald fact might help.

-rob

[rsc, 2011-10-28 14:38:54]

On Thu, Oct 27, 2011 at 23:08, Rob 'Commander' Pike <r@golang.org> wrote:
> yes, but so what? it seems such an odd thing to single out. upon
> reflection i think i see what you're after, but the message is
> cryptic. you mean, i believe, that although the function is called
> New, it doesn't work like other New functions in that the return
> values compare. but is that worth saying? is code likely to depend on
> multiple calls being unequal?
>
> anyway, a statement of intent rather than a bald fact might help.

It is a property that has surprised people and that people
may or may not be depending on.  I would be happy to
change it so that New("abc") != New("abc") and then drop
the sentence, but I have no idea what will break.

Russ

[gustavo_niemeyer.net, 2011-10-28 14:51:08]

> It is a property that has surprised people and that people
> may or may not be depending on.  I would be happy to
> change it so that New("abc") != New("abc") and then drop
> the sentence, but I have no idea what will break.

There will be some significant breakage, I believe.
There's quite a bit of code that does something like:

    foo, err := pkg.Foo()
    if err == pkg.ErrSomethingBad {
        ...
    }

Where ErrSomethinBad is purposefully declared today as

    var ErrSomethingBad = os.NewError(...)

If that changes, people will have to redeclare these errors with a
different type to preserve the idiom, or change the idiom.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[rsc, 2011-10-28 14:55:47]

On Fri, Oct 28, 2011 at 07:50, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> There will be some significant breakage, I believe.
> There's quite a bit of code that does something like:
>
>    foo, err := pkg.Foo()
>    if err == pkg.ErrSomethingBad {
>        ...
>    }
>
> Where ErrSomethinBad is purposefully declared today as
>
>    var ErrSomethingBad = os.NewError(...)
>
> If that changes, people will have to redeclare these errors with a
> different type to preserve the idiom, or change the idiom.

No, that will continue to work fine.  The property in question
is not whether == works at all.  It is whether, if you call New twice
with the same text, the two results are == to each other.

The surprising behavior is this:

package rpc
var ErrInternal = errors.New("internal error")

package math
var ErrInternal = errors.New("internal error")

package main
err := Foo()
assert(err == math.ErrInternal)  // that's what Foo returns
assert(err == rpc.ErrInternal)  // because it's the same text!

I should probably just fix the behavior and see what breaks.

Russ

[rsc, 2011-10-28 15:03:24]

On Fri, Oct 28, 2011 at 07:55, Russ Cox <rsc@golang.org> wrote:
> I should probably just fix the behavior and see what breaks.

Just to be clear, this behavior exists today with os.NewError.
It's not that I thought it was a good design for errors.New
but instead that I was trying not to break existing code.
However, I think the point can be made that existing code
depending on this is already broken.

Russ

[gustavo_niemeyer.net, 2011-10-28 15:11:50]

>> I should probably just fix the behavior and see what breaks.
>
> Just to be clear, this behavior exists today with os.NewError.
> It's not that I thought it was a good design for errors.New
> but instead that I was trying not to break existing code.
> However, I think the point can be made that existing code
> depending on this is already broken.

Agreed, this sounds like a relevant improvement. I misunderstood the
conversation as hinting that equality shouldn't work for errors, which
sounded strange.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[r, 2011-10-28 15:14:08]

it's an easier property to fix than to document.

-rob

[rsc, 2011-11-01 05:26:52]

Hello r@golang.org, gustavo@niemeyer.net (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-11-01 06:12:28]

http://codereview.appspot.com/5321061/diff/15001/src/pkg/errors/errors_test.go
File src/pkg/errors/errors_test.go (right):

http://codereview.appspot.com/5321061/diff/15001/src/pkg/errors/errors_test.g...
src/pkg/errors/errors_test.go:18: t.Errorf(`New("abc") == New("abc")`)
error message should be `New("abc") == New("xyz")`

[r, 2011-11-01 16:23:26]

LGTM

[r2, 2011-11-01 16:24:17]

On Nov 1, 2011, at 9:23 AM, r@golang.org wrote:

> LGTM

(after addressing adg's point)

[rsc, 2011-11-02 01:47:02]

*** Submitted as http://code.google.com/p/go/source/detail?r=b1c079c01a0a ***

errors: new package

The only function is errors.New, at least for now.

R=r, gustavo, adg, r
CC=golang-dev
http://codereview.appspot.com/5321061