assigned and not used

[gordon...@gmail.com]

After filing [0], I wrote a tool [1] to investigate the occurrence of the proposed "assigned and not used" error.  Running it on the standard library and whatever was in my GOPATH (not much) turned up 72 instances, most of which are just sloppy code, but 7 of which are clearly bugs:

cmd/compile/internal/big/floatconv.go:367:2 m

cmd/cover/cover_test.go:62:2 err

cmd/pprof/internal/profile/profile.go:131:10 err

math/big/ftoa.go:285:2 m

net/file_unix.go:66:7 err

golang.org/x/mobile/app/android.go:175:2 queue

golang.org/x/net/icmp/listen_posix.go:83:6 err

Should I file a single issue for all of these?

Some of the other instances may well be bugs.  I encourage you to look at the full lists (in [1]) and to run it on your own code.

I suppose this should be incorporated into go vet.  Anyone is welcome to adapt my code, or maybe I'll get around to it sometime.

[0] https://github.com/golang/go/issues/10989

[1] https://github.com/gordonklaus/ineffassign

[Ian Lance Taylor]

On Sat, May 30, 2015 at 1:44 PM, <gordon...@gmail.com> wrote:
>
> After filing [0], I wrote a tool [1] to investigate the occurrence of the
> proposed "assigned and not used" error. Running it on the standard library
> and whatever was in my GOPATH (not much) turned up 72 instances, most of
> which are just sloppy code, but 7 of which are clearly bugs:
>
> cmd/compile/internal/big/floatconv.go:367:2 m
> cmd/cover/cover_test.go:62:2 err
> cmd/pprof/internal/profile/profile.go:131:10 err
> math/big/ftoa.go:285:2 m
> net/file_unix.go:66:7 err
> golang.org/x/mobile/app/android.go:175:2 queue
> golang.org/x/net/icmp/listen_posix.go:83:6 err
>
> Should I file a single issue for all of these?

Please do. Thanks. Good analysis.

Ian

[alb.do...@gmail.com]

I would file separate issues.

Note that

cmd/compile/internal/big/floatconv.go:367:2 m

and

math/big/ftoa.go:285:2 m

are actually the same. The internal big package is just a copy
of the math/big one. In this case we have slighty different files because
the math/big floatconv.go was recently refactored (and the changes not
immediately ported to the internal package).

[Nigel Tao]

On Sun, May 31, 2015 at 6:44 AM, <gordon...@gmail.com> wrote:
> golang.org/x/mobile/app/android.go:175:2 queue
> golang.org/x/net/icmp/listen_posix.go:83:6 err

Thanks for the bug reports. I sent out:
https://go-review.googlesource.com/10560 to fix x/mobile
https://go-review.googlesource.com/10561 to fix x/net

I'm happy to fix the stdlib too, or review any changes, if somebody
else wants the commit log glory.

[Dave Cheney]

Thanks Gordon. I've proposed fixes for all of these errors.

[Dave Cheney]

Nigel, Ian, with absolutely no experience with the history of trying add this kind of check to vet, Gordon's tool found some important errors (and found a tonne more in my projects), what do you say to adding this kind of check to vet ?

[Rob Pike]

I'm surprised the compiler's flow analysis didn't catch this. It should, and that's where the fix should be.

-rob

--
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.
For more options, visit https://groups.google.com/d/optout.

[Dave Cheney]

I agree. I'll log a bug.

[Dave Cheney]

Rob, https://github.com/golang/go/issues/11031.

I'll let you set the priority on this one.

Thanks

Dave

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.

[Gordon Klaus]

Thanks for proposing the fixes, Dave.

In case you guys missed it, I initially filed #10989 for the compiler.  As proposed, it would be a backwards-incompatible change that, as indicated by my tool, would break a lot of innocuous (if sloppy) code, in addition to pointing out real bugs (~10%).  I think it should be marked Go2.

For Go1.x, I think we could implement a weaker check in the compiler that would only catch true bugs by limiting its complaints to cases where the assigned variable is also shadowing.  This would miss, e.g., the math/big and cmd/cover bugs found by the stronger check, so I think we should also have the stronger check in go vet.

I should be able to implement the weaker check this week, and at the same time improve my tool, which is quite rough (it won't catch bugs inside loops or function literals).

Dave

To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

[Klaus Post]

Thanks for your great tool. Already found a bug, and some unclear code.

I hope this at least make it as a 'go vet', since it is good to spot unintentional shadowing.

/Klaus

[andrewc...@gmail.com]

Would it be good to put it into vet then do a fix for the subsequent release? 

[Rob Pike]

I reopened #10989. I still think the compiler should do this, if anyone should.

[andrewc...@gmail.com]

I agree, the compiler already is doing this check in a weaker form, so it makes sense to fix it there.

I meant to say add to the fix to vet first, then fix in the compiler next release and remove from vet then. That way people have a long time to fix things first,