Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: Go 2: disallow "bare" nil values when comparing to an interface #30865

Closed
gwd opened this issue Mar 15, 2019 · 19 comments
Closed

proposal: Go 2: disallow "bare" nil values when comparing to an interface #30865

gwd opened this issue Mar 15, 2019 · 19 comments
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Milestone

Comments

@gwd
Copy link

gwd commented Mar 15, 2019

Summary

Don't allow a 'bare' nil keyword on either side of a comparison expression (== or !=) if the other side is an interface; only allow comparison to a type with an explicit cast; for example, interface{}(nil).

Motivation

The following is an ever-present trap:

func foo() *bar {
  // ...
  if something_wrong {
    return nil;
  }
}

var x interface{}

x = bar()

if x != nil {
  // Dereference x
}

The code is very obviously trying to see if x can be dereferenced; but instead it checks to see if x is some arcane type nobody uses.

Proposal

Don't allow a 'bare' nil keyword on either side of a comparison expression (== or !=). The code above would thus throw a compiler error, alerting the developer to the fact that she should do something else instead (perhaps reflect.ValueOf(x).IsNil() or some variant thereof).

If someone really did want to compare to an actual nil interface, they could use an explicit cast, as below:

if x != interface{}(nil) {
   // ...
}

This has the advantage of changing the language as little as possible, while preventing developers from walking into one of Go's few "language traps".

References

Other proposals which try to address this topic:

  1. proposal: Go 2: Add typednil keyword for checking whether an interface value is a typed nil. #24635
  2. proposal: Go 2: add kind-specific nil predeclared identifier constants #22729
  3. Proposal: use a value other than nil for uninitialized interfaces #21538
  4. proposal: Go 2: Ban typed nil interfaces through banning nil method receivers #27890
  5. proposal: Go 2: require explicit type cast to create "nil receiver" interface #30294
@gopherbot gopherbot added this to the Proposal milestone Mar 15, 2019
@ianlancetaylor ianlancetaylor changed the title Proposal: Go2: Disallow "bare" nil values when comparing to an interface proposal: Go 2: disallow "bare" nil values when comparing to an interface Mar 15, 2019
@ianlancetaylor ianlancetaylor added LanguageChange v2 A language change or incompatible library change labels Mar 15, 2019
@ianlancetaylor
Copy link
Contributor

Tempting, but since error is an interface type it would mean that all comparisons that look like err != nil would be rejected. I don't think that is feasible.

@gwd
Copy link
Author

gwd commented Mar 15, 2019

On the contrary, that's exactly why we need something like this.

The fact is, err != nil is a trap: 99.9% if the time, it's going to do exactly what you want; but if anything anywhere in the call chain accidentally returned you a nil value of some other pointer type, your err != nil check will do the wrong thing. If we're going to have a language where error(nil) != SpecificErrorType(nil) (and I don't see how Go would still be Go without it), then it is simply not safe to compare an interface to nil.

We could have an is_nil() keyword or something instead, which effectively did reflect.ValueOf(display).IsNil().

@ianlancetaylor
Copy link
Contributor

If we disallow err != nil, then essentially every Go package in existence will fail to compile, and essentially all existing Go documentation will be wrong. That is a non-starter.

@gwd
Copy link
Author

gwd commented Mar 15, 2019

But would you agree, though, that every Go package in existence currently behaves dangerously in this regard, and all existing Go documentation recommends dangerous behavior? There is nothing to stop some function way deep in the callstack -- perhaps in a package imported from a package imported from a package you import -- returning SpecificErrorType(nil), and making your error == nil check do the wrong thing. My whole reason for using a language like Go rather than a language like Python is to be able to avoid exactly this sort of problem.

If you agree there's a problem, then we can try to come up with a practical approach to making this sort of a change more feasible.

@ianlancetaylor
Copy link
Contributor

I'm not sure I agree that the problem is precisely as you've stated it, but I agree that there is a problem in this general area, and you've referenced earlier issues that try to address it.

But at this point in the language's evolution, I don't think that we can disallow err != nil.

@gwd
Copy link
Author

gwd commented Mar 15, 2019

Well, one approach, for instance, would be to begin by allowing individual packages / files to "opt-in" to the new behavior. If the core Go people then promoted enabling this option, and changed as much documentation as possible, it might receive significant uptake; if it did then we could consider enabling the feature by default (and allowing individual files to disable it).

The other approach would be, since err != nil is already "magic" (in that the type of nil changes based on the context) is to have the compiler interpret this to be equivalent to reflect.ValueOf(err).IsNil() when comparing an interface to a bare nil keyword; and (again), to have the current behavior only when the type is specified.

@OneOfOne
Copy link
Contributor

A simple counter proposal is maybe adding another keyword for nil that would deep match, something along the lines of:

var x0 interface{} = (*string)(nil)
var x1 interface{} = nil
x0 == nil // false
x0 == ifnil // true (ifnil is just an idea)
x1 == nil == ifnil

@gwd
Copy link
Author

gwd commented Mar 15, 2019

@OneOfOne The problem with what you recommend is that it doesn't actually close the "trap": that the normal, natural thing to do -- the thing that, as @ianlancetaylor points out, basically every Go package in existence does and all existing Go documentation recommends -- works 99% of the time and does the wrong thing the other 1%. If we introduced x0 == ifnil (or, as I recommended above, is_nil(x0) (or probably IsNil(x0) since Go prefers CamelCase to snake_case)), I would probably forget most of the time and use the unsafe version, x0 == nil instead.

The point of a statically typed language, to me, is to reduce the number of random things I have to remember to get right. I plan, in the future, of always using reflect.ValueOf(x).IsNil() when comparing to nil. Having a compact way to write that would certainly be helpful, but even more helpful would be a way to tell the the compiler reminded me to use it and not the other one I almost certainly didn't mean.

Or even better yet, did what I almost certainly did mean.

@dpinela
Copy link
Contributor

dpinela commented Mar 15, 2019

Using reflect.ValueOf(x).IsNil() everywhere would also be a non-starter, for performance reasons if nothing else. I think the best solution to this problem is to change the rules for pointer-to-interface conversions (like #30294), or interface-to-nil comparisons. Both are breaking changes, though.

@gwd
Copy link
Author

gwd commented Mar 15, 2019

Well yes, I don't think this can be fixed without breaking changes; that's why I'm aiming for Go 2.

I was expecting that if we introduced a special isnil() keyword that the compiler would have an optimized version, rather than calling out to the reflect package.

@go101
Copy link

go101 commented Mar 16, 2019

I feel the fact that nil represents as zero values of several kinds of types is half-consistent, which is the source of some confusions.
I remembered there were some proposals to solve the same problem in two ways:

  1. let nil be able to represent as zero values of any kinds of types. After all, nil in English means zero (I can't find back this proposal now again.).
  2. let nil only represent as zero values of interface types, and use some other predeclared identifiers for pointers/slices/maps/channels/functions. (Or use another predeclared identifier to represent zero values of interface types.)

Personally, I think those are better than the current one.

@go101
Copy link

go101 commented Mar 16, 2019

An advantage of letting nil be able to represent as zero values of any kinds of types is this will make some conveniences for future custom generics. When we want to reset a value of generic type, just assign nil to it.

@ianlancetaylor
Copy link
Contributor

See #22729 (mentioned in the original issue submission).

@beoran
Copy link

beoran commented Mar 19, 2019

I think the problem is not in the comparison, but in how it's too easy to accidentally make a non-nil interface value with a nil inside of it. In fact, is there even any use to such a value? Perhaps making such a value could panic unless forced not to by a ,ok = form...

@ianlancetaylor
Copy link
Contributor

@beoran I think you are looking for #30294.

@gwd
Copy link
Author

gwd commented Mar 19, 2019

@beoran Well I did recently see this interesting suggestion:

type Config struct {
	path string
}

func (c *Config) Path() string {
	if c == nil {
		return "/usr/home"
	}
	return c.path
}

func main() {
	var c1 *Config
	var c2 = &Config{
		path: "/export",
	}
	fmt.Println(c1.Path(), c2.Path())
}

On the whole, though, I think that's a bit too "clever" for me.

@beoran
Copy link

beoran commented Mar 20, 2019

Well, I admit that there will probably be some existing code out there that is that clever. After all, in Go, you can make methods nil-proof, it's just that most people don't bother. But yes, I prefer #30294 to this proposal, which should make it harder to accidentally wrap nil pointers.

@ibrt
Copy link
Contributor

ibrt commented Mar 21, 2019

I have definitely been bitten by this issue before and would like to see it fixed. To me it is a type inference "bug", and while the nil case is the worst, it applies to other types as well. Consider the following example:

package main

import (
	"fmt"
)

func main() {
	var v interface{}
	
	v = someInt()
	fmt.Println(v == 1)
	
	v = someIntPtr()
	fmt.Println(v == nil)	
}

func someInt() int64 {
	return 1
}

func someIntPtr() *int {
	return nil
}

This will print false twice. We are used to the fact that constants are automatically cast to the right type when referenced, except in this case it doesn't work as one intuitively would expect.

I suspect the best fix would be to make it just work with support from the runtime, but I'm not sure whether it's feasible.

@ianlancetaylor
Copy link
Contributor

There may be something to change in this area, but this specific proposal would invalidate essentially all existing Go code, and cannot be adopted.

@golang golang locked and limited conversation to collaborators Mar 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

8 participants