From d72f9c0957eac2c80b0dea1713ef6ce900405716 Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Wed, 8 Jul 2026 08:00:00 -0500 Subject: [PATCH] fix(scanner): declared-type-alone struct fields + package-level composite literals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/scanner/factory.go | 48 +++++++++++++ internal/scanner/identity.go | 55 +++++++++++++- internal/scanner/scanner.go | 49 ++++++++++++- internal/scanner/scanner_test.go | 31 ++++++++ internal/scanner/structtypes.go | 72 ++++++++++++++++++- ...ositive_package_level_composite_literal.go | 40 +++++++++++ ...ositive_struct_field_declared_type_only.go | 35 +++++++++ 7 files changed, 325 insertions(+), 5 deletions(-) create mode 100644 internal/scanner/testdata/fixtures/positive_package_level_composite_literal.go create mode 100644 internal/scanner/testdata/fixtures/positive_struct_field_declared_type_only.go diff --git a/internal/scanner/factory.go b/internal/scanner/factory.go index 278b42a..14d6bf0 100644 --- a/internal/scanner/factory.go +++ b/internal/scanner/factory.go @@ -116,6 +116,54 @@ func paramClientBindings(fl *ast.FieldList, imports sdkImports) map[string]strin return bindings } +// collectDeclaredClientFields returns "StructName.FieldName" -> SDK +// version for every struct field in file whose *declared type* is +// directly `*.LDClient` — the same "trust the signature, no build +// required" principle paramClientBindings already applies to function +// parameters, applied here to struct fields instead. Sound for the +// identical reason: Go's own type system guarantees such a field can +// only ever hold an SDK client (or nil), so no assignment or composite +// literal needs to be observed anywhere in the scanned tree for this to +// be safe. +// +// Found missing during corpus testing (flaglint/corpus: +// struct-field-receiver): the dominant Go dependency-injection pattern is +// a client field wired up by an external framework, or a constructor not +// included in what's actually scanned — there is no assignment to trace +// at all, exactly the gap paramClientBindings already closed for +// parameters, just never extended to fields. +func collectDeclaredClientFields(file *ast.File, imports sdkImports) map[string]string { + bindings := map[string]string{} + for _, decl := range file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok { + continue + } + for _, spec := range gen.Specs { + ts, ok := spec.(*ast.TypeSpec) + if !ok { + continue + } + st, ok := ts.Type.(*ast.StructType) + if !ok || st.Fields == nil { + continue + } + for _, f := range st.Fields.List { + version, ok := starLDClientType(f.Type, imports) + if !ok { + continue + } + for _, name := range f.Names { + if name.Name != "_" { + bindings[ts.Name.Name+"."+name.Name] = version + } + } + } + } + } + return bindings +} + // collectFactoryFunctions registers every free function (no receiver — // methods are out of scope here) in file whose declared return type // resolves to the SDK client type, keyed by pkgKey so cross-package call diff --git a/internal/scanner/identity.go b/internal/scanner/identity.go index cad8499..319e76c 100644 --- a/internal/scanner/identity.go +++ b/internal/scanner/identity.go @@ -123,6 +123,14 @@ type fileContext struct { // multi-level field-selector chains one hop at a time. structFieldTypes map[string]string + // packageVarTypes is the whole-scan index of "VarName" -> declared/ + // inferred type name for package-level vars (see collectPackageVarTypes, + // structtypes.go) — lets a chain rooted at a package-level variable + // (`svc.Client.BoolVariation(...)`, svc a package var, not a function + // parameter/receiver) resolve at all; resolveChainType otherwise only + // ever knows about the enclosing function's own declared parameter types. + packageVarTypes map[string]string + // typesInfo is real go/types information for this file's package, only // populated by the opt-in --strict-types pass (ScanStrict, strict.go) — // nil for an ordinary Scan. See resolveAssignedBinding's fallback and @@ -185,7 +193,11 @@ func collectFieldBindings(file *ast.File, ctx fileContext) map[string]string { if !ok || fn.Body == nil { continue } - declared := declaredParamTypes(fn) + // packageVarTypes merged in first (mergeBindings's later argument + // wins on collision) so a local parameter of the same name + // correctly shadows a package-level variable's type, matching real + // Go scoping — see fileContext.packageVarTypes. + declared := mergeBindings(ctx.packageVarTypes, declaredParamTypes(fn)) ast.Inspect(fn.Body, func(n ast.Node) bool { assign, ok := n.(*ast.AssignStmt) @@ -619,6 +631,47 @@ func compositeLiteralFieldBindings(lit *ast.CompositeLit, knownBindings map[stri return bindings } +// packageLevelCompositeLiterals returns every *ast.CompositeLit appearing +// in a package-level `var` declaration's initializer expression(s) in +// file — including one wrapped in `&Type{...}` (Go's address-of syntax +// for a composite literal): ast.Inspect walks into the wrapper +// regardless, so the inner literal is found either way. +// +// Found missing during corpus testing (flaglint/corpus: +// composite-literal-binding): compositeLiteralFieldBindings itself is +// entirely agnostic to where a literal appears syntactically — it only +// needs the literal node and whatever knownBindings map is in scope at +// that point — but runWholeScanAnalysis's Pass B only ever walked +// *function* scopes (collectFuncScopes/walkScoped), so a literal directly +// initializing a package-level var (`var svc = &Svc{Client: svcClient}`, +// never inside any function body at all) was never visited. Root-caused +// during issue #18's Pass B fixed-point fix but never filed as its own +// issue at the time. +func packageLevelCompositeLiterals(file *ast.File) []*ast.CompositeLit { + var lits []*ast.CompositeLit + for _, decl := range file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok || gen.Tok != token.VAR { + continue + } + for _, spec := range gen.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for _, val := range vs.Values { + ast.Inspect(val, func(n ast.Node) bool { + if lit, ok := n.(*ast.CompositeLit); ok { + lits = append(lits, lit) + } + return true + }) + } + } + } + return lits +} + // mergeBindings returns a new map containing every entry from both inputs; // b's entries win on key collision (there should never be one in practice, // since package-level and struct-field bindings key on disjoint identifier diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index fa42dfe..9cd34a3 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -167,19 +167,28 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag // granularity. Bare identifiers are never visible outside their own // package in real Go anyway, so this partitioning also just matches // actual Go semantics. + // packageVarTypesByPkg (per-package, same reason as structFieldTypesByPkg + // above) is the "VarName" -> declared/inferred type index that lets a + // chain rooted at a package-level variable resolve at all — see + // collectPackageVarTypes, structtypes.go. structFieldTypesByPkg := map[string]map[string]string{} + packageVarTypesByPkg := map[string]map[string]string{} factoryFunctions := map[factoryKey]string{} for _, pf := range parsed { pkg := pkgBindings(structFieldTypesByPkg, ownPkgKey[pf.file]) for k, v := range collectStructFieldTypes(pf.file) { pkg[k] = v } + varTypes := pkgBindings(packageVarTypesByPkg, ownPkgKey[pf.file]) + for k, v := range collectPackageVarTypes(pf.file) { + varTypes[k] = v + } collectFactoryFunctions(pf.file, ownPkgKey[pf.file], pf.imports, factoryFunctions) } // Pre-pass 3: per-file contexts, now that every whole-scan index above - // is complete. Each file's structFieldTypes is scoped to its own - // package's partition only — see the comment above. + // is complete. Each file's structFieldTypes/packageVarTypes is scoped + // to its own package's partition only — see the comment above. ctxs := make(map[*ast.File]fileContext, len(parsed)) for _, pf := range parsed { ctxs[pf.file] = fileContext{ @@ -188,6 +197,7 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag importAliases: resolveImportAliases(pf.file, importPathToPkgKey, importPathToPkgName), factoryFunctions: factoryFunctions, structFieldTypes: pkgBindings(structFieldTypesByPkg, ownPkgKey[pf.file]), + packageVarTypes: pkgBindings(packageVarTypesByPkg, ownPkgKey[pf.file]), typesInfo: pf.typesInfo, } } @@ -205,6 +215,7 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag // unqualified identifier is only ever visible within its own package. base := map[string]map[string]string{} fileScopes := make(map[*ast.File][]funcScope, len(parsed)) + packageLits := make(map[*ast.File][]*ast.CompositeLit, len(parsed)) for _, pf := range parsed { ctx := ctxs[pf.file] pkg := pkgBindings(base, ctx.ownPkgKey) @@ -214,7 +225,20 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag for k, v := range collectFieldBindings(pf.file, ctx) { pkg[k] = v } + // Declared-type-alone field bindings (no assignment/composite + // literal needs to be observed anywhere — see + // collectDeclaredClientFields, factory.go) never lose to an + // observed binding above, but also never override one: both + // sources always agree on the version for valid Go code (a + // field's declared type is fixed), so only fill in what isn't + // already there. + for k, v := range collectDeclaredClientFields(pf.file, ctx.imports) { + if _, exists := pkg[k]; !exists { + pkg[k] = v + } + } fileScopes[pf.file] = collectFuncScopes(pf.file, ctx.imports) + packageLits[pf.file] = packageLevelCompositeLiterals(pf.file) } // Pass B: composite-literal field bindings (`&LDIntegration{ldClient: @@ -238,11 +262,25 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag // legitimately be deeper than the number of files in the scan, so // hitting the cap without converging would only be possible from a // bug, not real code. + // + // Package-level composite literals (`var svc = &Svc{Client: + // svcClient}`) are processed in the same loop, using pkg itself as the + // known-bindings context — there's no function-local scope at package + // level, so a literal's field values can only ever reference other + // package-level bindings, which is exactly what pkg already holds. for sweep := 0; sweep <= len(parsed); sweep++ { changed := false for _, pf := range parsed { ctx := ctxs[pf.file] pkg := pkgBindings(base, ctx.ownPkgKey) + for _, lit := range packageLits[pf.file] { + for k, v := range compositeLiteralFieldBindings(lit, pkg, ctx) { + if existing, ok := pkg[k]; !ok || existing != v { + pkg[k] = v + changed = true + } + } + } for _, s := range fileScopes[pf.file] { walkScoped(s.body, mergeBindings(pkg, s.paramBindings), ctx, func(n ast.Node, current map[string]string, _ map[string]methodValueBinding) { lit, ok := n.(*ast.CompositeLit) @@ -272,7 +310,12 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag pkg := pkgBindings(base, ctx.ownPkgKey) d := &fileDetector{fset: fset, relPath: pf.relPath} for _, s := range fileScopes[pf.file] { - d.detect(s.body, mergeBindings(pkg, s.paramBindings), s.declared, ctx.structFieldTypes, ctx) + // packageVarTypes merged in first (mergeBindings's later + // argument wins on collision) so a local parameter of the + // same name correctly shadows a package-level variable's type + // — see fileContext.packageVarTypes. + declared := mergeBindings(ctx.packageVarTypes, s.declared) + d.detect(s.body, mergeBindings(pkg, s.paramBindings), declared, ctx.structFieldTypes, ctx) } allUsages = append(allUsages, d.usages...) } diff --git a/internal/scanner/scanner_test.go b/internal/scanner/scanner_test.go index 22031cd..c79bcfc 100644 --- a/internal/scanner/scanner_test.go +++ b/internal/scanner/scanner_test.go @@ -115,6 +115,37 @@ func TestScanFile_positiveCompositeLiteral(t *testing.T) { } } +func TestScanFile_positiveStructFieldDeclaredTypeOnly(t *testing.T) { + // A struct field declared *ld.LDClient, never assigned or composite- + // literal-bound anywhere in the scanned tree (flaglint/corpus: + // struct-field-receiver) — the field's declared type alone must be + // sufficient proof, same soundness as paramClientBindings already + // relies on for function parameters. + usages := parseFixture(t, "positive_struct_field_declared_type_only.go") + if len(usages) != 1 { + t.Fatalf("got %d usages, want exactly 1 (the unrelated string field must not be detected): %+v", len(usages), usages) + } + if got := usages[0]; got.FlagKey != "declared-type-only-flag" { + t.Errorf("usages[0] = %+v, want declared-type-only-flag", got) + } +} + +func TestScanFile_positivePackageLevelCompositeLiteral(t *testing.T) { + // A package-level composite literal (`var svcWrapper = &SvcWrapper{ + // Client: svcClient}`), never inside any function body — Pass B + // previously only walked function scopes (flaglint/corpus: + // composite-literal-binding). Also proves a chain rooted at a bare + // package-level identifier (not a function parameter/receiver) + // resolves at the call site. + usages := parseFixture(t, "positive_package_level_composite_literal.go") + if len(usages) != 1 { + t.Fatalf("got %d usages, want exactly 1 (the unrelated wrapper's string field must not be detected): %+v", len(usages), usages) + } + if got := usages[0]; got.FlagKey != "package-level-composite-literal-flag" { + t.Errorf("usages[0] = %+v, want package-level-composite-literal-flag", got) + } +} + func TestScanFile_falsePositiveCompositeLiteralUnbound(t *testing.T) { usages := parseFixture(t, "false_positive_composite_literal_unbound.go") if len(usages) != 0 { diff --git a/internal/scanner/structtypes.go b/internal/scanner/structtypes.go index 7f7e729..fe012fe 100644 --- a/internal/scanner/structtypes.go +++ b/internal/scanner/structtypes.go @@ -1,6 +1,9 @@ package scanner -import "go/ast" +import ( + "go/ast" + "go/token" +) // collectStructFieldTypes walks top-level `type X struct { ... }` // declarations in file and returns "StructName.FieldName" -> the field's @@ -47,3 +50,70 @@ func collectStructFieldTypes(file *ast.File) map[string]string { } return types } + +// collectPackageVarTypes returns "VarName" -> declared/inferred type name +// for every package-level `var` declaration in file — either from an +// explicit type annotation (`var svc Svc`) or inferred from a composite- +// literal initializer's own type (`var svc = &Svc{...}` or `var svc = +// Svc{...}` — ast.Inspect finds the literal either way, regardless of +// address-of wrapping). +// +// Found missing during corpus testing (flaglint/corpus: +// composite-literal-binding): resolveChainType (identity.go) only ever +// consulted the enclosing function's own declared parameter/receiver +// types for a bare identifier — a chain rooted at a *package-level* +// variable (`svc.Client.BoolVariation(...)` called from an ordinary +// function, not a method on Svc) always came up empty, since nothing +// recorded what type "svc" itself was. +func collectPackageVarTypes(file *ast.File) map[string]string { + types := map[string]string{} + for _, decl := range file.Decls { + gen, ok := decl.(*ast.GenDecl) + if !ok || gen.Tok != token.VAR { + continue + } + for _, spec := range gen.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + if vs.Type != nil { + typeName := simpleTypeName(vs.Type) + if typeName == "" { + continue + } + for _, name := range vs.Names { + if name.Name != "_" { + types[name.Name] = typeName + } + } + continue + } + for i, name := range vs.Names { + if name.Name == "_" || i >= len(vs.Values) { + continue + } + var lit *ast.CompositeLit + ast.Inspect(vs.Values[i], func(n ast.Node) bool { + if lit != nil { + return false + } + if l, ok := n.(*ast.CompositeLit); ok { + lit = l + return false + } + return true + }) + if lit == nil { + continue + } + typeName := simpleTypeName(lit.Type) + if typeName == "" { + continue + } + types[name.Name] = typeName + } + } + } + return types +} diff --git a/internal/scanner/testdata/fixtures/positive_package_level_composite_literal.go b/internal/scanner/testdata/fixtures/positive_package_level_composite_literal.go new file mode 100644 index 0000000..2b3ea3c --- /dev/null +++ b/internal/scanner/testdata/fixtures/positive_package_level_composite_literal.go @@ -0,0 +1,40 @@ +package fixtures + +import ( + "time" + + ld "github.com/launchdarkly/go-server-sdk/v7" +) + +// Mirrors flaglint/corpus's composite-literal-binding fixture: the client +// is bound via a package-level composite literal — never inside any +// function body at all — which runWholeScanAnalysis's Pass B previously +// never visited (it only walked function scopes). svcClient itself is +// bound via Pass A's ordinary package-level constructor-call handling; +// what's new is svcWrapper's own composite literal, and resolving +// svcWrapper.Client's chain from a bare package-level identifier (not a +// function parameter/receiver) at the call site in useSvcWrapper. +type SvcWrapper struct { + Client *ld.LDClient +} + +var svcClient, _ = ld.MakeClient("sdk-key", 5*time.Second) +var svcWrapper = &SvcWrapper{Client: svcClient} + +func useSvcWrapper() { + _, _ = svcWrapper.Client.BoolVariation("package-level-composite-literal-flag", nil, false) +} + +// unrelatedWrapper is a package-level composite literal too, but its +// field is never bound from a proven client — proving this mechanism +// requires the field's *value* to resolve, not just any package-level +// composite literal existing. +type unrelatedWrapper struct { + Label string +} + +var unrelatedGlobal = &unrelatedWrapper{Label: "not-a-flag-client"} + +func useUnrelatedWrapper() string { + return unrelatedGlobal.Label +} diff --git a/internal/scanner/testdata/fixtures/positive_struct_field_declared_type_only.go b/internal/scanner/testdata/fixtures/positive_struct_field_declared_type_only.go new file mode 100644 index 0000000..2dfdeec --- /dev/null +++ b/internal/scanner/testdata/fixtures/positive_struct_field_declared_type_only.go @@ -0,0 +1,35 @@ +package fixtures + +import ( + ld "github.com/launchdarkly/go-server-sdk/v7" +) + +// FlagService's client field is never assigned or composite-literal-bound +// anywhere in this scanned tree — the dominant Go dependency-injection +// pattern (found via flaglint/corpus's struct-field-receiver fixture): +// the client is wired up by an external framework or a constructor not +// included in what's actually scanned. The field's *declared type* alone +// (`*ld.LDClient`) is the only proof available, and it's sufficient: Go's +// own type system guarantees the field can only ever hold an SDK client, +// the same soundness paramClientBindings already relies on for function +// parameters (factory.go) — just never extended to struct fields before. +type FlagService struct { + ld *ld.LDClient +} + +func (s *FlagService) DarkMode(userID string) bool { + v, _ := s.ld.BoolVariation("declared-type-only-flag", nil, false) + return v +} + +// unrelatedFieldOwner's field is a plain string, not an SDK client — the +// field's own name ("ld") coincidentally overlaps with a common alias, +// but its declared type never resolves via starLDClientType, proving +// this mechanism keys strictly off the resolved type, never the name. +type unrelatedFieldOwner struct { + ld string +} + +func (u *unrelatedFieldOwner) Report() string { + return u.ld +}