Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/cmd/deadcode: unexported methods not reported as unreachable #65915

Closed
thesilentg opened this issue Feb 23, 2024 · 11 comments
Closed

x/tools/cmd/deadcode: unexported methods not reported as unreachable #65915

thesilentg opened this issue Feb 23, 2024 · 11 comments
Assignees
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@thesilentg
Copy link

thesilentg commented Feb 23, 2024

Go version

go version go1.21.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='/usr/local/opt/go/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/opt/go/libexec/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
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-build3150741677=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

go.mod

module scratchpad

go 1.21

example/main.go

package main

type example struct{}

func (e example) UnUsed() {}

func (e example) Used() {}

func (e example) unUsed() {}

func (e example) used() {}

type PublicExample struct{}

func (p PublicExample) UnUsed() {}

func (p PublicExample) Used() {}

func (p PublicExample) unUsed() {}

func (p PublicExample) used() {}

func main() {
	example{}.Used()
	example{}.used()
	PublicExample{}.Used()
	PublicExample{}.used()
}

What did you see happen?

The output of deadcode ./... reports:

example/main.go:15:24: unreachable func: PublicExample.UnUsed
example/main.go:19:24: unreachable func: PublicExample.unUsed

In this case, the non-used methods on example (UnUsed and unUsed) should be reported as unreachable, but they weren't.

The output of deadcode -whylive scratchpad/example.example.UnUsed ./... mistakenly reports:

deadcode: function "scratchpad/example.example.UnUsed" not found in program

The output of deadcode -whylive scratchpad/example.PublicExample.UnUsed ./... correctly reports:

deadcode: function scratchpad/example.PublicExample.UnUsed is dead code

What did you expect to see?

The output of deadcode ./... should report:

example/main.go:9:18: unreachable func: example.UnUsed
example/main.go:11:18: unreachable func: example.unUsed
example/main.go:15:24: unreachable func: PublicExample.UnUsed
example/main.go:19:24: unreachable func: PublicExample.unUsed

The output of deadcode -whylive scratchpad/example.example.UnUsed ./... should report:

deadcode: function "scratchpad/example.example.UnUsed" is dead code
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 23, 2024
@gopherbot gopherbot added this to the Unreleased milestone Feb 23, 2024
@findleyr
Copy link
Contributor

CC @adonovan

@seankhliao seankhliao changed the title x/tools/cmd/deadcode: Non-Exported Methods not reported as unreachable x/tools/cmd/deadcode: unexported methods not reported as unreachable Feb 24, 2024
@conditionals
Copy link

From my digging, here's what I've found -

in deadcode.go, we are utilizing ssautil's AllFunctions to get a list of functions from our program.

in ssautil/visit.go we have the following code:

switch mem := mem.(type) {
	case *ssa.Function:
		// Visit all package-level declared functions.
		function(mem)

	case *ssa.Type:
		exportedTypeHack(mem)
}

since the example struct is a type, and not a function, we call exportedTypeHack on it

exportedTypeHack, in turn, will only get named parameters if they are exported -- see ast.IsExported(t.Name()) on line 102 of go/ssa/ssautil/visit.go

this causes any functions belonging to the unexported example struct to not be included in our list returned by AllFunctions.

removing the call to ast.IsExported(t.Name()) will cause deadcode to produce the expected output for this specific file, and the tests outlined in testdata do all pass.

i myself am not positive that this will not induce negative side-effects elsewhere, but I figured i'd throw this out as a lead to someone who is more familiarized with this codebase than me

@gopherbot
Copy link

Change https://go.dev/cl/566421 mentions this issue: ssautil: remove export check on exportedTypeHack

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 26, 2024
@timothy-king
Copy link
Contributor

The fix seems reasonable, but...

i myself am not positive that this will not induce negative side-effects elsewhere, but I figured i'd throw this out as a lead to someone who is more familiarized with this codebase than me

I share this concern. This will need an investigation to make sure the cure is not worse than this issue.

@thesilentg
Copy link
Author

I will note that it is possible in some cases for methods on unexported structs to be correctly marked as deadcode. Running deadcode ./... on one of our codebases returns the following line (details redacted):

internal/{redacted}/file.go: unreachable func: unexportedStruct.ExportedMethod1
internal/{redacted}/{redacted}/file.go: unreachable func: unexportedStruct.unExportedMethod1

I unfortunately haven't been able to replicate what causes this to happen with a more succinct example that is sharable, but did want to clarify that in some cases these methods can actually be identified.

@adonovan adonovan self-assigned this Feb 26, 2024
@thesilentg
Copy link
Author

thesilentg commented Feb 26, 2024

Surprisingly, the addition of a fmt.Println(example{}) allows the deadcode detector to correctly identify example.unUsed as dead code:

example/main.go

// type definitions and defined methods are unchanged from above 
func main() {
	fmt.Println(example{})
	example{}.Used()
	example{}.used()
	PublicExample{}.Used()
	PublicExample{}.used()
}

The output of deadcode ./.. now reports:

example/main.go:11:18: unreachable func: example.unUsed
example/main.go:17:24: unreachable func: PublicExample.UnUsed
example/main.go:21:24: unreachable func: PublicExample.unUsed

The output of deadcode -whylive scratchpad/example.example.UnUsed ./... now reports:

deadcode: scratchpad/example.example.UnUsed is reachable only through reflection

@conditionals
Copy link

interestingly enough, when we call rta.Analyze in deadcode.go, we only check/set functions considered "Reachable" as true in the reachablePosn map, but it seems that when the fmt.Println() is added we start to consider almost everything reachable?

consider the following example, utilizing this print statement in main.go (not the best debugging tool I am aware :P)

example/main.go

type example struct{}

func (e example) UnUsed() {}
func (e example) Used()   {}

func (e example) unUsed() {}
func (e example) used()   {}

func main() {
	// fmt.Println(example{}) leaving this commented out in this example.
	example{}.Used()
	example{}.used()
}

as seen in deadcode.go

res := rta.Analyze(roots, *whyLiveFlag != "")

reachablePosn := make(map[token.Position]bool)

for fn := range res.Reachable {
if fn.Pos().IsValid() || fn.Name() == "init" {
	fmt.Println(fn.Name()) // added print statement here.
	reachablePosn[prog.Fset.Position(fn.Pos())] = true
}
}

will produce the following result -

used
init
main
Used
deadcode: function scratchpad/example.example.UnUsed is dead code

however, if we uncomment our println in main.go, it seems as though we print almost everything?
(edited for brevity, obviously)

pack
runqget
uvarint
Common
decref
full
releaseLockRank
newBucket
IsDirectIface
Kind
symPC
doaddtimer
chunkOf
next
addReflectOff
split
Chown
Width
HasName
Common
typeOff
Assign
alloc
findsghi
readLock
Seek
IfaceIndir
Unlock
typ
materializeGCProg
allocRange
Fchown$1
Release
casGToPreemptScan
newInlineUnwinder

(etc, etc, etc)

looking into why this happens now, but its quite interesting

@gopherbot
Copy link

Change https://go.dev/cl/567156 mentions this issue: cmd/deadcode: avoid ssautil.AllFunctions

@adonovan
Copy link
Member

adonovan commented Feb 26, 2024

Many thanks for the bug report and especially the minimal test case. The problem is indeed in AllFunctions, but the fix is not in that function: it's not to use it at all. AllFunctions performs a reachability analysis, which is not appropriate for the "denominator" of a dead code analysis. (The "numerator" is of course RTA, a more precise reachability analysis.) The correct denominator is the set of all source-level named functions, which is (happily) much cheaper to compute.

I mailed a fix in CL 567156.

@adonovan
Copy link
Member

[@thesilentg] Surprisingly, the addition of a fmt.Println(example{}) allows the deadcode detector to correctly identify example.unUsed as dead code:

AllFunction's use of a reachability analysis is the explanation of this paradoxical behavior: the print statement causes the example type to be added to RuntimeTypes, which causes all its methods to become reachable.

@thesilentg
Copy link
Author

Thanks for the prompt investigation, explanation, and fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants