Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions internal/scanner/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 `*<alias>.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
Expand Down
55 changes: 54 additions & 1 deletion internal/scanner/identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
49 changes: 46 additions & 3 deletions internal/scanner/scanner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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,
}
}
Expand All @@ -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)
Expand All @@ -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:
Expand All @@ -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)
Expand Down Expand Up @@ -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...)
}
Expand Down
31 changes: 31 additions & 0 deletions internal/scanner/scanner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
72 changes: 71 additions & 1 deletion internal/scanner/structtypes.go
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
}
Original file line number Diff line number Diff line change
@@ -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
}
Loading
Loading