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

[rsc]

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

[findleyr]

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 [email protected] milestone.

[adonovan]

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]

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]

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]

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]

@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.