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/gopls: spurious 'unused write' in package reflect #67684

Open
rsc opened this issue May 28, 2024 · 6 comments
Open

x/tools/gopls: spurious 'unused write' in package reflect #67684

rsc opened this issue May 28, 2024 · 6 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented May 28, 2024

I opened reflect/type.go and gopls started telling me:

/Users/rsc/go/src/reflect/type.go:1795.5,1795.5: unused write to field Dir
/Users/rsc/go/src/reflect/type.go:1798.5,1798.5: unused write to field Elem
/Users/rsc/go/src/reflect/type.go:2816.8,2816.8: unused write to field Elem
/Users/rsc/go/src/reflect/type.go:2831.8,2831.8: unused write to field Slice

This is false. The code for the first two errors is:

// Make a channel type.
var ichan any = (chan unsafe.Pointer)(nil)
prototype := *(**chanType)(unsafe.Pointer(&ichan))
ch := *prototype
ch.TFlag = abi.TFlagRegularMemory
ch.Dir = abi.ChanDir(dir)
ch.Str = resolveReflectName(newName(s, "", false, false))
ch.Hash = fnv1(typ.Hash, 'c', byte(dir))
ch.Elem = typ

ti, _ := lookupCache.LoadOrStore(ckey, toRType(&ch.Type))
return ti.(Type)

What the analyzer doesn't see is that &ch.Type provides access to all of ch.

/cc @rfindley @adonovan

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels May 28, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 28, 2024
@findleyr
Copy link
Contributor

CC @timothy-king

I suspect it would be enough for the analyzer to differentiate between implicit field access and explicit field references, bailing out for the latter. That would preserve most of the usefulness and avoid these types of false positives.

This looks (naively) like a pretty straightforward fix. Putting in the gopls@v0.17.0 milestone.

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.17.0 May 28, 2024
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label May 29, 2024
@adonovan
Copy link
Member

adonovan commented May 29, 2024

What the analyzer doesn't see is that &ch.Type provides access to all of ch.

toRType involves a C-style cast to coerce a *T pointer to a *struct{T; ...} pointer. I don't expect the analysis to be able to handle that, nor do I imagine this is a common thing to do in Go code outside the runtime.

Perhaps the analyzer should just be silent in packages that import "unsafe"?

@findleyr
Copy link
Contributor

Perhaps the analyzer should just be silent in packages that import "unsafe"?

That was my first thought as well, but there's no reason the cast must be in the same package as the field addr.

@adonovan
Copy link
Member

there's no reason the cast must be in the same package as the field addr.

Well, there kind of is: it is very bad form to expose unsafe public API.

There could be a constellation of internal packages working together this way, but that may be rare enough to justify the heuristic.

@MikeMitchellWebDev
Copy link
Contributor

What the analyzer doesn't see is that &ch.Type provides access to all of ch.

toRType involves a C-style cast to coerce a *T pointer to a *struct{T; ...} pointer. I don't expect the analysis to be able to handle that, nor do I imagine this is a common thing to do in Go code outside the runtime.

Perhaps the analyzer should just be silent in packages that import "unsafe"?

According to the information available here a significant number of packages import unsafe

@findleyr
Copy link
Contributor

@MikeMitchellWebDev pkg.go.dev reports over 4m importers of fmt, so the number of packages importing unsafe is at most 2-3%, probably more like 1-2%. Skipping this specific heuristic in the presence of unsafe seems acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants