fix(scanner): declared-type-alone struct fields + package-level composite literals#34
Conversation
…site literals
Closes two real gaps found by direct testing against flaglint/corpus, a
new external adversarial fixture corpus for FlagLint (both languages):
1. struct-field-receiver: a struct field declared *ld.LDClient, never
assigned or composite-literal-bound anywhere in the scanned tree — the
dominant Go dependency-injection pattern (client wired up by an
external framework or a constructor not included in what's actually
scanned). Fixed by collectDeclaredClientFields (factory.go): the same
"trust the declared type, no build required" principle
paramClientBindings already applies to function parameters, extended
to struct fields. Sound for the identical reason — Go's own type
system guarantees such a field can only ever hold an SDK client.
2. composite-literal-binding: a composite literal directly initializing a
package-level var (`var svc = &Svc{Client: svcClient}`), never inside
any function body. runWholeScanAnalysis's Pass B only ever walked
function scopes (collectFuncScopes/walkScoped) — a literal at package
scope was never visited at all. Root-caused during issue #18's Pass B
fixed-point fix but never filed as its own issue at the time. Fixed by
packageLevelCompositeLiterals (identity.go), fed into the same Pass B
fixed-point loop using pkg itself as the known-bindings context (no
function-local scope exists at package level).
This alone wasn't enough for the fixture's actual call site
(`svc.Client.BoolVariation(...)` called from an ordinary function, not
a method on Svc): resolveChainType only ever consulted the enclosing
function's own declared parameter/receiver types for a bare
identifier, so a chain rooted at a *package-level* variable always
came up empty regardless of whether the binding itself existed. Closed
by a second new whole-scan index, collectPackageVarTypes
(structtypes.go) — "VarName" -> declared/inferred type name, from an
explicit type annotation or inferred from a composite-literal
initializer's own type — merged into the "declared" map passed to
detect()/collectFieldBindings (a local parameter of the same name
correctly still shadows it, via mergeBindings' existing collision
semantics).
Verified: reproduced both gaps directly against the real flaglint/corpus
fixtures before fixing (0 usages on both), confirmed both resolve
correctly after (dark-mode / new-ui, matching the corpus's own
expected.json), then re-ran the full corpus (5/5 Go fixtures pass,
including the false-positive/decoy guard). Added project-convention
regression fixtures (positive_struct_field_declared_type_only.go,
positive_package_level_composite_literal.go) each with a false-positive
guard proving the mechanism keys off the resolved type/binding, never a
field or variable name. Full test suite green. Re-verified against
weaviate and mint-app: same usage counts as before, no regressions, no
new false positives.
Signed-off-by: Krishan Kant Sharma <krishansharma0327@gmail.com>
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 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 |
2 participants
Summary
Closes #32 and #33 — two real gaps found by direct testing against flaglint/corpus, a new external adversarial test corpus shared with flaglint-js.
scanner: struct field declared *ld.LDClient, no observed construction, is not detected #32 — struct field declared
*ld.LDClient, no observed construction anywhere in the scanned tree (the dominant Go DI pattern). Fixed bycollectDeclaredClientFields(factory.go): the same "trust the declared type, no build required" principleparamClientBindingsalready applies to function parameters, extended to struct fields. Sound for the identical reason — Go's own type system guarantees such a field can only ever hold an SDK client.scanner: package-level composite literal (var x = &Type{...}) is not detected #33 — a composite literal directly initializing a package-level
var, never inside any function body.runWholeScanAnalysis's Pass B only ever walked function scopes — a literal at package scope was never visited. Root-caused during issue scanner: composite-literal binding is a single forward pass, not fixed-point — order-dependent false negative #18's Pass B fixed-point fix but never filed at the time. Fixed bypackageLevelCompositeLiterals(identity.go), fed into the same Pass B fixed-point loop.This alone wasn't sufficient for the fixture's actual call site (
svc.Client.BoolVariation(...)called from an ordinary function, not a method onSvc):resolveChainTypeonly ever consulted the enclosing function's own declared parameter/receiver types for a bare identifier, so a chain rooted at a package-level variable always came up empty regardless of whether the binding existed. Closed by a second new whole-scan index,collectPackageVarTypes(structtypes.go) — merged into the "declared" map passed todetect()/collectFieldBindings(a local parameter of the same name still correctly shadows it, viamergeBindings' existing collision semantics).Verification
flaglint/corpusfixtures before fixing (0 usages on both), confirmed both resolve correctly after (dark-mode/new-ui, matching the corpus's ownexpected.json).