fix(tools/checklocks): Add generics support #11740
Conversation
This commit introduces support for Go generics in the checklocks analyzer, addressing issues with false-positive warnings and ensuring correct behavior with generic types. Key changes include: - Updated lock guard fact handling in analysis.go for generic types. - Added test cases for generics in tools/checklocks/test/generics.go. - Resolved regressions specific to generic value guards. All tests related to generics now pass. Pre-existing baseline failures in closure and defer handling are out of scope for this change. Fixes google#11671 Signed-off-by: Kemal Akkoyun <[email protected]>
kakkoyun
force-pushed
the
fix_generics_draft
branch
from
e2bec2b to
a195741
Compare
| // This statement should be unreachable, but the Go compiler cannot prove | ||
| // that the above type switch returns in all cases (due to `break` in some | ||
| // branches). Provide a safe default to satisfy the type checker. | ||
| return fmt.Sprintf("{unknown:%T:%p}", v, v), nil |
There was a problem hiding this comment.
Please panic instead, since this should be unreachable.
| // itself. | ||
| func genericInstantiate() { | ||
| var _ genericGuardStruct[int] | ||
| } |
There was a problem hiding this comment.
What would happen in the case where a library defines the genericGuardStruct without ever instantiating it, as all instantiations of genericGuardStruct would be in other libraries that import it? I think this would be a more common case than what this test file is doing.
| @@ -495,39 +495,46 @@ func (pc *passContext) checkFunctionCall(call callCommon, fn *types.Func, lff *l | |||
| // Update all lock state accordingly. | |||
| pc.postFunctionCallUpdate(call, lff, ls, false /* aliases */) | |||
|
|
|||
| // Fast-path handling for sync.(RW)Mutex/Locker helper methods. | |||
| if fn == nil || fn.Pkg() == nil || len(args) == 0 { | |||
There was a problem hiding this comment.
I'm not sure I understand how this works, but this feels brittle. What is this skipping over?
| case *types.Named: | ||
| obj := t.Obj() | ||
| if obj != nil && obj.Pkg() != nil { | ||
| if obj.Pkg().Path() == "sync" && (obj.Name() == "Mutex" || obj.Name() == "RWMutex") { |
There was a problem hiding this comment.
Will this work for the gVisor codebase, which uses import path gvisor.dev/gvisor/pkg/sync?
https://github.com/search?q=repo%3Agoogle%2Fgvisor+%22%5C%22gvisor.dev%2Fgvisor%2Fpkg%2Fsync%5C%22%22&type=code
| cur := typ | ||
| for { | ||
| if _, dup := seen[cur]; dup { | ||
| // Cycle guard, should never happen. |
Summary
This PR introduces support for Go generics to the
tools/checklocksstatic analysis tool. It resolves issues wherechecklockswould produce false-positive warnings ("may require checklocks annotation") for mutex fields within generic types and ensures that+checklocksfailannotations behave correctly with generic code.The core of the changes involves updating the type resolution and fact-finding logic within
analysis.goandfacts.goto correctly interpret lock annotations in the context of generic structs and their instantiations.Key Changes
analysis.goto correctly resolve types and associate lock annotations for fields within generic structs. This includes improvements to how guarded-by facts are discovered and applied, particularly usingOrigin()to trace back to generic definitions.isMutexTypeto accurately identify mutex types, including those used within generic contexts.*ssa.Allocand*ssa.Storeoperations instate.goandanalysis.gorespectively, which was crucial for resolving a regression related to lock state propagation in generic helper functions.tools/checklocks/test/generics.gowith specific test cases to validatechecklocksbehavior with generic resources, including scenarios with+checklocksand+checklocksfailannotations.analysis.goto use a canonical helper function (importLockGuardFacts) for importing lock guard facts, reducing code duplication. Removed extensive debug logging now that the core functionality is stable.Impact & Testing
checklocksno longer produces false-positive "may require checklocks annotation" warnings for mutex fields in generic types.+checklocksfailannotation now correctly identifies (or not identify) violations in test cases involving generics.tools/checklocks/test/generics.gonow pass as expected.test/closures.goandtest/defer.go(which predate this work and exist in the main branch) are unaffected and considered out of scope for this PR.Fixes #11671
Signed-off-by: Kemal Akkoyun [email protected]