On Thu, Jun 27, 2019 at 12:10 PM Axel Wagner <axel.wagner...@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. > > 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/CAMV2Rqp_zkVCjf8FFi7fgdJ8fdfmBaqdrc5cEXTWvxV1xpXs%3DQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
