Skip to content

proposal: Go 2: Ban typed nil interfaces through banning nil method receivers #27890

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

Closed
JavierZunzunegui opened this issue Sep 27, 2018 · 18 comments
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Milestone

Comments

@JavierZunzunegui
Copy link

Overview

Methods with pointer receivers have no restrictions on nil receivers. For example the below is valid:

type X struct{}
func (x *X) A() {...}
var x *X
x.A()

This extends to interfaces, creating the issue of typed nil interfaces. See typed-nils-in-go-2 for further context. I would like to see this situation removed in Go2 as typed nil interfaces are a source of bugs and offer no meaningful additional functionality.

Proposed Changes

I propose some changes to the Go2 language spec that would mean banning nil receivers, resulting in banning typed nil interfaces.

Make every method with a pointer receiver nil-pointer panic (at the beginning of the method) on a nil receiver. Implicitly that means if this == nil {panic NilPointer} is added at the beginning of every method. Note this is already implicitly added to most methods (those that use the receiver), just further down the method on the first use the receiver. Compared to the current specs, this would result in:

type X struct{}
func (x *X) A() {...}
var x *X
x.A() // this would now nil-panic (before any logic from A is executed)

Follow this up with another change, when assigning a (typed) pointer to an interface, check its value. If nil, set the interface value as the untyped nil. Note this means the specific type of the pointer unrecoverable. As an example:

type X struct{}
var x *X
i interface{}
i = x
isNil := i == nil  // currently isNil == false, I propose isNil == true
_, ok := i.(*X) // currently ok == true, I propose ok == false

This means if an interface is a typed pointer, it doesn't have to check if the pointer is nil or not, it is non-nil by design, hence the typed nil interface problem is gone.

Other considerations

I imagine others gophers will identify many others issues, these are some I am aware and would like to share upfront:

Optional nil receivers

Methods optionally allowing nil receivers are no longer allowed:

func (x *X) ... {
  if x == nil {
    return nil
  }
  // do something else

Instead a pointer type would be required, type XPtr struct {*X}, which would do the nil check.

Type assertions in nil types

By design, type assertions from nil pointer types are no longer supported. I don't think it is a common (or advisable) pattern, but it will still break the existing libraries using it. Where genuinely needed a pointer type could be used to wrap the pointer: type XPtr struct {*X}.

Methods not using the receiver

Methods do not have to use the receiver. The current spec does not distinguish between the two ways these may be defined (with/without named receiver): func (x *X) ... or func (*X) .... I suggest using this ambiguity and defining them differently in Go2, while the named receiver is just a normal method (and can't be called on a nil typed pointer) the unnamed one may be called on typed nil and does not have the implicit nil check. Note however that because interfaces are never typed nils, once called from an interface it will always be called with a non-nil receiver.

Methods as functions

Consider the below situation:

type X struct {}
func (x *X) A() {...}
var x *X
f := x.A // what to do with this? I suggest panic here

A method on a nil typed pointer is being assigned to a function variable. Calling that function will always panic, making it useless. For this reason I think it is better to change the language spec to have this panic on the assignment.

Compiler considerations

Note these considerations are not affecting the language spec, but I think they may make this changes more appealing.

This proposal means interfaces holding pointers do not have to check if the pointer they hold is nil valued. But that is largely useless if the first thing all methods do is check if the value of the pointer is nil. It would be much better if the nil check where performed by the caller (and still NilPointer panic if nil), improving performance by reducing the number of nil checks (among other things, methods would not need to make that check when calling other methods on themselves).

Also related, when assigning a pointer to an interface the value has to be checked for nil (which is not required currently). While this makes assigning to an interface more expensive, it is likely the compiler can optimize away many of these checks as the nil-ness of the pointer may be known at compile time.

@JavierZunzunegui JavierZunzunegui changed the title Ban typed nil interfaces through banning nil method receivers proposal: Ban typed nil interfaces through banning nil method receivers Sep 27, 2018
@gopherbot gopherbot added this to the Proposal milestone Sep 27, 2018
@JavierZunzunegui JavierZunzunegui changed the title proposal: Ban typed nil interfaces through banning nil method receivers proposal: Go2: Ban typed nil interfaces through banning nil method receivers Sep 27, 2018
@JavierZunzunegui JavierZunzunegui changed the title proposal: Go2: Ban typed nil interfaces through banning nil method receivers proposal: Go 2: Ban typed nil interfaces through banning nil method receivers Sep 27, 2018
@FiloSottile FiloSottile added the v2 An incompatible library change label Sep 27, 2018
@ianlancetaylor ianlancetaylor added the LanguageChange Suggested changes to the Go language label Sep 27, 2018
@deanveloper
Copy link

deanveloper commented Sep 27, 2018

And what about this?
https://play.golang.org/p/d8_dxaLJBRn

Are we really going to allow the assignment operator to panic, or is it just going to be disallowed to assign like this? Would this mean that go vet will flag every time I don't nil-check before I pass a pointer value into a function that takes an interface argument?

Especially with functions like fmt.Println which take interface{} arguments, does this mean I can't pass any typed nils in to them, instead it'd panic?

EDIT
Reading into this further, it looks like this is purely about banning nil method receivers, not removing typed nils entirely. I thought the goal was to ban typed nil values entirely, and that you were really only talking about nil method receivers as an example...

@deanveloper
Copy link

I don't like this mainly because I like thinking of methods in Go as syntactic sugar over functions, which really is what they are. A method really should just be a function, which allows arguments to be nil.

Something else that may become an issue is the following playground:
https://play.golang.org/p/mF44g_INnxP

@JavierZunzunegui
Copy link
Author

@deanveloper the example in your playground is interesting.
I imagine (*X).DoSomethingshares the implementation with (x *X) DoSomething(i int), meaning it would have to enforce panic on nil, yes.
It is still just a function, it simply starts with check and panic on the first argument if nil.

@Merovius
Copy link
Contributor

You are exclusively talking about pointers. But there are more types that can be assigned (and compared to) nil, namely funcs, chans, []s and maps. One implication of this, for example, would be that a nil sort.Ints no longer would be a valid sort.Interface.

In the end, I don't even think this proposal really helps or reduces complexity. Say, I have a function

type Barer interface {
    Bar()
}

type ConcreteBarer int

func (b *ConcreteBarer) Bar() {
    // …
}

func Foo(x Barer) {
    x.Bar()
}

Under your proposal, to make this correct we'd have to either

  1. Add a nil-check to Foo against x
  2. Or add a nil-check to every caller of Foo using a *ConcreteBarer

Whereas to make it correct now, you'd have to

  1. Add a nil-check to (*ConcreteBarer).Bar

In the end, you'll still have to have nil-checks everywhere. You either have to have them in the receiver, or you have them in the caller, or you have them in the callee. At least right now, if *ConcreteBarer can work with a nil-pointer, you can leave out the nil-check.

Personally, I find the current contract pretty clear: If I take an interface, I expect that to be a valid value implementing that. I don't care what that value is and I don't want to. The implementer of the interface knows best what values are valid. Ultimately, if a message call panics, that just means the implementation wasn't valid (and note that it will always be possible for methods to panic, so you can't ever include that). With this change, the contract becomes much harder, because it isn't clear whether I have to check that it's a valid interface-value (i.e. not nil) or the caller has to check that it's a valid value of the type implementing the interface (which might be nil, if that's a pointer).

@JavierZunzunegui
Copy link
Author

JavierZunzunegui commented Sep 28, 2018

@Merovius thank you for the feedback. Like you said I hadn't considered other nulable types, but the same changes do apply to them. In fact it is easier for these than for pointers as none of these types (func, chan, [] and map) actually implement any interface (besides interface{}). Your sort.Ints example is flawed solved easily as []int doesn't implement sort.Interface, it is sort.IntSlice (defined as type IntSlice []int with Len, Less and Swap methods that implements it. Modify sort.IntSlice to be type IntSlice struct {data []int}, then it is not nulable, the fact that it contains a nil []int is irrelevant (it is just a parameter of a struct!). My proposal changes nothing (subject to this library change) on this example, it works identically in both cases.

As your feedback has highlighted, type definitions are also nilable and will be cast to untyped nil when used in an interface. The better patter is a struct wrapping the nilable: type NilablePtr struct {Nilable}

To emphasize, typed nil pointers do implement interfaces, which makes them more complex as defined in my original post. But these other nilable types don't implement (non-interface{}) interfaces, these proposed changes are irrelevant. The only change in these is that you can't recover the type of a nil:

var x func()
var i interface{}
i = x
_ = i.(func()) // this used to be OK, now panics as the nil has become untyped

But I don't think relying on this property significant (and if you are using it you should probably not anyways).

If not convinced just note the below code:

s := sort.Interface(sort.IntSlice(nil))
switch s.(type) {
  // case []int: // this doesn't even compile! "impossible type switch case: s (type sort.Interface) cannot have dynamic type []int (missing Len method)"
  case sort.IntSlice: // this case holds
}

@deanveloper
Copy link

deanveloper commented Sep 28, 2018

Something to consider is that this would be pretty hard to implement in some kind of go1to2 transpiler... Although it is possible, so I don't think it should be a barrier to add a language feature, but it is something to think about.

Also, remember that nil maps and slices are significant and can be used. Consider the following:
https://play.golang.org/p/Pupjh2tZo_u

Sorry for any spelling mistakes, I'm on mobile right now

@Merovius
Copy link
Contributor

sort.IntSlice is not nulable, the fact that it contains a nil []int is irrelevant (it is just a parameter of a struct!).

No, it is not. It is a defined type with underlying type []int and you can totally assign nil to it. sort.IntSlice(nil) is a valid implementation of sort.Interface. So you should talk about what sort.Sort(sort.IntSlice(nil)) does, if it should panic and if not, why not. Currently it works fine.

which makes them more complex as defined in my original post.

Not really, they are far less complex, TBQH. nil-maps allow reading-accesses without panic, which pointers don't. nil-slices allow range, slicing len and cap without problems - they are indeed pretty much identical to non-nil-slices, except that they compare true to nil. nil-channels allow sending, receiving and selects, with well-defined semantics (blocking forever).

ISTM that the rules for pointers are far simpler than for most of the other types - the only operation that is special about pointers is dereferencing and that works if and only if they are not nil. The other types allow more special operations that each have their own peculiar semantics when working on a nil-value.

But I don't think relying on this property significant (and if you are using it you should probably not anyways).

But as I pointed out - it is. Currently, code doesn't need to check for nil when passing well-behaved implementations as interfaces and code doesn't need to check for nil when being passed well-behaved implementations. Under your proposal, at least one (if not both) needs to happen.

i.e. pretty much all Go code out there is relying on this property, by not cluttering everything with nil-checks.

If not convinced just note the below code:

Type assertions on concrete types check whether the dynamic type is identical to the asserted one. []int an sort.IntSlice are different types - but that doesn't change the fact that sort.IntSlice has underlying type []int and thus is a slice-type that can be nil.

@JavierZunzunegui
Copy link
Author

@Merovius regarding the Barer example, there is only one place that nil *ConcreteBarer has to be checked - when Barer is first assigned a *ConcreteBarer. Calling x.Bar() requires no nil check - x is already an interface. It's type has to be resolved to a *ConcreteBarer (as it already does without my proposal, of course), but once that happens it doesn't have to check if it is nil - it isn't by design, there are no more typed nil interfaces!

Not only there are no more nil checks, but they are almost certainly fewer. Say ConcreteBarer.Bar requires the receiver to be non-nil (for example by accessing some parameter on itself). Under the current go spec, it must nil-check the receiver before accessing it. With my proposal, again the pointer is non-nil by construction, and the check can be removed. Depending on your implementation and how many methods ConcreteBarer.Bar calls on itself it may even check for nilnes many times. With my proposal only once, and in fact a smart compiler might often remove that one also, after all (&ConcreteBarer{}).Bar() needs no nil receiver check!

The exception I do acknowledge is when ConcreteBarer.Bar doesn't access the receiver. When cast to an interface my proposal simply disallows it (by design), you need a non-nil pointer to access any functionality. In this case there would be once additional nil check, and one additional memory allocation. Or you can wrap it in a type ConcreteBarerPtr struct {*ConcreteBarer}, in which case you get the exact same functionality. See Methods not using the receiver in my original post for a little more details on this.

@JavierZunzunegui
Copy link
Author

sort.IntSlice is not nulable, the fact that it contains a nil []int is irrelevant (it is just a parameter of a struct!).

@Merovius I stand corrected, what I meant is for sort.IntSlice to be defined as type IntSlice struct {data []int} and not type IntSlice []int as it currently is (so yes, library change, plus initialization now use {} not ()). Note this is how I define my XPtr in the original post, and the appropriate pattern to handle all similar situations must be handled.

@JavierZunzunegui
Copy link
Author

@deanveloper yes the re-writes may be significant and non-trivial. I would favor discussing the merits of the proposed change first though, only if we (the go community!) see benefits in it is the discussion about the cost of backwards incompatibility changes worth having.

See the exchange with @Merovius on other nilable types.

@JavierZunzunegui
Copy link
Author

Not really, they are far less complex, TBQH. nil-maps allow reading-accesses without panic, which pointers don't. nil-slices allow range, slicing len and cap without problems - they are indeed pretty much identical to non-nil-slices, except that they compare true to nil. nil-channels allow sending, receiving and selects, with well-defined semantics (blocking forever).

@Merovius slice read/write, map read/write, channel send/receive, len, cap, (...?) are all functions in the runtime, not methods on these types. My proposal changes nothing on functions, these remain unchanged.

@Merovius
Copy link
Contributor

regarding the Barer example, there is only one place that nil *ConcreteBarer has to be checked

Yes, and my point was exactly that currently there is also only one place that nil has to be checked - in the method. Except that currently, that check is only needed if nil is not a valid value - and the person needing to do the checking is exactly the person implementing the type. In your proposal, the check (whether at the caller or callee site) always has to happen.

You are strictly increasing the amount of nil-checks needed. For every nil-check in a method you can leave out, you have to add a nil-check at all usage sites.

it isn't by design, there are no more typed nil interfaces!

"typed nil interfaces" are not a thing. You mean interfaces with dynamic value nil. I'm not trying to just nitpick - being reasonably precise is important to ensure we talk about the same things and don't unnecessarily talk past each other.

Say ConcreteBarer.Bar requires the receiver to be non-nil (for example by accessing some parameter on itself). Under the current go spec, it must nil-check the receiver before accessing it.

I don't think this argument holds. If think checking the receiver for nil in a method is required for safety, then it's also required for safety to check an interface value for nil before calling any methods on it. In practice, we do neither - we simply accept a certain level of lack of type-safety (also, we usually can't do anything but panic anyway). But if you want that type-safety, you have to actually enforce it everywhere.

Depending on your implementation and how many methods ConcreteBarer.Bar calls on itself it may even check for nilnes many times. With my proposal only once

Only if you assume any concrete pointer type is only used exactly once. In practice, that's certainly not the case. For example, there are many assignments of *bytes.Buffer to io.Writer. And even more usages of an io.Writer interface. Each of these - by your argument of type-safety - would need to be nil-checked.

I don't believe there is a plausible scenario for a concrete type to have more methods than usages.

The exception I do acknowledge is when ConcreteBarer.Bar doesn't access the receiver.

Slice, map and channel types all allow accessing a nil-receiver just fine.

I stand corrected, what I meant is for sort.IntSlice to be defined as type IntSlice struct {data []int} and not type IntSlice []int as it currently is (so yes, library change, plus initialization now use {} not ()).

Also, range, slicing, indexing, len and cap don't work anymore. So this is now effectively becoming a proposal to disallow declaring methods on any non-struct type.

It is my impression that your thinking is heavily influenced by classes in OO-languages, in that methods are closely associated with pointers to struct. But that's not how the Go type system works. TBQH I think it's easier to just accept that - because then it also becomes obvious why thinking about "typed nil interfaces" is such a fallacy. An interface value is defined by the methods it implements. It does not make sense to compare it to nil and expect that to have any relationship to the dynamic type (whether pointer, slice, map…). If you intentionally forget anything about the dynamic type of an interface and purely concentrate on the method set, the question of "is the dynamic value nil" is about as natural as the question "is the dynamic value an integer type storing 23".

It's simply an unfortunate accident of history that the zero-value of interface-types, the zero-value of pointers, and of maps, slices, channels and funcs are all denoted by the same identifier. But they really are different values.

@Merovius
Copy link
Contributor

slice read/write, map read/write, channel send/receive, len, cap, (...?) are all functions in the runtime, not methods on these types.

The argument is, that the behavior of non-pointer types for nil-values is far more complicated to reason about than the behavior of pointer-types for nil-values. Thus it doesn't make sense to exclusively focus on the pointer case, as the non-pointer case has far more questions to answer. I find sort.IntSlice a pretty good illustration of that, as it's methods are 100% natural, without any nil-checks and still a completely valid implementation with the correct, expected behavior when used on a nil-receiver. i.e. it illustrates that nil-receiver are not, in general, a problem and there are cases where they work completely naturally. So you need to at least explicitly say a) whether you want e.g. sort.Interface(sort.IntSlice(nil)) == nil, b) if so, yield that this means any slice used that way need to be checked for nil or sort.Interface needs to check its argument for nil before using it and c) if that equality doesn't hold, explain why it's a problem to have interface{}((*int)(nil)) != nil, but not a problem to have interface{}(([]int)(nil)) != nil.

@JavierZunzunegui
Copy link
Author

@Merovius just to verify we are on the same page, by 'nil check' I mean a runtime check that panics with nilPointer on nil. That is what you mean too, right? As in the same kind of check that go currently does when trying to access a pointer that the compiler can't guarantee is non-nil.

Yes, and my point was exactly that currently there is also only one place that nil has to be checked - in the method. Except that currently, that check is only needed if nil is not a valid value - and the person needing to do the checking is exactly the person implementing the type. In your proposal, the check (whether at the caller or callee site) always has to happen.

The nil check in my proposal happens exactly once for all cases (as you acknowledge). The nil check does not happen in the current go for methods were nil is a valid value (as you highlight). But for methods where nil is invalid, the check will happen at least once, but may happen many times! Consider the below:

type X struct {
  value bool
}
func (x *X) LoopFoo() {
  for i := 0; i < 100; i++ {
    x.Foo()
  }
}
func (x *X) Foo() {
  // do something that requires x.value
}

This will not be inlined (and if unconvinced simply assume that Foo is much bigger). LoopFoo has no direct requirement on x being non-nil, it doesn't have to check it. But Foo does need to check it, and it gets called 100 times. Thus in this scenario current go would do many more checks that my proposal.

Only if you assume any concrete pointer type is only used exactly once. In practice, that's certainly not the case. For example, there are many assignments of *bytes.Buffer to io.Writer. And even more usages of an io.Writer interface. Each of these - by your argument of type-safety - would need to be nil-checked.

Usages of io.Writer involve no nil check, once the interface is resolved to a *bytes.Buffer it isn't nil by design. Assignments of *bytes.Buffer to io.Writer do, except the compiler can likely remove most,
i.e. buf := &bytes.Buffer{}; func(w io.Writer) {...}(buf) needs no check.

@JavierZunzunegui
Copy link
Author

So you need to at least explicitly say a) whether you want e.g. sort.Interface(sort.IntSlice(nil)) == nil

yes. As I said before sort.IntSlice would have to be modified for sorting to still work. Alternatively and much easier simply change (in sort package) func Sort(data Interface) to:

func Sort(data Interface) {
  if data == nil {return} // this is new
  // the rest is unchanged
}

b) if so, yield that this means any slice used that way need to be checked for nil or sort.Interface needs to check its argument for nil before using it

Calling any method in sort.Interface requires the type to be resolved (as it does in current go), and panics if the type is the zero value (also as it does in current go, sort.Interface(nil).Len() panics). The type (whichever it is) that it resolves to does not need nil checking. In your example, you can guarantee that it is a non-nil sort.IntSlice (underlying type []int)

@JavierZunzunegui
Copy link
Author

It is my impression that your thinking is heavily influenced by classes in OO-languages

Unlikely because I come from Maths not CS, I barely did any formal OO classes 😆. But who knows where the influence lies...

Thanks for the great feedback today, @Merovius . Looking forward to resume this very soon

@Merovius
Copy link
Contributor

just to verify we are on the same page, by 'nil check' I mean a runtime check that panics with nilPointer on nil. That is what you mean too, right?

No, I'm talking about if foo == nil { … } code. AFAIK what you are talking about doesn't exist - if you dereference a nil-pointer, the process gets a SIGSEGV and the runtime translates that into a panic. So the generated code assumes optimistically that pointers aren't nil and lets the regular hardware/kernel mechanisms catch it.

I'm concerned with actual code - that is, how would the way we write Go have to change to maintain correctness. I don't see any reason to touch how nil/pointers/interfaces interact at all, except to reduce the number of comparisons to nil in correct code.

LoopFoo has no direct requirement on x being non-nil, it doesn't have to check it. But Foo does need to check it, and it gets called 100 times. Thus in this scenario current go would do many more checks that my proposal.

As I said, these checks don't exist. But FTR, even if they would - I highly doubt it would matter in practice. CPU branch predictors are very good and with modern architectures this wouldn't actually incur any cost except in extreme edge cases.

Alternatively and much easier simply change (in sort package) func Sort(data Interface) to:

Yes, that's the second half of my b) :) But it's good to get a definitive "yes" from you that you intend to also have this apply to non-pointers. :)

Unlikely because I come from Maths not CS

FTR: Me too ;)

@JavierZunzunegui
Copy link
Author

AFAIK what you are talking about doesn't exist - if you dereference a nil-pointer, the process gets a SIGSEGV and the runtime translates that into a panic. So the generated code assumes optimistically that pointers aren't nil and lets the regular hardware/kernel mechanisms catch it.

Oh. I was under the wrong assumption on that, and it does mean the runtime nil-check reductions my proposal promised are not real (as there are no such thing as runtime nil checks! 😞). That reduces the advantages of this proposal only to removing the interfaces with dynamic value nil issue, but makes some code trickier (see sort.IntSlice part) and makes what would have previously been a nil panic on a specific method and type a nil panic on an untyped nil (thereby making it likely harder to debug). And on top of that it is a significantly backwards-incompatible change.

Overall I think it is time to acknowledge this introduces more problems than it solves. Thanks for the feedback @Merovius . 👋

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange Suggested changes to the Go language Proposal v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests

6 participants