golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.88k stars 17.52k forks source link

proposal: spec: add kind-specific nil predeclared identifier constants #22729

Open ianlancetaylor opened 6 years ago

ianlancetaylor commented 6 years ago

A common error for people new to Go is misunderstanding that a interface that is not nil can contain a nil pointer. This is one of the most commonly cited entries in the Go FAQ: https://golang.org/doc/faq#nil_error. A quick search shows at least 24 threads on golang-nuts discussing this, even though it is already in the FAQ.

It is not new to observe that one of the causes of this common mistake is that nil is overloaded. Since changing that would not be backwards compatible, I propose the following changes.

1) We add six new predefined identifiers: nilptr, nilinterface, nilslice, nilchan, nilfunc, nilmap. These new identifiers are untyped constants that only resolve to a certain kind, much as 1.1 is an untyped constant that only resolves to a float or complex type. nilptr may be used as the zero value for any pointer type, nilinterface may be used as the zero value for any interface type, and so forth. An attempt to use, for example, nilptr in an assignment or comparison with a variable or value that is not a pointer type is invalid.

2) We add a vet check that warns about any comparison of a value of any type with plain nil. We encourage people to change x == nil to x == nilinterface or x == nilptr or whatever is appropriate. Since initially this vet check will trigger all the time, we can not turn it on when running go test. It could be on by default when running go vet.

3) At this point people who run go vet will no longer make the common mistake. If v is a value of interface type, then writing v == nilptr will be a compilation error. Writing v == nilinterface will not cause people to erroneously think that this is testing whether v contains a nil pointer.

4) In some later release, we turn on the vet check when running go test.

5) If we are ever willing to make a backward incompatible change, we can make v == nil a compilation error rather than simply being a vet error.

Something to consider is that one could imagine permitting v == nilptr when v has an interface type, and having this be true if v is not nilinterface, if v holds a value of pointer type, and if the pointer value is nilptr. I don't know that this is a good idea, and I'm not proposing it. I'm only proposing the above.

cespare commented 6 years ago

One consequence of this would be that the extremely common if err != nil { ... } would be significantly longer: if err != nilinterface { ... }.

ianlancetaylor commented 6 years ago

@cespare Very valid point.

bradfitz commented 6 years ago

I'd be happier with something in the middle: only adding nilinterface, but naming it none or something shorter. I don't think nilmap, nilslice, etc generate enough confusion.

ianlancetaylor commented 6 years ago

@bradfitz It sounds like you are suggesting that we move toward a different name for the zero value of an interface type, but keep the general overloading of nil for non-interface types. Does that sound right?

I don't really see none as conveying the right idea, but I'm having trouble coming up with a good name. We could try nili or inil, but those might be too close to nil. Maybe empty? I hope someone else has a better idea.

(C++ has a similar confusion in which 0 is the zero value for both integers and pointers, with the preprocessor macro NULL often being used in code. In C++ this has now been resolved by preferring the new keyword nullptr as the pointer zero value.)

bradfitz commented 6 years ago

It sounds like you are suggesting that we move toward a different name for the zero value of an interface type, but keep the general overloading of nil for non-interface types. Does that sound right?

Yes.

Maybe empty?

I like empty.

Seeing lots of if err != empty { ... } is still pretty verbose, though.

cznic commented 6 years ago

Maybe empty?

empty is nice, and it would be enough to predeclare it in the universe scope without breaking any existing Go1 program.

package main

import (
    "fmt"
    "io"
)

var empty interface{}

func main() {
    var w io.Writer
    fmt.Println(w == empty)
}

Playground

cznic commented 6 years ago

it would be enough to predeclare it in the universe scope without breaking any existing Go1 program.

Which is actually what the proposal says, but I noticed that only now, mea culpa :-1:

davecheney commented 6 years ago

@ianlancetaylor thank you for raising this issue. I think this proposal adds a lot of complexity to solve a single issue, returning a typed nil. What about just making a typed nil equal to nil? It is after all what the author of the check intended.

bradfitz commented 6 years ago

What about just making a typed nil equal to nil?

That breaks valid code.

neild commented 6 years ago

If interface{}((*T)(nil)) == nil, how do you test to see if an interface value is zero? I also think changing this equality might be superficially more intuitive, but is more confusing when you dig into it.

Instead of changing the name of a zero-valued interface, it would also be possible to change the name of a zero-valued pointer. null, nullptr, or maybe just 0.

davecheney commented 6 years ago

I don’t doubt that, but the situation now is equally error prone. Anyway, this was always in the context of a Go 2 major version bump.

On 15 Nov 2017, at 09:32, Brad Fitzpatrick notifications@github.com wrote:

What about just making a typed nil equal to nil?

That breaks valid code.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

faiface commented 6 years ago

This issue of nil vs nil comes up all the time.

I think it's time to reconsider what the real issue is. I personally think the current implementation is sound, useful, and when someone understands how it works, they're not going to make any mistakes with it. Not any more than with any other feature.

Maybe the only real problem is education around the issue. People come to Go with expectations. And, when they are not explicitly and repeatedly told about this behavior, they assume it works differently than it actually does. But, that doesn't mean it works bad, it just means that people's expectations are bad. Maybe the only thing we need to fix is people's expectations.

neild commented 6 years ago

Perhaps the problem is less interface equality and more the ease of accidentally assigning a nil pointer to an interface value when you didn't intend to.

ianlancetaylor commented 6 years ago

@davecheney Even for Go 2, I don't think it's OK to remove the ability to test whether an interface is the zero value for interfaces.

@faiface You're not wrong, but if people consistently get something wrong it's worth exploring ways to change it to make it less error prone.

@neild suggested in conversation that we should simply change the zero value of pointers to null. A comparison of an interface value with null would be a compilation error. Assigning nil to a pointer, or comparing a pointer with nil, would be a vet error at least for now. Another option would be to copy C++ and use nullptr. The advantage of these approaches is that all the err != nil code would still be fine. The disadvantage is that all the p != nil code, where p is some pointer type, would want to change to p != null.

jimmyfrasche commented 6 years ago

I'm concerned that this would just push the misunderstanding around while breaking everyone else's muscle memory—but another alternative:

Accept #19642 for making _ a universal zero value and then (initially via a vet check) disallow checking an interface (and possibly other things) against untyped nil.

All the err == nil would change but they'd shorten to err == _.

This could of course expand the problem by allowing one to think it possible to see if whatever is in the interface is zero, but that would come with allowing a universal zero value anyway.

griesemer commented 6 years ago

I'm not a fan of introducing so much nomenclature (nilptr, nilmap, etc.) for relatively little benefit. I also think that we should keep nil as the zero value for pointers, it seems to me that nil is more commonly used with pointers than interfaces (to be verified).

It also would be a mistake to equate a test of (interface) x == nil as a test that ignores typed nils (as @davecheney suggested). @bradfitz already pointed out that it would not be backward-compatible. But it's actually important that we have this facility in the first place: A common idiom in Go is to define data types whose zero values are values that are ready to use. It can make a lot of sense to define a pointer type whose zero value (nil) is a ready-to use value. It may have methods that work with nil receivers. Such a type may even implement the error interface and thus represent a valid error (and we might not know about it if such a type is defined in some external library). It is imperative that in such a case the test err == nil doesn't yield true. Perhaps far-fetched a scenario for an error type, but not so much for other situations.

But there is another side to this problem, and that is that there is no easy way (*) to test if a variable of interface type holds a (typed) nil pointer (or map, or channel) without knowing the actual type. (This does come up, e.g., in iterators walking over graphs such as the Go syntax tree in go/ast.)

If the language had a facility to test for such non-nil interfaces x holding nil values (perhaps via a builtin function isnilptr(x)), presented squarely next to the x == nil test, as a programmer one might be more acutely aware of the two possibilities and automatically think twice.

(*) One way of doing it is using reflection

func isnilptr(x interface{}) bool {
    v := reflect.ValueOf(x)
    return v.IsValid() && v.Kind() == reflect.Ptr && v.IsNil()
}
jimmyfrasche commented 6 years ago

@griesemer why single out pointer-y things for such a builtin? Why not a more general holdsZero?

func holdsZero(i interface{}) bool {
    v := reflect.ValueOf(i)
    if !v.IsValid() { // maybe drive the point home further and let this panic if i == nil
        return false
    }
    z := reflect.Zero(v.Type()).Interface()
    return i == z
}

(I wouldn't be surprised if that's missing some edge cases)

In either case, if it were a builtin it should fail to compile unless it's parameter is an interface, to avoid confusion like holdsZero(0) being true.

neild commented 6 years ago

Why single out testing for the presence of a zero value?

An interface type specifies the desired properties of implementations of that interface; if zero-ness is a significant property of an interface, then shouldn't it be part of the type definition?

type T interface {
  IsZero() bool
}

It feels to me that interface equality is approaching the problem from the wrong direction; I think that in cases where someone is trying to test for a typed nil value in an interface they almost always really wanted an untyped nil interface value instead. e.g., problem the nil error FAQ entry describes is not that error((*myerror)(nil)) != nil, but that the error contains a typed nil in the first place.

griesemer commented 6 years ago

@jimmyfrasche Maybe. I chose the pointer case because this is the one that I've actually seen in practice.

@neild You have a point with using a method.

earthboundkid commented 6 years ago

I love this proposal.

I think it's worth considering null as the name to add. It's a very commonly used keyword. Semantically, it would make more sense for everything but interface to use null, and interface would continue to use nil. OTOH, if we use none, then interface could use that and everything else could continue using nil. In either case, there's an easy transition using go vet from Go 1 to Go 2.

ugorji commented 6 years ago

What about making nil a variable OR a function that has a context i.e.

This way, there's no new keyword, but we expand the use of the nil keyword (like we expanded the use of make).

If we combine this with a builtin iszero(x) function where x is any value, and accompanying reflect.IsZero(reflect.Value) function, we should capture most of the use cases without introducing many new keywords.

griesemer commented 6 years ago

@ugorji Just for the record: nil is not a keyword. It's a predeclared identifier. There's a difference.

faiface commented 6 years ago

I would like to point out one thing. Having interfaceValue == nil or even interfaceValue == nilptr is inconsistent with current Go anyway. Let me explain why. It basically assumes, that the nilptr value will get automatically converted to the concrete type of interfaceValue and the true comparison will happen afterwards.

This is not how it happens in Go. Let's take a look at numbers for a similar example:

var x interface{}
x = float64(0)
if x == 0 {
    fmt.Println("this doesn't happen!")
}

Did you guess correctly what happens in the above example? It is similarly counter-intuitive at first, but it makes sense. The 0 value gets converted to its default type, which is int and comparison proceeds and results in false, because we are comparing different types.

With interfaces it's similar. Does it even make any sense whatsoever, to ask whether an interface value is a nil value of an arbitrary pointer type?

ianlancetaylor commented 6 years ago

@faiface I agree: it doesn't. But the evidence is clear that that is what many people new to Go think is happening. It's a FAQ, and it's visibly a stumbling block for new Go programmers. I think it's worth examining whether there is something we can do to cause fewer people to stumble here.

faiface commented 6 years ago

@ianlancetaylor How about the numbers? Would you solve those too? If not, being able to check if interface contains arbitrarily typed nil pointer, but not being able to check if it contains an arbitrarily typed zero, or one, would be an inconsistency that could cause more confusion than clarification.

ianlancetaylor commented 6 years ago

@faiface This proposal would not fix the comparisons of an interface value with 0. Current experience is that that is a much less significant problem: it is rarely reported as a bug, and there has been no need to create a FAQ entry for it. I'm not particularly worried about that inconsistency myself.

(Note that this specific proposal is not for a way to check whether an interface contains a nil pointer of arbitrary type. I mentioned that as "something to consider," but it is not part of the proposal.)

faiface commented 6 years ago

@ianlancetaylor If I understood correctly, v == nilptr returns true iff v contains a nil pointer of arbitrary pointer type. Is that wrong?

Edit: Ah, ok, I see, that's just an "appendix", not the main proposal.

jimmyfrasche commented 6 years ago

Ideally this would be addressed without legitimizing the incorrect understanding and without making it more work for those with the correct understanding.

Since there is time yet before Go2, perhaps some effort could be put into the faq and training material to better explain why this is incorrect and see if that at least reduces the frequency of the question.

I skimmed the FAQ and it looks like this only addressed in https://golang.org/doc/faq#nil_error . If that's true (and apologies if I missed anything) maybe there should be a separate entry for something like "How do I tell if my interface contains a nil pointer" as this comes up with more than just errors.

pcostanza commented 6 years ago

I support this proposal. Overloading nil with lots of different meanings is, in my humble opinion, a case of leaking implementation details into the language specification. nil slices, nil pointers, nil functions, nil maps, and nil channels have nothing in common at the conceptual level. The only commonality is that they happen to be implemented at the machine level by a particular bit pattern stored in a machine word. Even at the behavioural level, there are subtle differences between the different types: While most operations on nil values panic, appending to a nil slice is actually fine, and receiving from a nil channel just blocks, but doesn't panic. It's still defensible to keep the nil identifier for these different types because the type system ensures that they cannot be mixed up. However, nil interfaces /can/ currently be mixed up with other kinds of nil values, and the confusion this creates is real. I believe it's an unnecessary stumbling block, and the tutorial pages that could be saved by a relatively simple language change could then be spend on more important topics. ;-)

There is precedent that distinguishing concepts even if they have the same or very similar implementations is a good thing. Think of booleans being very similar to integers in many languages; strings being very similar to random-access arrays in most languages that don't use UTF-8 internally; functions being very similar to methods; interfaces being very similar to abstract classes in C++; and so on. It often pays off to add clarity at the conceptual level by making distinctions explicit, and I believe the case of nil interfaces vs other kinds of nil values is one such instance.

ianlancetaylor commented 6 years ago

(Underlying problem again incorrectly reported as a bug at #24835.)

cznic commented 6 years ago

I support this proposal. Overloading nil with lots of different meanings is, in my humble opinion, a case of leaking implementation details into the language specification.

It seems exactly opposite to me. nil says nothing about implementation details. nilMap, nilSlice etc. explicitly reveal that the implementation of map and slice are different. nil is not an actual value. It's an implementation detail independent concept that represent different concrete values in different contexts. It's a bit like number zero. A particular number zero value can by any of byte, int, uintNN, floatNN, complexNN, ... zeros differing in size and interpretation.

ianlancetaylor commented 6 years ago

(Underlying problem again incorrectly reported as a bug at #24947.)

pcostanza commented 5 years ago

I found an even smaller example that illustrates the problem. It's impossible to tell whether the following code prints true or false without knowing the types of x or y.

x = nil; y = x; fmt.Print(y==nil)

No amount of tutorial can help you here. ;)

faiface commented 5 years ago

@pcostanza

You can make the same point with numbers: https://play.golang.org/p/FrUSqnE4GjU

package main

import (
    "fmt"
)

func main() {
    var (
        x float64
        y interface{}
    )
    x = 0
    y = x
    fmt.Println(y == 0)
}
pcostanza commented 5 years ago

@faiface

That's a good point, and maybe needs fixing as well.

gwd commented 5 years ago

My main issue with this approach is that in the common case where I trip up over this is a situation like this:

var InterfaceType x

if ( something ) {
  x = GetThingA()
} else if (something_else) {
  x = GetThingB()
}

if x != nil {
  // Do something with x
} else {
  // Handle the "nothing suitable to do" case
}

GetThingA and GetThingB return pointers to ThingA and ThingB respectively. Both satisfy InterfaceType. But may also return nil if ThingA or ThingBcan't be returned for whatever reason.

Under this proposal, I'd need to check if x != nilinterface && x != nilptr { to get the behavior I want. I'm not really sure that's better than if reflect.ValueOf(x).IsNil().

EDIT (The initial version of this wasn't very clear about what I wanted instead)

This will currently execute the else clause if both something and something_else are false (i.e., if x is uninitialized), but not if GetThingA or GetThingB return nil.

What I'd ideally like is for there to be a single, simple, obvious operation I could use to execute the else clause if either x is uninitialized, or if one of the two functions return nil.

pcostanza commented 5 years ago

You say that GetThingA and GetThingB return *ThingA and *ThingB respectively. Without this proposal, the second else block would therefore never be executed. This is exactly why this proposal, or similar ones, are beneficial.

gwd commented 5 years ago

@pcostanza Sorry, my comment had a bunch of context that I failed to bring in. I've edited the comment to add a bit more about what I want.

tandr commented 5 years ago

(I got here from #32845)

@bradfitz wrote some time ago (https://github.com/golang/go/issues/22729#issuecomment-344668186)

What about just making a typed nil equal to nil?

That breaks valid code.

I am curious - what kind of valid code, sorry? Is it extremely common or it is a "nifty hack"?

ianlancetaylor commented 5 years ago

@tandr See, e.g., https://github.com/golang/go/issues/22729#issuecomment-344828048 .

tandr commented 5 years ago

Thanks Ian. If I understood that comment correctly, it talks about "plain uninitialized pointer is nil", (maybe zero initialized memory to be equivalent to nil), but I am looking at "checking for nil on the receiver" part. You see, I am not sure if checking receiver against fat-nil instead of "just" nil will change what happens. Even so, it feels like quite a lot of places of "naked" nil comparison will work fine under fat-nil comparison, and for the rest of the cases - maybe something else is needed, like an explicit cast to something. Forbidding untyped nil comparison sounds like an overkill, since compiler could help here with type deduction and have nil correctly typed/wrapped/casted. But still, it might need "go vet" to the rescue...

Now, how many of these cases of comparing something to nil would behave differently (say in Go sources themselves, and maybe some libraries) under these changes is a task that is out of my league (i.e. I have no clue how do it right now).

Thank you for your time.

PS. "fat nil" is fat pointer, "typed nil"

alanfo commented 5 years ago

Whatever the merits of this proposal, perhaps I could make one simple point:

If anything is done which would change if err != nil, then the folks who disliked the try proposal will go nuclear!

I have a lot of sympathy with what @faiface said earlier, namely that all that is really needed here is better education on how interfaces work.

However, if something must be done to draw attention to the difference between an interface variable being nil and its dynamic type having a nil value, then I thought @griesemer's idea of an isnilptr built in function to test for the latter would be the best solution though holdsnil might be a better name for it. I can't see the point of testing for zero values generally as the confusion only arises when the dynamic value is nil.

ianlancetaylor commented 5 years ago

Adding an isnilptr function really only helps people who already understand the problem. The goal of this issue is to help guide people who do not understand the problem.

alanfo commented 5 years ago

Adding an isnilptr function really only helps people who already understand the problem.

I think it might in fact also help those people who don't understand the problem or are confused about it because, as @griesemer said earlier:

If the language had a facility to test for such non-nil interfaces x holding nil values (perhaps via a builtin function isnilptr(x)), presented squarely next to the x == nil test, as a programmer one might be more acutely aware of the two possibilities and automatically think twice.

Even if we had something like nilinterface I am not convinced it would necessarily solve the problem because a beginner might still think that if x == nilinterface was testing the interface's dynamic type for 'nilness' and that (regardless of the dynamic type) you had to use nilinterface because x was an interface variable.

Reading the proposal again, your alternative idea of allowing x == nilptr for interfaces would be equivalent to isnilptr(x) though I prefer the latter as the former would (in effect) be overloading the == operator.

pcostanza commented 5 years ago

@alanfo You seem convinced that interface nil vs. pointer nil is just a matter of teaching and better understanding of the language. I don't agree with that. I rather think it's objectively confusing. Consider:

n = nil
e = n
if e != nil {
   fmt.Println(e)
}

This code fragment prints <nil> depending on the involved types. It's a pity you have to know the involved types in a language that otherwise (rightfully) prides itself of being particularly easy to read and understand.

The problem is that interface nil and pointer nil are conceptually two vastly different concepts, and it was not a good idea in the original design of Go to give them the same name.

I believe this particular problem should not anymore be the rite of passage for Go programmers that it currently is, because it's unnecessary and doesn't serve a purpose. (Backwards compatibility is not a particularly convincing purpose in a language that is currently seeing some dramatic changes anyway.)

Just my 0.02€... ;)

Pascal

alanfo commented 5 years ago

@pcostanza

The problem is that interface nil and pointer nil are conceptually two vastly different concepts, and it was not a good idea in the original design of Go to give them the same name.

They are different but presumably, back in v1.0 days, the Go team didn't consider they were different enough to justify the use of another predefined identifier (such as empty or void) to indicate that an interface contained no value at all. So we are where we are.

Although we are likely to see a dramatic change to the language in the introduction of generics (but probably not now error handling) the current generics proposal is still backwards compatible (or, at any rate, can be) and I know the Go team regard this as a very important consideration.

As I don't really see how anyone can use interfaces successfully without being able to distinguish between an interface variable and what (if anything) it currently contains, then education must play an important part and, whilst I don't object to a language change being made to reinforce this, I do think such a change should be simple, proportionate and (preferably) backwards compatible.

ianlancetaylor commented 5 years ago

Reading the proposal again, your alternative idea of allowing x == nilptr for interfaces would be equivalent to isnilptr(x) though I prefer the latter as the former would (in effect) be overloading the == operator.

This proposal has a series of five steps aimed to fix the problem over time. Now maybe it's a bad proposal--in fact, it probably is--but it's more than just one idea.

alanfo commented 5 years ago

When I referred to your 'alternative idea' I was alluding to your final paragraph:

Something to consider is that one could imagine permitting v == nilptr when v has an interface type, and having this be true if v is not nilinterface, if v holds a value of pointer type, and if the pointer value is nilptr. I don't know that this is a good idea, and I'm not proposing it. I'm only proposing the above.

As you made clear, this wasn't part of the actual proposal but an idea to consider.

ianlancetaylor commented 5 years ago

Ah, sorry.

andig commented 4 years ago

This is a little off-topic, but since https://golang.org/doc/faq#nil_error was mentioned I feel it's not precise enough:

func returnsError() error {
  var p *MyError = nil
  if bad() {
      p = ErrBad
  }
  return p // Will always return a non-nil error.
}

func returnsError() error {
  if bad() {
      return ErrBad
  }
  return nil
}

It's a good idea for functions that return errors always to use the error type in their signature (as we did above) rather than a concrete type such as *MyError, to help guarantee the error is created correctly.

It seems that the bad (first) example is "use(ing) the error type in their signature" as advised, yet still broken. It should not only use the error type but also return the nil interface instead of a differently-typed nil (if that's the correct wording).