x/tools/cmd/deadcode: add new command

[adonovan]

The golang.org/x/tools/internal/cmd/deadcode command reports dead code, that is, functions not reachable by a sequence of calls from main, even through reflection, based on an RTA-based call graph. We have found it useful for identifying orphaned bits of code in x/tools and we imagine others might find it useful too. We propose to publish it at cmd/deadcode in the same repo.

This is an example of the tool in action, showing functions in x/tools not reachable from gopls' main or any of its tests, and suppressing functions in generated source files.

xtools$ deadcode -test ./gopls/...
golang.org/x/tools/gopls/internal/lsp
	openClientEditor

golang.org/x/tools/gopls/internal/lsp/cache
	(fileLoadScope).aScope
	(packageLoadScope).aScope
	(moduleLoadScope).aScope
	(viewLoadScope).aScope

golang.org/x/tools/gopls/internal/lsp/debug/log
	(Level).Log
	(Level).Logf

golang.org/x/tools/gopls/internal/lsp/filecache
	SetBudget

golang.org/x/tools/gopls/internal/lsp/regtest
	(State).outstandingWork

golang.org/x/tools/gopls/internal/lsp/source
	WidestPackageForFile

golang.org/x/tools/gopls/internal/lsp/template
	(*Parsed).WriteNode
	(wrNode).writeNode

xtools$ deadcode -test -json ./gopls/...
[
	{
		"Path": "golang.org/x/tools/gopls/internal/lsp",
		"Funcs": [
			{
				"Name": "golang.org/x/tools/gopls/internal/lsp.openClientEditor",
				"RelName": "openClientEditor",
				"Posn": "/Users/adonovan/w/xtools/gopls/internal/lsp/command.go:1178:6",
				"Generated": false
			}
		]
	},
...

[earthboundkid]

Cf @bep's https://github.com/bep/punused, which I have used to check for dead code before. That one can have false positive though.

[adonovan]

I'd particularly interested in feedback on how we could improve the user interface to make it clearer. For example, the first command I ran to produce the example above used just ./gopls, so it did not include tests from subpackages, causing it to report some functions that are only used from those tests. Perhaps better defaults would make such mistakes less likely.

Please try it out and let us know what you find.

[earthboundkid]

-generated=false is awkward. -exclude-generated would be less awkward.

[hdonnay]

One thing I noticed is that it needs some logic around Example* functions in tests.

[adonovan]

it needs some logic around Example* functions in tests.

Logic to do what? The help message says:

Bear in mind that an Example test
function without an "Output:" comment is merely documentation:
it is dead code, and does not contribute coverage.

Are you suggesting that it should treat non-executed Example functions as somehow reachable? I think its current behavior is more sensible. It's also unlikely that anyone would casually delete an Example based on the report produced by this tool.

[rsc]

Why does the output print package "path"? Why not just print path, or perhaps like the go command # path?
package "path" is weird because that's not the syntax of a package declaration.

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[hdonnay]

Logic to do what? The help message says:

Bear in mind that an Example test
function without an "Output:" comment is merely documentation:
it is dead code, and does not contribute coverage.

Are you suggesting that it should treat non-executed Example functions as somehow reachable? I think its current behavior is more sensible. It's also unlikely that anyone would casually delete an Example based on the report produced by this tool.

I can see the logic in that, but having the tool not recognize functions that are meaningful to the standard go tooling feels like friction for most users. The invocation is going to become deadcode . | grep -v Example for users to remove the technically-correct-but-not-useful output.

[gopherbot]

Change https://go.dev/cl/539661 mentions this issue: internal/cmd/deadcode: support -json, -format=template

[findleyr]

-generated=false is awkward. -exclude-generated would be less awkward.

I think this depends on the default behavior. If generated files are excluded by default, then -generated=true or -include_generated seems clearer than the doubly negative -exclude_generated=false.

And based on my experience with this command, I think generated files should be excluded by default. I almost always want to exclude generated code, and commands should err on the side of less noise.

[gopherbot]

Change https://go.dev/cl/540218 mentions this issue: internal/cmd/deadcode: omit package/func keywords in default output

[rsc]

One small note about the output: the parens in (*Parsed).WriteNode are both pedantic and harmful.
Pedantic because there is no potential for confusion, since there is only one actual method definition in the source code, so distinguishing pointer vs non-pointer receiver is unnecessary.
Harmful because the result is different syntax from what go doc uses,
so you can't paste the output into a go doc command line.
This should be printed as plain 'Parsed.WriteNode'.
Same thing for (Level).Log: just Level.Log.

[rsc]

In the JSON, the fields "RelName" and "Posn" seem to be new spellings for Go. Are they necessary?
Posn in particular seems like it should be Pos or Position, following our usual spellings.
And maybe Name/RelName should be ID/Name?

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

See the top comment.

[adonovan]

Pedantic because there is no potential for confusion, since there is only one actual method definition in the source code, so distinguishing pointer vs non-pointer receiver is unnecessary.

Fair enough. (These names from go/ssa, which must be able to distinguish (T).f from (*T).f.)

In the JSON, the fields "RelName" and "Posn" seem to be new spellings for Go. Are they necessary?

Perhaps not: once we've simplified (*T).f to T.f then package-qualifying it is as simple as pkg.T.f. Nice.

[gopherbot]

Change https://go.dev/cl/541238 mentions this issue: internal/cmd/deadcode: simplify (*T).f -> T.f in output

[rsc]

Sounds like RelName can be deleted. Posn should be kept but changed to Pos or Position.

[rsc]

I'm still not sure about the semi-structured default output. It is easy to fall back on "well it doesn't matter, programs will use json", but really the default output should be as usable for people as we can make it, including people typing at a Unix prompt. The current output is not greppable, for example. Also, I think it would make sense to think of deadcode as like vet in that it flags code that the user should take a look at. That strongly suggests the position belongs in the default output, so that it is easy to navigate to that code. So the output should probably be individual lines, not stateful sections, and each line should begin with the position:

file:line: pkg.name

And the file:line should default to relative to the current directory, like go vet and go build do. With the file:line, the full package path is probably unnecessary:

$ deadcode ./...
internal/lsp/command.go:1178:6: unused func: openClientEditor 
...

The "unused func:" is important because deadcode might also be printing ordinary build errors.
It also leaves the door open to "unused var" in the future (or maybe that's already there?).

[gopherbot]

Change https://go.dev/cl/542375 mentions this issue: internal/cmd/deadcode: use compiler-style diagnostics by default

[adonovan]

Sounds like RelName can be deleted. Posn should be kept but changed to Pos or Position.

Yep, that was done in CL 541238.

I'm still not sure about the semi-structured default output.

OK, done in 542375. I chose "unreachable" over "unused" because it's a property of the call graph, not the defs/uses graph.

$ go run ./internal/cmd/deadcode -test ./gopls/...
gopls/internal/lsp/command.go:1206:6: unreachable func: openClientEditor
gopls/internal/lsp/cache/pkg.go:100:22: unreachable func: fileLoadScope.aScope
...

The old format is now just an example -f template in the documentation.

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

See the top comment.

[gopherbot]

Change https://go.dev/cl/547035 mentions this issue: cmd/deadcode: publish internal/cmd/deadcode