code review 12507043: cmd/vet: add an "unused" module

cmd/vet: add an "unused" module
The algorithm is straightforward although some corner cases are tricky.
We use the go/types Object map to iterate over the identifiers. A used
identifier will appear at least twice: once at the declaration, once at the use.
An unused identifier will appear only once: at its declaration. Thus we
reference count the identifiers and those with only one appearance
are errors. Moreover, by luck, that one instance is the correct place
to flag the error.

Tricky cases:
- implicit declarations such as in import clauses and type switches.
        - handled through the Implicits map.
- methods used only through interfaces
        - done by cross-checking methods and interfaces
- fields in nonce structs (these appear often in tests)
        - not handled yet

The nonce struct case is unsolved yet because the right heuristic is not
clear.

This check is enabled by default. It's a little noisy, especially inside tests,
but it's already found lots of cruft in the standard library so I think it's
worth turning on. Heuristics will improve to reduce noise.

Fixes issue 4827.

[r, 2013-08-06 06:18:33]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.tools

[Dominik Honnef, 2013-08-07 02:01:04]

https://codereview.appspot.com/12507043/diff/1/cmd/vet/unused.go
File cmd/vet/unused.go (right):

https://codereview.appspot.com/12507043/diff/1/cmd/vet/unused.go#newcode20
cmd/vet/unused.go:20: // use increments the reference count for the identifier,
if it's not exported and not the blank identifier.
"useIdentifier increments ..."

https://codereview.appspot.com/12507043/diff/1/cmd/vet/unused.go#newcode43
cmd/vet/unused.go:43: if obj == nil {
Same condition twice, collapse into single if?

https://codereview.appspot.com/12507043/diff/1/cmd/vet/unused.go#newcode179
cmd/vet/unused.go:179: // This doesn't count as a use of the actual interface.
Just having a type satisfy it its not sufficient;
its → is

[r, 2013-08-07 04:17:25]

Hello golang-dev@googlegroups.com, dominik.honnef@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[gri, 2013-08-07 22:40:42]

What's the reason for having a use _count_ as opposed to a _use_ bit? Is it so
that you don't have to distinguish between lhs and rhs of an assignment? Is it
possible the the count heuristic can fail in cases where a use bit would not?

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/main.go#newcode48
cmd/vet/main.go:48: "unused":      flag.Bool("unused", false, "check for unused
unexported package-level identifiers"),
CL desc says it's on by default

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/testdata/unused.go
File cmd/vet/testdata/unused.go (right):

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/testdata/unused.go#...
cmd/vet/testdata/unused.go:19: // Implicitly declared variables in type switches
should not trigger an error.
- should also have a test with an explicitly declared variable
- also, please add a test of the form

switch t := t; t := t.(type) { ... }

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go
File cmd/vet/unused.go (right):

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode10
cmd/vet/unused.go:10: "code.google.com/p/go.tools/go/types"
put this at the bottom in separate section

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode83
cmd/vet/unused.go:83: // Now the types, which may contain functions, as in f(fn
func(a int)): we need to mark "a".
one might have an in-line interface type - presumably all methods of that
interface should be marked as well

perhaps have a useType function instead?

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode96
cmd/vet/unused.go:96: case *ast.FuncType:
same here: factor out this code into a useType function

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode114
cmd/vet/unused.go:114: case *ast.FuncType:
useType

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode124
cmd/vet/unused.go:124: // useMethod remember the method declaration, if the
method is not exported,
remembers

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode146
cmd/vet/unused.go:146: case *ast.Ident:
I'd check for the base type and then do the indirect only once.

recvPtrType = types.NewPointer(deref(pkg.idents[node].Type()))

and define:

// If typ is of the (unnamed) form *T, deref(typ) returns T, otherwise it
returns typ.
func deref(typ types.Type) types.Type {
   if p, _ := typ.(*types.Pointer); p != nil {
      typ = p.Elem()
   }
   return typ
}

will be easier to read

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode148
cmd/vet/unused.go:148: recvPtrType = types.NewPointer(pkg.idents[node].Type())
baseTypeObj = pkg.idents[node]

and factor out the NewPointer computation ?

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode151
cmd/vet/unused.go:151: recvPtrType =
types.NewPointer(pkg.idents[node.X.(*ast.Ident)].Type())
similarly

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode154
cmd/vet/unused.go:154: }
recvPtrType := types.NewPointer(baseTypeObj.Type()) ?

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode155
cmd/vet/unused.go:155: // Search all interfaces for types this method appears
in.
this comment is hard to understand - you are not searching the interfaces for
types, you are searching for types that might satisfy the interfaces, don't you?

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode156
cmd/vet/unused.go:156: for _, interSpec := range pkg.interfaces {
s/inter/iface/

iface seems established shortcut - inter is a bit confusing

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode162
cmd/vet/unused.go:162: if !types.IsAssignableTo(recvPtrType, interType) {
Once a recvType is assignable to the interType, doesn't that mean that the
intersection of methods (i.e., all methods the exist on the interface type) are
used?

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode165
cmd/vet/unused.go:165: // We're using the method.
// We might be using the method.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode167
cmd/vet/unused.go:167: // We're using this method of the interface. Searching
for it by name is good enough.
There may be multiple non-exported names that are the same (because the package
path needs to be considered as well). You can use an objects' Id() as a unique
identifier for comparison.

[r, 2013-08-08 06:38:36]

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/main.go#newcode48
cmd/vet/main.go:48: "unused":      flag.Bool("unused", false, "check for unused
unexported package-level identifiers"),
On 2013/08/07 22:40:42, gri wrote:
> CL desc says it's on by default

It is on by default. Notice that all the others are false too, except "all" is
true and this is a map. They are enabled through the vet function, in particular
line 61:

      return *report["all"] || *report[name]

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/testdata/unused.go
File cmd/vet/testdata/unused.go (right):

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/testdata/unused.go#...
cmd/vet/testdata/unused.go:19: // Implicitly declared variables in type switches
should not trigger an error.
On 2013/08/07 22:40:42, gri wrote:
> - should also have a test with an explicitly declared variable
> - also, please add a test of the form
> 
> switch t := t; t := t.(type) { ... }

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go
File cmd/vet/unused.go (right):

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode10
cmd/vet/unused.go:10: "code.google.com/p/go.tools/go/types"
On 2013/08/07 22:40:42, gri wrote:
> put this at the bottom in separate section

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode83
cmd/vet/unused.go:83: // Now the types, which may contain functions, as in f(fn
func(a int)): we need to mark "a".
On 2013/08/07 22:40:42, gri wrote:
> one might have an in-line interface type - presumably all methods of that
> interface should be marked as well
> 
> perhaps have a useType function instead?

i couldn't make this work. need to think about it some more. the interaction of
ast.Nodes with Type fields, FuncTypes and InterfaceTypes and StructTypes,
types.Type, TypeSpecs and so on makes this all a hall of mirrors. today i got a
compiler message that said roughly,

  can't use typ of type types.Type as arg to Type; missing method method

i'll take another crack at it tomorrow.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode124
cmd/vet/unused.go:124: // useMethod remember the method declaration, if the
method is not exported,
On 2013/08/07 22:40:42, gri wrote:
> remembers

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode146
cmd/vet/unused.go:146: case *ast.Ident:
On 2013/08/07 22:40:42, gri wrote:
> I'd check for the base type and then do the indirect only once.
> 
> recvPtrType = types.NewPointer(deref(pkg.idents[node].Type()))
> 
> and define:
> 
> // If typ is of the (unnamed) form *T, deref(typ) returns T, otherwise it
> returns typ.
> func deref(typ types.Type) types.Type {
>    if p, _ := typ.(*types.Pointer); p != nil {
>       typ = p.Elem()
>    }
>    return typ
> }
> 
> will be easier to read

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode148
cmd/vet/unused.go:148: recvPtrType = types.NewPointer(pkg.idents[node].Type())
On 2013/08/07 22:40:42, gri wrote:
> baseTypeObj = pkg.idents[node]
> 
> and factor out the NewPointer computation ?

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode151
cmd/vet/unused.go:151: recvPtrType =
types.NewPointer(pkg.idents[node.X.(*ast.Ident)].Type())
On 2013/08/07 22:40:42, gri wrote:
> similarly

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode154
cmd/vet/unused.go:154: }
On 2013/08/07 22:40:42, gri wrote:
> recvPtrType := types.NewPointer(baseTypeObj.Type()) ?

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode155
cmd/vet/unused.go:155: // Search all interfaces for types this method appears
in.
On 2013/08/07 22:40:42, gri wrote:
> this comment is hard to understand - you are not searching the interfaces for
> types, you are searching for types that might satisfy the interfaces, don't
you?

I'm doing what it says. Loop over the interfaces, find ones that the type
satisfies. But I've rejiggered a bit by adding an extra loop to see that the
method appears in the interface.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode156
cmd/vet/unused.go:156: for _, interSpec := range pkg.interfaces {
On 2013/08/07 22:40:42, gri wrote:
> s/inter/iface/
> 
> iface seems established shortcut - inter is a bit confusing

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode162
cmd/vet/unused.go:162: if !types.IsAssignableTo(recvPtrType, interType) {
On 2013/08/07 22:40:42, gri wrote:
> Once a recvType is assignable to the interType, doesn't that mean that the
> intersection of methods (i.e., all methods the exist on the interface type)
are
> used?

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode165
cmd/vet/unused.go:165: // We're using the method.
On 2013/08/07 22:40:42, gri wrote:
> // We might be using the method.

Done.

https://codereview.appspot.com/12507043/diff/6001/cmd/vet/unused.go#newcode167
cmd/vet/unused.go:167: // We're using this method of the interface. Searching
for it by name is good enough.
On 2013/08/07 22:40:42, gri wrote:
> There may be multiple non-exported names that are the same (because the
package
> path needs to be considered as well). You can use an objects' Id() as a unique
> identifier for comparison.

Done.

[r, 2013-08-08 06:38:57]

Hello golang-dev@googlegroups.com, dominik.honnef@gmail.com, gri@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2013-08-08 06:59:54]

For the record, the actual message was:

./unused.go:149: cannot use method.Recv.List[0].Type (type ast.Expr) as
type types.Type in function argument:
ast.Expr does not implement types.Type (missing MethodSet method)

The word "type" appears seven times.

[gri, 2013-08-08 23:16:59]

looking forward to the general useType function

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/main.go#newcode448
cmd/vet/main.go:448: if d.Body == nil {
why is this here and below? could use a comment.

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/unused.go
File cmd/vet/unused.go (right):

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/unused.go#newcode17
cmd/vet/unused.go:17: file  *File // File holding first use we see. For unused
items, this is sure to be the declaration.
I don't understand why this is true. On the package level, an identifier might
be used textually before it's declaration if this code operates on more than one
file.

[r, 2013-08-09 00:37:08]

Please explain for the incompetent here how the useType function would
work. What is its signature? Keep in mind I need a TypeSpec in there in
some cases but don't always have one.

-rob

[r, 2013-08-09 00:38:27]

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/main.go#newcode448
cmd/vet/main.go:448: if d.Body == nil {
On 2013/08/08 23:16:59, gri wrote:
> why is this here and below? could use a comment.

because it's of the form

func implementInAssembler()

will add a comment.

-rob

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/unused.go
File cmd/vet/unused.go (right):

https://codereview.appspot.com/12507043/diff/14001/cmd/vet/unused.go#newcode17
cmd/vet/unused.go:17: file  *File // File holding first use we see. For unused
items, this is sure to be the declaration.
On 2013/08/08 23:16:59, gri wrote:
> I don't understand why this is true. On the package level, an identifier might
> be used textually before it's declaration if this code operates on more than
one
> file.

It's only true for *unused* identifiers. That's the whole point of this file.

[r, 2013-08-13 05:30:02]

Hello golang-dev@googlegroups.com, dominik.honnef@gmail.com, gri@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[r, 2013-08-22 23:14:36]

Anyone else want to look at this? Robert is out and it would be nice to let
people try this new vet module to help me refine it.

Also I have an idea about the heuristic for struct tags but don't want to make
this CL any bigger.

[remyoudompheng, 2013-08-23 05:57:26]

I've used the module and noticed that when the binary packages are not
installed, the import fails silently and instead of an error it gives weird
false positives.

[crawshaw1, 2013-08-23 19:54:54]

https://codereview.appspot.com/12507043/diff/22001/cmd/vet/main.go
File cmd/vet/main.go (right):

https://codereview.appspot.com/12507043/diff/22001/cmd/vet/main.go#newcode448
cmd/vet/main.go:448: if d.Body == nil {
This case is covered below.

https://codereview.appspot.com/12507043/diff/22001/cmd/vet/unused.go
File cmd/vet/unused.go (right):

https://codereview.appspot.com/12507043/diff/22001/cmd/vet/unused.go#newcode134
cmd/vet/unused.go:134: // crossReferenceMethods connects unused methods with the
interfaces they might satisfy,
Adding

type c interface { methodp() } // line 62

func Usec(x c) { x.methodp() }

func Makec() c { return new(T) }

to testdata/unused.go gives

testdata/unused.go:62: declared but not used: methodp

https://codereview.appspot.com/12507043/diff/22001/cmd/vet/unused.go#newcode186
cmd/vet/unused.go:186: pkg.uses[obj].count++
type c interface {
  String()
  f()
}

var ac c = AC(7)

type AC int

func (AC) String() { }
func (AC) f() {}

func UseAC() { ac.f() }

gives

panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x8 pc=0x416dec]

goroutine 1 [running]:
main.(*Package).crossReferenceMethods(0xc210047120)
        /home/crawshaw/go/src/code.google.com/p/go.tools/cmd/vet/unused.go:186
+0x5fc
main.(*Package).unusedCheck(0xc210047120)
        /home/crawshaw/go/src/code.google.com/p/go.tools/cmd/vet/unused.go:225
+0x6c
main.doPackage(0x615fe0, 0x1, 0xc21000a020, 0x1, 0x1, ...)
        /home/crawshaw/go/src/code.google.com/p/go.tools/cmd/vet/main.go:264
+0xb3f
main.main()
        /home/crawshaw/go/src/code.google.com/p/go.tools/cmd/vet/main.go:159
+0x665

[r, 2013-08-27 05:03:48]

*** Abandoned ***