go vet should warn about a potential non-nil interface with nil value

[Burak Serdar]

This happened to me more than once, and not only with errors.

For a function that is declared to return an interface, if you return
a nil-pointer with a type other than that interface, the returned
interface is not nil.

Here's an example:

https://play.golang.org/p/dpd76zyN9Fv

I think go vet should warn about this case. What do you think?

[Axel Wagner]

It's been discussed a bunch of times - the main argument against remains that "nil" is not a special value and can be a perfectly reasonable implementation of an interface. For example, `sort.StringSlice(nil)` is a perfectly fine implementation of `sort.Interface` (the same, of course, for any other slice type you might implement `sort.Interface` for yourself). I.e. there is nothing *inherently* wrong putting a `nil` concrete value into an interface to make a non-nil interface. And as vet errs on the side of caution (and also, because TBQH it seems useless to specifically initialize a slice with an empty slice just to satisfy vet), there shouldn't be vet-checks with (likely) false positives.

I think it *might* be possible to write a careful vet-check that avoids false positives and can still help catch some of the bugs. If you find an assignment of a definitely-nil-value to an interface, you could check the methods of that concrete type whether they panic when the receiver is nil (i.e. if they dereference it). This wouldn't have false positives, but should catch most interesting and simple cases.

If that sounds interesting, I would recommend trying to implement that as a tool out-of-tree, to give the community opportunity to test it out. If you base it on the analysis package, it would be easy to integrate into other tools (and maybe eventually vet) when the time comes :)

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAMV2RqrpdCa4t6VrBrkbU%3DrbdNhC_oz%2BusTD9PA6VgCb%3D2SXJw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Burak Serdar]

On Thu, Jun 27, 2019 at 8:51 AM Axel Wagner
<axel.wa...@googlemail.com> wrote:
>
> It's been discussed a bunch of times - the main argument against remains that "nil" is not a special value and can be a perfectly reasonable implementation of an interface. For example, `sort.StringSlice(nil)` is a perfectly fine implementation of `sort.Interface` (the same, of course, for any other slice type you might implement `sort.Interface` for yourself). I.e. there is nothing *inherently* wrong putting a `nil` concrete value into an interface to make a non-nil interface. And as vet errs on the side of caution (and also, because TBQH it seems useless to specifically initialize a slice with an empty slice just to satisfy vet), there shouldn't be vet-checks with (likely) false positives.
>
> I think it *might* be possible to write a careful vet-check that avoids false positives and can still help catch some of the bugs. If you find an assignment of a definitely-nil-value to an interface, you could check the methods of that concrete type whether they panic when the receiver is nil (i.e. if they dereference it). This wouldn't have false positives, but should catch most interesting and simple cases.

I agree with what you said about false positives. Some of them, like
the one you gave above about the sort.StringSlice can be avoided.

To give some context on why this is so annoying:

I have a function that simply returns the next item in chain:

func (a X) Next() *Item {
return a.Next
}

...
if a.Next()==nil {
...
}


Later, this function is converted to use an interface instead of the
concrete type:

func (a X) Next() Intf {
return a.Next
}

Now the if a.Next()==nil {} no longer works. It has to be changed to:

func (a X) Next() Intf {
if a.Next==nil {
return nil
}
return a.Next
}

So if vet could check :

- function returns an interface Y
- there is a return X where X is not of type Y and is a reference
- X is not assigned a value in at least one code path
- there are no nil-checks for X

then it is likely that function will mask a nil-return.

[Axel Wagner]

Can you post some working code to the playground? ISTM that the code you shown wouldn't compile (X has both a Next field and a Next method). Also, the receiver is called X, but you return an *Item. That's not totally hypothetical as a follow-up, because in general, linked lists are an example of where IMO a nil-value can be made a completely useful implementation of an interface, by putting a nil-check in the methods. However, it's hard to talk about it concretely, without working code to adapt to show the idea :)

On Thu, Jun 27, 2019 at 5:46 PM Burak Serdar <bse...@ieee.org> wrote:

  - function returns an interface Y
  - there is a return X where X is not of type Y and is a reference
  - X is not assigned a value in at least one code path
  -  there are no nil-checks for X

IMO this is not sufficient to avoid false positives. For one, if nil is a valid implementation of an interface, returning a nil-value of that type is of course totally fine. Then, with the sort.StringSlice example, there are no nil-checks in any methods - but there don't *have* to be, because `len` on a nil-slice returns zero and the contract of the interface guarantees that Swap and Less are never called with out-of-bound indices. Note, that there is no way to check for this programmatically - for a tool, if StringSlice.Swap is ever called, with any arguments, on a nil-value, it would panic.

So the implementation of sort.StringSlice is totally fine as is and it's totally reasonable to use a nil-value as a sort.Interface (e.g. say you declare a variable of that type and then fill it conditionally with append - you might end up with a nil-slice) but would be reported as a false-positive.

Come to think of it, this actually returns false-positives even with the most strict implementation of the check - that is, if it only triggers if *all* code-paths at the interface creation-point return nil and if *all* code-paths in the method actually panic. Because even that check would falsely flag sort.StringSlice.

Even more sensible then, to implement this as a separate tool, to see what kind of false-positives it comes up with and how many true-positives it finds :)

[Burak Serdar]

On Thu, Jun 27, 2019 at 10:06 AM Axel Wagner
<axel.wa...@googlemail.com> wrote:
>
> Can you post some working code to the playground? ISTM that the code you shown wouldn't compile (X has both a Next field and a Next method). Also, the receiver is called X, but you return an *Item. That's not totally hypothetical as a follow-up, because in general, linked lists are an example of where IMO a nil-value can be made a completely useful implementation of an interface, by putting a nil-check in the methods. However, it's hard to talk about it concretely, without working code to adapt to show the idea :)

Sorry for the sloppy code. The first email I sent has a working
example to illustrate. My real code where I had this was in the
context of an ssh-based library with a bastion host. Here's a summary
version:

https://play.golang.org/p/tNmWWb_28qZ

When I first wrote this, I had a Host struct, with func (h Host)
GetVia() *Host . After it worked, I changed the Host to an interface,
HostIntf and added Via() HostIntf function to it, and ended up with an
enabled recursion, because h.Via() is never nil.

>
> On Thu, Jun 27, 2019 at 5:46 PM Burak Serdar <bse...@ieee.org> wrote:
>>
>> - function returns an interface Y
>> - there is a return X where X is not of type Y and is a reference
>> - X is not assigned a value in at least one code path
>> - there are no nil-checks for X
>
>
> IMO this is not sufficient to avoid false positives. For one, if nil is a valid implementation of an interface, returning a nil-value of that type is of course totally fine. Then, with the sort.StringSlice example, there are no nil-checks in any methods - but there don't *have* to be, because `len` on a nil-slice returns zero and the contract of the interface guarantees that Swap and Less are never called with out-of-bound indices. Note, that there

If you do "return sort.StringSlice(nil)", then the return value is
created/assigned in the function body, so it wouldn't alert. It would
only alert if the function gets a reference value from somewhere else,
converts it to an interface and returns it. So the following would be
ok:

func f() sort.Interface {
return sort.StringSlice(nil)
}

But this would be a false-positive:

func g() sort.StringSlice {
return sort.StringSlice(nil)
}

func f() sort.Interface {
return g()

}



is no way to check for this programmatically - for a tool, if
StringSlice.Swap is ever called, with any arguments, on a nil-value,
it would panic.
> So the implementation of sort.StringSlice is totally fine as is and it's totally reasonable to use a nil-value as a sort.Interface (e.g. say you declare a variable of that type and then fill it conditionally with append - you might end up with a nil-slice) but would be reported as a false-positive.

You are talking about analyzing what happens after function returns. I
am talking about only analyzing the function that causes the possible
nil-value-interface. Above, only the function f() would be analyzed,
and the first copy would be ok, and the second copy not, which, in a
way makes sense because f() doesn't know what g() does, and if g()
returns a nil instead of a nil-value-interface, then f() will hide
that nil.

[Axel Wagner]

I don't quite understand the logic you are proposing - but my point still stands: I believe you should first try and implement it as a separate tool. There is no real downside to that approach; it's just as useful as its own tool. And with a working implementation, we can concretely check its false-/true-positive rate against existing Go code. It should be fairly painless to actually move it into vet, once it exists.

[Burak Serdar]

On Thu, Jun 27, 2019 at 12:10 PM Axel Wagner
<axel.wa...@googlemail.com> wrote:
>
> I don't quite understand the logic you are proposing - but my point still stands: I believe you should first try and implement it as a separate tool. There is no real downside to that approach; it's just as useful as its own tool. And with a working implementation, we can concretely check its false-/true-positive rate against existing Go code. It should be fairly painless to actually move it into vet, once it exists.

I'll do that when I have some time. Thanks for the suggestions.