feat(scanner): --strict-types forwarding-call detection, closing #26#31
Merged
Conversation
Implements ADR 006's approach A (the "forwarding function" pattern) for issue #26 — a method value captured in one function, passed as an argument, invoked from inside a different callee — then extends it past the ADR's own original scope once real-world verification against the ADR's own motivating repo (e2b-dev/infra) showed the actual shape needed more than a single hop. The real chain: a package-level var (e.g. SnapshotFeatureFlag) built by a factory (NewBoolFlag) that stores a literal into a struct field, read back through a trivial accessor method (Key), forwarded through TWO levels of function wrapping (a "pass-through" wrapper method that fixes the SDK method internally, then a generic "direct forwarding" function that actually invokes the callback) before reaching the SDK call. Closing this needed four new whole-scan passes, chained together in the same fixed-point spirit as factory.go's cross-file resolution (ADR 004) and issue #18's Pass B loop — still no go/ssa/go/callgraph (ADR 006's approach B): - accessorFields: trivial single-field-accessor methods (`func (f S) Key() string { return f.name }`) - factoryFieldParams: constructors that store a parameter into a composite-literal field of their own return type - flagDescriptorLiterals: package-level vars built via a known factory with a literal argument - findEvalSummaries: direct-forwarding functions (F calls its own callback parameter), then pass-through functions to a fixed point (G calls a function with an already-known summary, forwarding one of G's own parameters into its flag-descriptor position) — a chain can deepen one hop per round, closing the real two-level case Two real bugs found and fixed during verification against e2b-dev/infra itself, not caught by the (initially too-simple) fixtures: 1. A call through an interface-typed parameter (`flag.Key()` where `flag typedFlag[T]`) resolves via go/types to the *interface's* abstract method object — a completely different *types.Func than the concrete type's own implementation, even though only the concrete implementation has a body to learn a field from. Fixed by keying accessorFields by (type name, method name) instead of *types.Func identity, resolved against the concrete type only once a real value reaches an actual call site. 2. findDirectForwarding took the *first* argument referencing one of a function's own parameters as "the flag key" — wrong whenever an unrelated parameter (almost always ctx, conventionally first) comes before the real flag-descriptor argument in the callback invocation. Fixed by collecting *every* candidate argument position (evalSummary.keyCandidates) and selecting the correct one at detection time by matching methodSpecs[resolved method].keyArgIndex, never by position in the source. Also fixed (caught by `go vet`, not just golangci-lint in CI): `(*types.Func).Signature()` requires go1.23; the project's own go.mod targets go1.22.0, so this would have broken the Go 1.22 CI job. Replaced with the older `.Type().(*types.Signature)` pattern. Verified: two new buildable fixtures — testdata/strict/forwarding_call (direct forwarding, a generic-instantiated variant matching issue #26's own repro exactly, a one-hop pass-through, a false-positive guard, and a genuinely-dynamic-key guard) and testdata/strict/flag_descriptor (the full two-hop accessor/factory/package-var chain, built to precisely mirror e2b-dev/infra's real shape including parameter order — the first draft of this fixture accidentally omitted the ctx-comes-first detail and passed despite bug #2 above still being present, which is why it was rebuilt to match the real source exactly rather than trusted on its own). Full test suite green. Re-verified directly against e2b-dev/infra's own featureflags package: 7 real usages detected, all correctly resolved (JSONVariationCtx calls through the same two-hop chain), zero warnings. Re-verified against weaviate: unchanged, same 4 real usages, no regression. Fixes #26 Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds a new static-analysis module (forwarding.go) that detects flag-evaluation calls forwarded through intermediate functions via a two-hop pattern, wires it into ScanStrict replacing strictTypesUsages, and adds two test fixtures with corresponding strict-scan tests. ChangesForwarding call detection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WrapperFunc
participant SDKClient
participant DetectForwardingCallUsages
participant EvalSummaries
Caller->>WrapperFunc: usePassThrough(literal key)
WrapperFunc->>SDKClient: MakeClient()
WrapperFunc->>SDKClient: callDirect(client.BoolVariation, key)
Note over EvalSummaries: findEvalSummaries pre-pass
EvalSummaries->>EvalSummaries: findDirectForwarding(callDirect)
EvalSummaries->>EvalSummaries: findPassThrough(passThroughWrapper)
DetectForwardingCallUsages->>EvalSummaries: match call site to evalSummary
DetectForwardingCallUsages->>DetectForwardingCallUsages: resolveFlagDescriptorKey(key arg)
DetectForwardingCallUsages-->>Caller: emit types.FlagUsage (flag key, SDK, DetectedBy=strict-types)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Independent review found: accessorKey (forwarding.go) was keyed by bare
type name + method name only, with no package qualifier. Two unrelated
types in different packages of the scanned repo, both named the same
(e.g. "BoolFlag") and both trivial single-field accessor methods named
the same (e.g. "Key"), would silently collide in the shared cross-package
accessorFields index — whichever package's entry a given map iteration
visited last would win, non-deterministically (Go's map iteration order
is randomized per process), and the "losing" type's field name would be
used for the other type's lookups.
Fixed by package-qualifying both sides: accessorFields (write side) now
takes the owning package's path and keys by "pkgPath.TypeName"; namedTypeName
(read side, used by resolveFlagDescriptorKey to resolve a concrete call-
site value's type) does the same via named.Obj().Pkg().Path().
Verified: added a fixture package (testdata/strict/flag_descriptor/
unrelated) declaring a type also named BoolFlag with an also-named Key()
method, reading a *different* field ("identifier", not "name") — proving
the real chain still reliably resolves the correct flag key regardless.
Ran the existing chain test 5 times in a row to rule out map-order-
dependent flakiness (there wasn't any, confirming the fix, since the keys
are now genuinely distinct rather than accidentally non-colliding by luck
of iteration order). Full test suite green.
Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements ADR 006's approach A (the "forwarding function" pattern) for issue #26 — a method value captured in one function, passed as an argument, invoked from inside a different callee — then extends it past the ADR's own original scope once real-world verification against the ADR's own motivating repo (e2b-dev/infra) showed the actual shape needed more than a single hop.
The real chain
A package-level var (e.g.
SnapshotFeatureFlag) built by a factory (NewBoolFlag) that stores a literal into a struct field, read back through a trivial accessor method (Key), forwarded through two levels of function wrapping (a "pass-through" wrapper method that fixes the SDK method internally, then a generic "direct forwarding" function that actually invokes the callback) before reaching the SDK call.Four new whole-scan passes, chained together in the same fixed-point spirit as factory.go's cross-file resolution (ADR 004) and issue #18's Pass B loop — still no go/ssa/go/callgraph (ADR 006's approach B):
accessorFields— trivial single-field-accessor methodsfactoryFieldParams— constructors storing a parameter into a composite-literal fieldflagDescriptorLiterals— package-level vars built via a known factory with a literal argumentfindEvalSummaries— direct-forwarding functions, then pass-through functions to a fixed point (a chain can deepen one hop per round)Two real bugs found and fixed during verification against e2b-dev/infra itself
flag.Key()whereflag typedFlag[T]resolves viago/typesto the interface's abstract method object — a different*types.Functhan the concrete type's own implementation, which is the only one with a body to learn a field from. Fixed by keying by (type name, method name) instead of object identity.ctx, conventionally first) comes before the real flag-descriptor argument. Fixed by collecting every candidate position and selecting the correct one at detection time by matchingmethodSpecs[resolved method].keyArgIndex.Also fixed (caught by
go vet):(*types.Func).Signature()requires go1.23, but the project's own go.mod targets go1.22.0 — would have broken the Go 1.22 CI job. Replaced with the older.Type().(*types.Signature)pattern.Verification
testdata/strict/forwarding_call(direct forwarding, generic-instantiated variant matching the issue's exact repro, one-hop pass-through, false-positive guard, genuinely-dynamic-key guard) andtestdata/strict/flag_descriptor(the full two-hop chain, built to precisely mirror e2b-dev/infra's real shape — including parameter order, since the first draft accidentally omitted thectx-comes-first detail and passed despite bug feat(core): add shared types, config loader, and fingerprint generator #2 still being present).featureflagspackage: 7 real usages detected, all correctly resolved, zero warnings.Summary by CodeRabbit
New Features
Bug Fixes