x/tools/cmd/deadcode: Underscore in file path results in no dead code detection

[thesilentg]

Go version

go version go1.22.0 darwin/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN='/Users/aggnolek/go/bin'
GOCACHE='/Users/aggnolek/Library/Caches/go-build'
GOENV='/Users/aggnolek/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/aggnolek/go/pkg/mod'
GONOPROXY='*'
GONOSUMDB='*'
GOOS='darwin'
GOPATH='/Users/aggnolek/go'
GOPRIVATE='*'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/aggnolek/sdk/go1.22.0'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/aggnolek/sdk/go1.22.0/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/aggnolek/gorepos/aggnolek/scratchpad/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/x7/2f8ynt3954s4y78yt_54v9fr0000gs/T/go-build2810237142=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

go.mod

module scratchpad

go 1.21

_utils/utils.go

package main

func foo() {}

_utils/pkg/pkg.go

package pkg

func foo() {}

_utils/pkg2/pkg2.go

package main

func foo() {}

Ran deadcode ./...:

What did you see happen?

deadcode ./... had no output

What did you expect to see?

deadcode ./... to output:

_utils/utils.go:XX:X: unreachable func: foo
_utils/pkg/pkg.go:XX:X: unreachable func: foo
_utils/pkg2/pkg2.go:XX:X: unreachable func: foo

Note that simply changing the directory name from _utils to utils and rerunning deadcode ./... results in the expected behavior:

utils/utils.go:XX:X: unreachable func: foo
utils/pkg/pkg.go:XX:X: unreachable func: foo
utils/pkg2/pkg2.go:XX:X: unreachable func: foo

[seankhliao]

This is consistent with the go command, which ignores paths with leading dots or underscores.

https://pkg.go.dev/cmd/go#hdr-Import_path_syntax:~:text=Directory%20and%20file%20names%20that%20begin%20with%20%22.%22%20or%20%22_%22%20are%20ignored%20by%20the%20go%20tool%2C%20as%20are%20directories%20named%20%22testdata%22.

[thesilentg]

@seankhliao Thanks for that additional context, I wasn't aware of that. However, the behavior of deadcode is still inconsistent here. If package with a underscore path is included via the reachability analysis (because some of the code within it is live), then dead code from that package will be reported, despite the name. Consider the example below:

go.mod

module scratchpad

go 1.21

_utils/utils.go

package _utils

func Used() {}

func UnUsed() {}

example/main.go

package main

import "scratchpad/_utils"

func main() {
    _utils.Used()
}

The output of deadcode ./... here will report:

_utils/utils.go:X:X: unreachable func: UnUsed

Removing the call to _utils.Used() and rerunning deadcode ./... it will then report there is no dead code. This behavior is counter-intuitive, as removing code should never result in more functions being marked live. It appears that either:

1. _utils.UnUsed() should never be reported as dead in the first example (if the desire is to exactly match behavior with the go command)
2. _utils.UnUsed() (and _utils.Used()) should be reported as dead in the second example (if the desire is to report the maximum amount of unreachable code)

[thesilentg]

As another potential example of the challenges with the current behavior, consider the -whylive flag:

When _utils.Used() is present in main: deadcode -whylive scratchpad/_utils.Used ./..

                   scratchpad/example.main
  static@L0061 --> scratchpad/_utils.Used

When _utils.Used() is not present in main: deadcode -whylive scratchpad/_utils.Used ./..

deadcode: function "scratchpad/_utils.Used" not found in program

Instead, the second example should likely report that _utils.Used is dead code (not that it is not found).

[seankhliao]

This is still consistent with cmd/go: explicit imports are always part of the build, otherwise ./... has the meaning as documented.

[thesilentg]

Appreciate your follow-up clarifications and totally agree with the high-level desire to maintain consistency between go tools whenever possible.

Working-backwards from how I expect customers of this tool to use deadcode, the most likely use case seems to be:

1. Requesting all the deadcode in a given module: deadcode ./...
2. Drilling-down on the data for individual packages: deadcode {packagename}
3. Inspecting why a specific function is not named: deadcode -whylive {functionname}

Does that mental model of expected / typical customer usage match your own?

If so, it seems like the current behavior could lead to customer confusion or missed deadcode detection, for a few different reasons:

1. It seems relatively unlikely that users would be aware of the underscore-based restrictions when running the deadcode tool. Anecdotally, asking developers at my workplace, for which Go is their primary language for daily software development, only 2 out of 19 were aware of this behavior. While I appreciate and recognize that the behavior is de jure documented, in practice, Go code is written and builds without error when placed in paths or packages with names containing underscores, reducing notice for this potential pitfall. The fact that these packages are being silently and implicitly filtered out for deadcode analysis (as compared to any sort of warning / error) also reduces the chance that this behavior will be noticed by engineers.

2. The path-level restriction means that underscores that are "early" in the path, can have a high impact on dead code analysis. While filtering of package names or "leaf" directories containing underscores would be non-ideal, this would still much more narrowly scope the limits of this filtering compared to the current behavior today. In contrast, a path like internal/_clients/backend/{client}.go is going to unknowing filter out a lot of code.

3. Unlike other go tools, in which unused packages containing underscores have no impact on tool execution, deadcode specifically is trying to identify functions that are not used. This leads to adverse behavior where the very packages / functions that deadcode is trying to target are those least likely to be imported by other packages. These are thus less likely to be force included in the build graph (via imports) and thus more likely to be silently filtered out of deadcode reporting given the current behavior.

A good analogy to the challenge here in my mind is Go's original treatment of test coverage for packages that had no tests. Rather than reporting that such packages were untested when calculating overall test coverage, Go originally did not include these packages in coverage reporting, leading to users mistakenly having incorrect coverage measurements due to this implicit filtering. Eventually, this was changed such that packages without tests were still included in coverage reporting when reporting on overall coverage.

I see the behavior of deadcode today in a very similar way, in which the benefits of more complete and accurate dead code detection potentially are worth breaking consistency with how other go tools work today.

Thoughts?

[timothy-king]

Closed issues do not get much visibility. If you think there is something to be done about the clarity of package lists and patterns, I would encourage you to open a different bug. This could be requests for docs, a blog post, more code examples, better examples of tool usage, a new reserved name in package lists, etc. Your informal survey seems to indicate that this the root cause of this issue.