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.
On Thu, Jun 27, 2019 at 6:32 PM Burak Serdar <bser...@ieee.org> wrote: > On Thu, Jun 27, 2019 at 10:06 AM Axel Wagner > <axel.wagner...@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 <bser...@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. > > > > > 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 :) > > > >> > >> > >> then it is likely that function will mask a nil-return. > >> > >> > >> > > >> > 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 :) > >> > > >> > On Thu, Jun 27, 2019 at 4:20 PM Burak Serdar <bser...@ieee.org> > wrote: > >> >> > >> >> 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? > >> >> > >> >> -- > >> >> 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+unsubscr...@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. > -- 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+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAEkBMfEKk39S0-QVT7FzTv%3DWP8N%3Dwjx1N5KF4%2BExoCT9wZya1g%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
