From 9ddb4da3569223bbe85ce65c9914bffcc0ae2ce8 Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Tue, 7 Jul 2026 13:57:53 -0500 Subject: [PATCH 1/3] =?UTF-8?q?feat(scanner):=20--strict-types=20Phase=202?= =?UTF-8?q?a=20=E2=80=94=20interface=20satisfaction,=20closing=20#15?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements ADR 005's Phase 2a: an opt-in --strict-types pass that loads the target module with real go/types information and uses it to prove client identity through an interface — Phase 1's syntactic tracing sees only the interface type name, never the concrete *ld.LDClient underneath (`var evaluator FlagEvaluator = client`). internal/typecheck is a new, small, LaunchDarkly-agnostic package wrapping golang.org/x/tools/go/packages: Load(dir) loads every package under dir with full type info, excluding (never failing on) any package that doesn't type-check cleanly — including transitively, via IllTyped, not just its own direct errors — since type info built on an ill-typed dependency can't be trusted as proof of anything. Each exclusion is returned as a LoadFailure with a human-readable reason. The actual identity-resolution logic stays in internal/scanner, which already owns every other piece of "what counts as an SDK usage" domain knowledge, rather than duplicating it in a new package: - resolveAssignedBinding (identity.go) gained a static-type fallback, resolveByStaticType — when the existing syntax-only checks (constructor call, factory call) don't match, and real go/types information is available, it checks the expression's actual static type against the SDK's *LDClient directly. Reusing this ONE function means every existing Phase 1 mechanism (walkScoped's block-scoped binding tracking, method values, the works) gains type-backed coverage uniformly, not just a narrow bespoke walk for interface satisfaction specifically. - ScanStrict (strict.go) runs Scan (Phase 1, entirely unchanged) first, then re-runs the full whole-scan analysis over go/packages-loaded ASTs (which carry the type info Scan's own independently go/parser-parsed ASTs never do) and merges in only the fingerprints Phase 1 didn't already find — enforced by an explicit dedup, not assumed from the re-scan happening to be a superset. Every merged-in finding is tagged DetectedBy: "strict-types" (types.FlagUsage's new additive field). - --strict-types is wired as a flag (not a config-file field — Go-only, no flaglint-js equivalent) on scan/audit/validate, via one new runScan cli helper so all three commands share identical --strict-types behavior. A real, non-obvious bug found and fixed during real-repo verification: running --strict-types against a real 4500+-file codebase (weaviate) produced 321 typecheck-failure warnings — nearly every package. Root cause: flaglint-go's own go.mod declares an older go version than weaviate's, and it's the *compiled binary's* embedded GODEBUG default (not the scanned target's own go version) that governs how go/types represents Go 1.24+ generic type aliases, since the actual type-checking runs in-process. Fixed by having typecheck.Load force GODEBUG's gotypesalias=1 via os.Setenv before loading (packages.Config.Env alone doesn't reach it, since that only affects the subprocesses Load spawns for package discovery, not this process's own in-process type-checking) — unless the caller's environment already specifies one explicitly, which is left alone. Verified directly: 321 warnings before the fix, 0 after, against the same real repo. Verified: a real, buildable fixture module (testdata/strict/ interface_satisfaction, with a local-replace stub SDK so tests need no network access or the real launchdarkly SDK dependency) proves the interface-satisfaction repro is detected by ScanStrict and invisible to plain Scan, plus a false-positive guard (an unrelated type satisfying the same interface shape must not be detected). Full test suite green. A directory with no go.mod at all falls back to Phase 1's result plus a warning, never a hard failure. Re-verified against weaviate: same 4 real usages Phase 1 already found, no regressions, zero warnings with the GODEBUG fix, and no interface-satisfaction pattern present there to find (that fixture's specific pattern isn't in the codebase, which is why the buildable-fixture proof, not just a real-repo re-scan, is what actually demonstrates this closes #15). Fixes #15 Signed-off-by: Krishan Kant Sharma --- README.md | 5 + go.mod | 5 +- go.sum | 8 + internal/cli/audit.go | 7 +- internal/cli/scan.go | 7 +- internal/cli/shared.go | 26 ++++ internal/cli/validate.go | 7 +- internal/scanner/identity.go | 71 +++++++-- internal/scanner/scanner.go | 8 + internal/scanner/strict.go | 146 ++++++++++++++++++ internal/scanner/strict_test.go | 64 ++++++++ .../strict/interface_satisfaction/go.mod | 7 + .../strict/interface_satisfaction/main.go | 41 +++++ .../interface_satisfaction/stubsdk/client.go | 18 +++ .../interface_satisfaction/stubsdk/go.mod | 3 + internal/typecheck/load.go | 118 ++++++++++++++ internal/typecheck/load_test.go | 26 ++++ internal/types/types.go | 11 +- 18 files changed, 556 insertions(+), 22 deletions(-) create mode 100644 internal/scanner/strict.go create mode 100644 internal/scanner/strict_test.go create mode 100644 internal/scanner/testdata/strict/interface_satisfaction/go.mod create mode 100644 internal/scanner/testdata/strict/interface_satisfaction/main.go create mode 100644 internal/scanner/testdata/strict/interface_satisfaction/stubsdk/client.go create mode 100644 internal/scanner/testdata/strict/interface_satisfaction/stubsdk/go.mod create mode 100644 internal/typecheck/load.go create mode 100644 internal/typecheck/load_test.go diff --git a/README.md b/README.md index 99db8e5..2241e12 100644 --- a/README.md +++ b/README.md @@ -65,6 +65,11 @@ brew install flaglint/tap/flaglint-go flaglint-go scan ./services flaglint-go scan ./services --format json +# --strict-types: additionally resolves findings only provable with real +# go/types information (interface satisfaction) — requires the module to +# build; see docs/adr/005-strict-types-pass.md +flaglint-go scan ./services --strict-types + # Inventory + migration-readiness score flaglint-go audit ./services flaglint-go audit ./services --write-baseline .flaglint-baseline.json diff --git a/go.mod b/go.mod index bd17843..64ca757 100644 --- a/go.mod +++ b/go.mod @@ -1,13 +1,16 @@ module github.com/flaglint/flaglint-go -go 1.22 +go 1.22.0 require ( github.com/bmatcuk/doublestar/v4 v4.10.0 github.com/spf13/cobra v1.10.2 + golang.org/x/tools v0.30.0 ) require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/spf13/pflag v1.0.9 // indirect + golang.org/x/mod v0.23.0 // indirect + golang.org/x/sync v0.11.0 // indirect ) diff --git a/go.sum b/go.sum index ff8559e..72085c8 100644 --- a/go.sum +++ b/go.sum @@ -1,6 +1,8 @@ github.com/bmatcuk/doublestar/v4 v4.10.0 h1:zU9WiOla1YA122oLM6i4EXvGW62DvKZVxIe6TYWexEs= github.com/bmatcuk/doublestar/v4 v4.10.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= @@ -9,4 +11,10 @@ github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiT github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +golang.org/x/mod v0.23.0 h1:Zb7khfcRGKk+kqfxFaP5tZqCnDZMjC5VtUBs87Hr6QM= +golang.org/x/mod v0.23.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= +golang.org/x/sync v0.11.0 h1:GGz8+XQP4FvTTrjZPzNKTMFtSXH80RAzG+5ghFPgK9w= +golang.org/x/sync v0.11.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk= +golang.org/x/tools v0.30.0 h1:BgcpHewrV5AUp2G9MebG4XPFI1E2W41zU1SaqVA9vJY= +golang.org/x/tools v0.30.0/go.mod h1:c347cR/OJfw5TI+GfX7RUPNMdDRRbjvYTS0jPyvsVtY= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/cli/audit.go b/internal/cli/audit.go index 19e42f7..2897597 100644 --- a/internal/cli/audit.go +++ b/internal/cli/audit.go @@ -6,11 +6,11 @@ import ( "github.com/flaglint/flaglint-go/internal/baseline" "github.com/flaglint/flaglint-go/internal/readiness" "github.com/flaglint/flaglint-go/internal/reporter" - "github.com/flaglint/flaglint-go/internal/scanner" ) func newAuditCommand(version string) *cobra.Command { var format, output, configPath, writeBaselinePath string + var strictTypes bool cmd := &cobra.Command{ Use: "audit [dir]", @@ -29,7 +29,7 @@ func newAuditCommand(version string) *cobra.Command { return exitErr } - result, err := scanner.Scan(dir, cfg) + result, err := runScan(dir, cfg, strictTypes) if err != nil { return internalError("scan failed: %v", err) } @@ -50,7 +50,7 @@ func newAuditCommand(version string) *cobra.Command { r.LowRiskCalls, r.MediumRiskCalls, r.HighRiskCalls) } for _, w := range result.Warnings { - stderrInfo(cmd, "warning: %s: %s\n", w.Kind, w.File) + printWarning(cmd, w) } if writeBaselinePath != "" { @@ -85,6 +85,7 @@ func newAuditCommand(version string) *cobra.Command { cmd.Flags().StringVarP(&output, "output", "o", "", "write report to file instead of stdout") cmd.Flags().StringVar(&configPath, "config", "", "path to config file") cmd.Flags().StringVar(&writeBaselinePath, "write-baseline", "", "write current finding fingerprints to a baseline file") + cmd.Flags().BoolVar(&strictTypes, "strict-types", false, "additionally resolve findings only provable with real go/types information (requires the module to build; see docs/adr/005-strict-types-pass.md)") return cmd } diff --git a/internal/cli/scan.go b/internal/cli/scan.go index 7962e48..c776d1a 100644 --- a/internal/cli/scan.go +++ b/internal/cli/scan.go @@ -4,11 +4,11 @@ import ( "github.com/spf13/cobra" "github.com/flaglint/flaglint-go/internal/reporter" - "github.com/flaglint/flaglint-go/internal/scanner" ) func newScanCommand() *cobra.Command { var format, output, configPath string + var strictTypes bool cmd := &cobra.Command{ Use: "scan [dir]", @@ -27,7 +27,7 @@ func newScanCommand() *cobra.Command { return exitErr } - result, err := scanner.Scan(dir, cfg) + result, err := runScan(dir, cfg, strictTypes) if err != nil { return internalError("scan failed: %v", err) } @@ -40,7 +40,7 @@ func newScanCommand() *cobra.Command { stderrInfo(cmd, "Scan complete — %d unique flag(s) across %d call site(s) (%s, %d file(s))\n", len(result.UniqueFlags), result.TotalUsages, formatDuration(result.ScanDurationMs), result.ScannedFiles) for _, w := range result.Warnings { - stderrInfo(cmd, "warning: %s: %s\n", w.Kind, w.File) + printWarning(cmd, w) } // scan is an inventory command — enforcement exit codes belong @@ -53,5 +53,6 @@ func newScanCommand() *cobra.Command { cmd.Flags().StringVarP(&format, "format", "f", "markdown", "output format: json | markdown") cmd.Flags().StringVarP(&output, "output", "o", "", "write report to file instead of stdout") cmd.Flags().StringVar(&configPath, "config", "", "path to config file") + cmd.Flags().BoolVar(&strictTypes, "strict-types", false, "additionally resolve findings only provable with real go/types information (requires the module to build; see docs/adr/005-strict-types-pass.md)") return cmd } diff --git a/internal/cli/shared.go b/internal/cli/shared.go index b971e39..4177a30 100644 --- a/internal/cli/shared.go +++ b/internal/cli/shared.go @@ -7,6 +7,8 @@ import ( "github.com/spf13/cobra" "github.com/flaglint/flaglint-go/internal/config" + "github.com/flaglint/flaglint-go/internal/scanner" + "github.com/flaglint/flaglint-go/internal/types" ) // validateDirectory checks that dir exists and is a directory, returning an @@ -46,6 +48,17 @@ func loadConfig(configPath string) (config.Config, *ExitError) { return cfg, nil } +// runScan calls scanner.Scan, or scanner.ScanStrict when strictTypes is +// set — the single place all three commands (scan/audit/validate) decide +// which to run, so the --strict-types flag behaves identically everywhere +// it's exposed. See docs/adr/005-strict-types-pass.md. +func runScan(dir string, cfg config.Config, strictTypes bool) (types.ScanResult, error) { + if strictTypes { + return scanner.ScanStrict(dir, cfg) + } + return scanner.Scan(dir, cfg) +} + // writeReport writes report to outputPath if non-empty, otherwise to the // command's stdout. A file-write failure is an internal error (exit 3) — // NOT matching flaglint-js's current shipped behavior, which calls @@ -72,6 +85,19 @@ func writeReport(cmd *cobra.Command, report, outputPath string) error { return nil } +// printWarning writes one scan warning to cmd's stderr, appending w's +// Reason (only ever set for a "typecheck-failure" warning — see +// types.ScanWarning) when present — without it, a --strict-types package +// load failure would print only a bare package path with no clue why it +// failed. +func printWarning(cmd *cobra.Command, w types.ScanWarning) { + if w.Reason != "" { + stderrInfo(cmd, "warning: %s: %s: %s\n", w.Kind, w.File, w.Reason) + return + } + stderrInfo(cmd, "warning: %s: %s\n", w.Kind, w.File) +} + // stderrInfo writes a formatted progress/summary line to cmd's stderr, // mirroring flaglint-js's own stderrInfo helper (src/commands/shared.ts). // The write error is deliberately discarded: a progress line failing to diff --git a/internal/cli/validate.go b/internal/cli/validate.go index 6e649c4..097b829 100644 --- a/internal/cli/validate.go +++ b/internal/cli/validate.go @@ -7,7 +7,6 @@ import ( "github.com/spf13/cobra" "github.com/flaglint/flaglint-go/internal/baseline" - "github.com/flaglint/flaglint-go/internal/scanner" "github.com/flaglint/flaglint-go/internal/types" "github.com/flaglint/flaglint-go/internal/validator" ) @@ -21,6 +20,7 @@ func newValidateCommand() *cobra.Command { configPath string baselinePath string failOnNew bool + strictTypes bool ) cmd := &cobra.Command{ @@ -44,12 +44,12 @@ func newValidateCommand() *cobra.Command { return exitErr } - result, err := scanner.Scan(dir, cfg) + result, err := runScan(dir, cfg, strictTypes) if err != nil { return internalError("scan failed: %v", err) } for _, w := range result.Warnings { - stderrInfo(cmd, "warning: %s: %s\n", w.Kind, w.File) + printWarning(cmd, w) } vOpts := validator.Options{NoDirectLaunchDarkly: noDirectLD, BootstrapExclude: bootstrapExclude} @@ -98,6 +98,7 @@ func newValidateCommand() *cobra.Command { cmd.Flags().StringVar(&configPath, "config", "", "path to config file") cmd.Flags().StringVar(&baselinePath, "baseline", "", "baseline file for comparing against known debt") cmd.Flags().BoolVar(&failOnNew, "fail-on-new", false, "exit 1 if any findings are not in the baseline") + cmd.Flags().BoolVar(&strictTypes, "strict-types", false, "additionally resolve findings only provable with real go/types information (requires the module to build; see docs/adr/005-strict-types-pass.md)") return cmd } diff --git a/internal/scanner/identity.go b/internal/scanner/identity.go index dd2dec0..cad8499 100644 --- a/internal/scanner/identity.go +++ b/internal/scanner/identity.go @@ -3,6 +3,7 @@ package scanner import ( "go/ast" "go/token" + "go/types" "strconv" ) @@ -121,6 +122,12 @@ type fileContext struct { // declared field type name (see structtypes.go), used to walk // multi-level field-selector chains one hop at a time. structFieldTypes 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 + // docs/adr/005-strict-types-pass.md. + typesInfo *types.Info } // collectPackageLevelBindings finds client bindings established by @@ -336,20 +343,64 @@ func opensScope(n ast.Node) bool { // resolveAssignedBinding reports what a single assignment-target position // should resolve to: the SDK version, if rhs is a recognized constructor -// or factory call, or ok=false for anything else — including a value we -// simply don't recognize. Callers that track scope (see walkScoped) must -// treat ok=false as "actively clear any inherited binding for this name", -// not just "don't add one" — see walkScoped's doc comment for why. +// or factory call, if ctx carries real type information and rhs's static +// type proves it directly (see resolveByStaticType), or ok=false for +// anything else — including a value we simply don't recognize. Callers +// that track scope (see walkScoped) must treat ok=false as "actively clear +// any inherited binding for this name", not just "don't add one" — see +// walkScoped's doc comment for why. func resolveAssignedBinding(rhs ast.Expr, ctx fileContext) (version string, ok bool) { - call, ok := rhs.(*ast.CallExpr) - if !ok { + if call, isCall := rhs.(*ast.CallExpr); isCall { + if version, ok := isSDKConstructorCall(call, ctx.imports); ok { + return version, true + } + if version, ok := isFactoryCall(call, ctx.ownPkgKey, ctx.importAliases, ctx.factoryFunctions); ok { + return version, true + } + } + if ctx.typesInfo != nil { + if version, ok := resolveByStaticType(rhs, ctx.typesInfo); ok { + return version, true + } + } + return "", false +} + +// resolveByStaticType checks rhs's real static type directly against the +// SDK's client type — only reachable when ctx.typesInfo is populated (the +// opt-in --strict-types pass, ScanStrict; see docs/adr/005-strict-types- +// pass.md). This is what proves identity through an interface — +// `var evaluator FlagEvaluator = client` has neither a recognized +// constructor call nor a factory call on its RHS for the syntax-only +// checks above to find, but go/types can tell us directly that the value +// underneath is really *ld.LDClient. Closes issue #15, same-scope only: +// rhs is checked as a single expression, not traced across a function +// boundary (that remains Phase 2b, issue #26). +func resolveByStaticType(rhs ast.Expr, info *types.Info) (version string, ok bool) { + t := info.TypeOf(rhs) + if t == nil { return "", false } - version, ok = isSDKConstructorCall(call, ctx.imports) - if ok { - return version, true + ptr, isPtr := t.(*types.Pointer) + if !isPtr { + return "", false + } + named, isNamed := ptr.Elem().(*types.Named) + if !isNamed || named.Obj().Name() != "LDClient" { + return "", false + } + pkg := named.Obj().Pkg() + if pkg == nil { + return "", false + } + switch pkg.Path() { + case sdkImportV6: + return "v6", true + case sdkImportV7: + return "v7", true + default: + return "", false } - return isFactoryCall(call, ctx.ownPkgKey, ctx.importAliases, ctx.factoryFunctions) } // methodValueBinding records that an identifier was bound to a specific diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index 6c96f95..fa42dfe 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -9,6 +9,7 @@ import ( "go/ast" "go/parser" "go/token" + gotypes "go/types" "os" "path/filepath" "sort" @@ -32,6 +33,12 @@ type parsedFile struct { dir string file *ast.File imports sdkImports + + // typesInfo is real go/types information for this file's package, only + // populated by ScanStrict (strict.go) — nil for an ordinary Scan. See + // fileContext.typesInfo (identity.go) and docs/adr/005-strict-types- + // pass.md. + typesInfo *gotypes.Info } // Scan walks root for files matching cfg's include/exclude patterns, parses @@ -181,6 +188,7 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag importAliases: resolveImportAliases(pf.file, importPathToPkgKey, importPathToPkgName), factoryFunctions: factoryFunctions, structFieldTypes: pkgBindings(structFieldTypesByPkg, ownPkgKey[pf.file]), + typesInfo: pf.typesInfo, } } diff --git a/internal/scanner/strict.go b/internal/scanner/strict.go new file mode 100644 index 0000000..9a84bf9 --- /dev/null +++ b/internal/scanner/strict.go @@ -0,0 +1,146 @@ +package scanner + +import ( + "go/token" + "path/filepath" + "sort" + + "golang.org/x/tools/go/packages" + + "github.com/flaglint/flaglint-go/internal/config" + "github.com/flaglint/flaglint-go/internal/typecheck" + "github.com/flaglint/flaglint-go/internal/types" +) + +// ScanStrict runs Scan (Phase 1, entirely unchanged) and then augments its +// result with additional findings only provable with real go/types +// information — see docs/adr/005-strict-types-pass.md. This is strictly +// additive: every Phase 1 finding is preserved exactly as Scan produced +// it; this pass can only add findings Phase 1's syntactic tracing +// structurally could not prove (today: interface satisfaction, issue #15). +// +// Loading root as a Go module can fail, or fail per-package (a mid- +// refactor branch, a partial checkout, broken dependencies) — this never +// fails the scan outright. A total load failure or a per-package failure +// is recorded as a "typecheck-failure" warning, and Phase 1's result is +// returned with whatever additional findings the packages that *did* +// type-check made possible (zero, if none did). +func ScanStrict(root string, cfg config.Config) (types.ScanResult, error) { + result, err := Scan(root, cfg) + if err != nil { + return types.ScanResult{}, err + } + + absRoot, err := filepath.Abs(root) + if err != nil { + return types.ScanResult{}, err + } + + pkgs, failures, err := typecheck.Load(absRoot) + if err != nil { + result.Warnings = append(result.Warnings, types.ScanWarning{ + Kind: "typecheck-failure", + File: root, + Reason: err.Error(), + }) + return result, nil + } + for _, f := range failures { + result.Warnings = append(result.Warnings, types.ScanWarning{ + Kind: "typecheck-failure", + File: f.PkgPath, + Reason: f.Reason, + }) + } + + mergeStrictTypesUsages(&result, strictTypesUsages(pkgs, absRoot)) + return result, nil +} + +// strictTypesUsages re-runs the whole-scan analysis over go/packages-loaded +// ASTs, which carry real go/types information (Scan's own ASTs, parsed +// independently via go/parser, never do) — this is what lets +// resolveAssignedBinding's static-type fallback (identity.go) fire. +// Re-running the *full* whole-scan analysis, rather than a bespoke walk +// narrowly targeting interface satisfaction, means every existing Phase 1 +// detection mechanism gains type-backed coverage uniformly for free — not +// just the one pattern this pass was originally written for. +func strictTypesUsages(pkgs []*packages.Package, absRoot string) []types.FlagUsage { + var fset *token.FileSet + var parsed []parsedFile + for _, pkg := range pkgs { + if pkg.Fset == nil || pkg.TypesInfo == nil { + continue + } + if fset == nil { + fset = pkg.Fset + } + for _, file := range pkg.Syntax { + pos := pkg.Fset.Position(file.Pos()) + if pos.Filename == "" { + continue + } + rel, err := filepath.Rel(absRoot, pos.Filename) + if err != nil { + continue + } + parsed = append(parsed, parsedFile{ + relPath: rel, + dir: filepath.Dir(pos.Filename), + file: file, + imports: traceSDKImports(file), + typesInfo: pkg.TypesInfo, + }) + } + } + if len(parsed) == 0 { + return nil + } + sort.Slice(parsed, func(i, j int) bool { return parsed[i].relPath < parsed[j].relPath }) + return runWholeScanAnalysis(fset, parsed) +} + +// mergeStrictTypesUsages folds extra into result in place, keeping every +// existing entry byte-for-byte unchanged and adding only fingerprints +// result doesn't already have — the additive guarantee ADR 005 promises, +// enforced here rather than assumed from strictTypesUsages happening to be +// a superset of Scan's own result for the files it covers. +func mergeStrictTypesUsages(result *types.ScanResult, extra []types.FlagUsage) { + if len(extra) == 0 { + return + } + seen := make(map[string]bool, len(result.Usages)) + for _, u := range result.Usages { + seen[u.Fingerprint] = true + } + added := false + for _, u := range extra { + if seen[u.Fingerprint] { + continue + } + seen[u.Fingerprint] = true + // Only stamped on entries that actually survive the dedup above — + // an entry strictTypesUsages also happened to (re-)find that Phase + // 1 already reported keeps its original DetectedBy ("", meaning + // Phase 1) unchanged, since it's Phase 1's own copy of that finding + // that's kept, not this re-scanned one. + u.DetectedBy = "strict-types" + result.Usages = append(result.Usages, u) + added = true + } + if !added { + return + } + sort.Slice(result.Usages, func(i, j int) bool { + a, b := result.Usages[i], result.Usages[j] + if a.File != b.File { + return a.File < b.File + } + if a.Line != b.Line { + return a.Line < b.Line + } + return a.Column < b.Column + }) + result.TotalUsages = len(result.Usages) + result.UniqueFlags = uniqueFlags(result.Usages) +} diff --git a/internal/scanner/strict_test.go b/internal/scanner/strict_test.go new file mode 100644 index 0000000..7e5d420 --- /dev/null +++ b/internal/scanner/strict_test.go @@ -0,0 +1,64 @@ +package scanner + +import ( + "testing" + + "github.com/flaglint/flaglint-go/internal/config" +) + +func TestScanStrict_positiveInterfaceSatisfaction(t *testing.T) { + dir := "testdata/strict/interface_satisfaction" + cfg := config.Config{ + Include: []string{"**/*.go"}, + Exclude: []string{"**/vendor/**", "**/.git/**"}, + Provider: "launchdarkly", + } + + phase1, err := Scan(dir, cfg) + if err != nil { + t.Fatalf("Scan error = %v", err) + } + if len(phase1.Usages) != 0 { + t.Fatalf("Scan (Phase 1) found %d usage(s), want 0 — interface satisfaction is exactly what Phase 1 cannot see: %+v", len(phase1.Usages), phase1.Usages) + } + + strict, err := ScanStrict(dir, cfg) + if err != nil { + t.Fatalf("ScanStrict error = %v", err) + } + if len(strict.Warnings) != 0 { + t.Fatalf("ScanStrict warnings = %+v, want none — the fixture module builds cleanly", strict.Warnings) + } + if len(strict.Usages) != 1 { + t.Fatalf("ScanStrict found %d usage(s), want exactly 1 — an unrelated type satisfying the same interface shape (not *ld.LDClient) must not be detected: %+v", len(strict.Usages), strict.Usages) + } + got := strict.Usages[0] + if got.FlagKey != "interface-satisfaction-flag" { + t.Errorf("usages[0].FlagKey = %q, want interface-satisfaction-flag (not should-not-be-detected)", got.FlagKey) + } + if got.DetectedBy != "strict-types" { + t.Errorf("usages[0].DetectedBy = %q, want strict-types", got.DetectedBy) + } + if got.SDK != "go-server-sdk-v7" { + t.Errorf("usages[0].SDK = %q, want go-server-sdk-v7", got.SDK) + } +} + +func TestScanStrict_failedLoadFallsBackToPhase1(t *testing.T) { + // A directory with no go.mod at all (or above it) fails to load as a + // module — ScanStrict must still return Phase 1's result plus a + // warning, never a hard failure (ADR 005's fail-soft guarantee). + dir := t.TempDir() + cfg := config.Config{Include: []string{"**/*.go"}, Exclude: nil, Provider: "launchdarkly"} + + result, err := ScanStrict(dir, cfg) + if err != nil { + t.Fatalf("ScanStrict error = %v, want nil (fail soft, not fail hard)", err) + } + if len(result.Usages) != 0 { + t.Errorf("usages = %+v, want none (empty dir)", result.Usages) + } + if len(result.Warnings) != 1 || result.Warnings[0].Kind != "typecheck-failure" { + t.Errorf("warnings = %+v, want exactly one typecheck-failure warning", result.Warnings) + } +} diff --git a/internal/scanner/testdata/strict/interface_satisfaction/go.mod b/internal/scanner/testdata/strict/interface_satisfaction/go.mod new file mode 100644 index 0000000..0aa6048 --- /dev/null +++ b/internal/scanner/testdata/strict/interface_satisfaction/go.mod @@ -0,0 +1,7 @@ +module flaglint-strict-fixture-interface-satisfaction + +go 1.22 + +require github.com/launchdarkly/go-server-sdk/v7 v7.0.0 + +replace github.com/launchdarkly/go-server-sdk/v7 => ./stubsdk diff --git a/internal/scanner/testdata/strict/interface_satisfaction/main.go b/internal/scanner/testdata/strict/interface_satisfaction/main.go new file mode 100644 index 0000000..3853853 --- /dev/null +++ b/internal/scanner/testdata/strict/interface_satisfaction/main.go @@ -0,0 +1,41 @@ +package main + +import ( + "time" + + ld "github.com/launchdarkly/go-server-sdk/v7" +) + +// FlagEvaluator is an interface the real client happens to satisfy — Phase +// 1's syntactic tracing sees only this interface type, never the concrete +// *ld.LDClient underneath (issue #15). Only real go/types information +// (--strict-types) can prove evaluator really is a client here. +type FlagEvaluator interface { + BoolVariation(key string, ctx interface{}, defaultVal bool) (bool, error) +} + +func run() { + client, _ := ld.MakeClient("sdk-key", 5*time.Second) + var evaluator FlagEvaluator = client + _, _ = evaluator.BoolVariation("interface-satisfaction-flag", nil, false) +} + +// unrelatedEvaluator satisfies FlagEvaluator's shape but is not, and never +// wraps, a real client — proving interface-satisfaction resolution +// requires the concrete type to actually *be* *ld.LDClient, not just any +// value satisfying the same interface method set. +type unrelatedEvaluator struct{} + +func (u *unrelatedEvaluator) BoolVariation(key string, ctx interface{}, defaultVal bool) (bool, error) { + return defaultVal, nil +} + +func runUnrelated() { + var evaluator FlagEvaluator = &unrelatedEvaluator{} + _, _ = evaluator.BoolVariation("should-not-be-detected", nil, false) +} + +func main() { + run() + runUnrelated() +} diff --git a/internal/scanner/testdata/strict/interface_satisfaction/stubsdk/client.go b/internal/scanner/testdata/strict/interface_satisfaction/stubsdk/client.go new file mode 100644 index 0000000..f87b677 --- /dev/null +++ b/internal/scanner/testdata/strict/interface_satisfaction/stubsdk/client.go @@ -0,0 +1,18 @@ +// Package ldclient is a minimal local stand-in for the real +// github.com/launchdarkly/go-server-sdk/v7 — just enough of its real API +// surface (package name, LDClient type, MakeClient, BoolVariation) for +// go/types to type-check against, without this fixture module needing +// network access or the real (much larger) SDK dependency in CI. +package ldclient + +import "time" + +type LDClient struct{} + +func MakeClient(sdkKey string, waitFor time.Duration) (*LDClient, error) { + return &LDClient{}, nil +} + +func (c *LDClient) BoolVariation(key string, ctx interface{}, defaultVal bool) (bool, error) { + return defaultVal, nil +} diff --git a/internal/scanner/testdata/strict/interface_satisfaction/stubsdk/go.mod b/internal/scanner/testdata/strict/interface_satisfaction/stubsdk/go.mod new file mode 100644 index 0000000..032528e --- /dev/null +++ b/internal/scanner/testdata/strict/interface_satisfaction/stubsdk/go.mod @@ -0,0 +1,3 @@ +module github.com/launchdarkly/go-server-sdk/v7 + +go 1.22 diff --git a/internal/typecheck/load.go b/internal/typecheck/load.go new file mode 100644 index 0000000..339cc6c --- /dev/null +++ b/internal/typecheck/load.go @@ -0,0 +1,118 @@ +// Package typecheck loads a Go module with real go/types information via +// golang.org/x/tools/go/packages — the foundation for flaglint-go's opt-in +// --strict-types pass (see docs/adr/005-strict-types-pass.md). It knows +// nothing about LaunchDarkly or flag detection; it only answers "what are +// this module's packages, and what does the type checker know about them." +// The actual identity-resolution logic that consumes this lives in +// internal/scanner, which already owns every other piece of "what counts +// as an SDK usage" domain knowledge. +package typecheck + +import ( + "fmt" + "os" + "strings" + + "golang.org/x/tools/go/packages" +) + +// LoadFailure records one package that could not be loaded or did not +// type-check cleanly. This is never fatal to the caller — see Load's doc +// comment — it exists purely for the caller to surface as a warning. +type LoadFailure struct { + PkgPath string + Reason string +} + +const loadMode = packages.NeedName | + packages.NeedFiles | + packages.NeedCompiledGoFiles | + packages.NeedImports | + packages.NeedTypes | + packages.NeedTypesInfo | + packages.NeedSyntax + +// Load loads every package in dir (via the "./..." pattern) with full type +// information. Per ADR 005's fail-soft design, a package that fails to +// load or has type errors is never treated as fatal to the whole call — it +// is reported as a LoadFailure and simply excluded from the returned +// slice, so a caller can still make use of whatever packages *did* +// type-check cleanly. Load only returns a non-nil error for something that +// prevents loading from happening at all (e.g. dir isn't a Go module). +func Load(dir string) ([]*packages.Package, []LoadFailure, error) { + // The actual type-checking behind NeedTypes/NeedTypesInfo runs + // in-process (unlike package discovery, which packages.Load shells out + // for) — so it's *this process's* own GODEBUG that governs it, not + // packages.Config.Env, which only reaches the subprocesses Load spawns + // for discovery. os.Setenv (rather than just building a modified Env + // slice) is what's needed to actually affect it — Go's runtime keeps + // GODEBUG's internal cache in sync with os.Setenv calls specifically + // for this purpose. See withGotypesalias's doc comment for why this + // override exists at all. Mutating process-wide state here is only + // safe because Load runs synchronously, once, near the start of a CLI + // invocation — not a pattern to repeat somewhere with concurrent + // unrelated callers. + if updated, changed := withGotypesalias(os.Getenv("GODEBUG")); changed { + os.Setenv("GODEBUG", updated) + } + + cfg := &packages.Config{ + Mode: loadMode, + Dir: dir, + } + + pkgs, err := packages.Load(cfg, "./...") + if err != nil { + return nil, nil, fmt.Errorf("load packages under %s: %w", dir, err) + } + + var loaded []*packages.Package + var failures []LoadFailure + for _, pkg := range pkgs { + // IllTyped covers errors in the package itself *or any dependency* + // (per packages.Package's doc comment) — excluded too, not just + // pkg.Errors, since type info built on an ill-typed dependency + // can't be trusted as proof of anything (ADR 005: fail soft, but + // never guess). + if len(pkg.Errors) == 0 && !pkg.IllTyped { + loaded = append(loaded, pkg) + continue + } + reason := "package has unresolved type errors" + if len(pkg.Errors) > 0 { + reason = pkg.Errors[0].Error() + } + failures = append(failures, LoadFailure{PkgPath: pkg.PkgPath, Reason: reason}) + } + + return loaded, failures, nil +} + +// withGotypesalias returns the GODEBUG value that should be in effect for +// type-checking (godebug, changed=true) — current with gotypesalias forced +// to 1 (real type aliases, not the pre-1.24 desugared representation) — or +// (current, false) if the caller's environment already specifies a +// gotypesalias setting explicitly, which is left alone rather than +// overridden. +// +// Found empirically: flaglint-go's own go.mod declares an older go version +// than a scanned target's, which makes the *compiled binary's* embedded +// GODEBUG default (not the target module's own go version) govern how +// go/types represents the target's type aliases. On a target using Go +// 1.24+ generic type aliases, that mismatch makes every package that +// (transitively) touches one fail to type-check with "generic type alias +// requires GODEBUG=gotypesalias=1 or unset" — confirmed directly against a +// real, large codebase (weaviate/weaviate): 321 of its packages failed +// without this override and zero failed with it. This is a property of +// the *analyzing* binary, unrelated to anything in the scanned module's +// own go.mod, so it must be forced here rather than left for a user to +// discover and work around themselves. +func withGotypesalias(current string) (godebug string, changed bool) { + if strings.Contains(current, "gotypesalias=") { + return current, false + } + if current == "" { + return "gotypesalias=1", true + } + return current + ",gotypesalias=1", true +} diff --git a/internal/typecheck/load_test.go b/internal/typecheck/load_test.go new file mode 100644 index 0000000..9384e0a --- /dev/null +++ b/internal/typecheck/load_test.go @@ -0,0 +1,26 @@ +package typecheck + +import "testing" + +func TestWithGotypesalias(t *testing.T) { + cases := []struct { + name string + current string + wantGodebug string + wantChanged bool + }{ + {"empty", "", "gotypesalias=1", true}, + {"unrelated setting present", "panicnil=1", "panicnil=1,gotypesalias=1", true}, + {"already set explicitly", "gotypesalias=0", "gotypesalias=0", false}, + {"already set alongside others", "panicnil=1,gotypesalias=0", "panicnil=1,gotypesalias=0", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + godebug, changed := withGotypesalias(tc.current) + if godebug != tc.wantGodebug || changed != tc.wantChanged { + t.Errorf("withGotypesalias(%q) = (%q, %v), want (%q, %v)", + tc.current, godebug, changed, tc.wantGodebug, tc.wantChanged) + } + }) + } +} diff --git a/internal/types/types.go b/internal/types/types.go index e9b25f6..62d621f 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -58,6 +58,12 @@ type FlagUsage struct { Language string `json:"language"` // additive: always "go" SDK string `json:"sdk"` // e.g. "go-server-sdk-v7" Risk Risk `json:"risk"` + // DetectedBy is a Go-specific additive field (see ADR 005): "strict-types" + // for a finding only provable with real go/types information (an + // interface-satisfaction case ordinary syntactic tracing can't see), + // omitted entirely for an ordinary Phase 1 finding — so Scan's JSON + // output is byte-for-byte unchanged from before this field existed. + DetectedBy string `json:"detectedBy,omitempty"` } // IsStale reports whether a usage carries any staleness signal. Always false @@ -68,9 +74,10 @@ func IsStale(u FlagUsage) bool { // ScanWarning records a non-fatal problem encountered while scanning. type ScanWarning struct { - Kind string `json:"kind"` // "read-failure" | "parse-failure" - File string `json:"file"` + Kind string `json:"kind"` // "read-failure" | "parse-failure" | "typecheck-failure" + File string `json:"file"` // for "typecheck-failure", the package path that failed to load/type-check, not a file FsCode string `json:"fsCode,omitempty"` // e.g. "ENOENT", "EACCES" — only set for "read-failure" + Reason string `json:"reason,omitempty"` // human-readable cause — only set for "typecheck-failure" } // ScanResult is the top-level output of a scan, shared by audit/scan/validate. From ce056fab65ec37b48bf9a1d820f4273a9c719e44 Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Tue, 7 Jul 2026 13:59:34 -0500 Subject: [PATCH 2/3] fix(typecheck): explicitly discard os.Setenv's error (errcheck) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit golangci-lint's errcheck correctly flagged the unchecked return value. Setenv on a literal, well-formed key like "GODEBUG" can only fail for a key containing "=" or a NUL byte — never true here — so there's nothing meaningful to do with an error even if one somehow occurred; explicitly discard it rather than silence the linter another way. Signed-off-by: Krishan Kant Sharma --- internal/typecheck/load.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/typecheck/load.go b/internal/typecheck/load.go index 339cc6c..edb6be0 100644 --- a/internal/typecheck/load.go +++ b/internal/typecheck/load.go @@ -53,7 +53,11 @@ func Load(dir string) ([]*packages.Package, []LoadFailure, error) { // invocation — not a pattern to repeat somewhere with concurrent // unrelated callers. if updated, changed := withGotypesalias(os.Getenv("GODEBUG")); changed { - os.Setenv("GODEBUG", updated) + // Setenv on a literal, well-formed key like "GODEBUG" can only + // fail for a key containing "=" or a NUL byte — never true here — + // so there's nothing meaningful to do with an error even if one + // somehow occurred. + _ = os.Setenv("GODEBUG", updated) } cfg := &packages.Config{ From 3abeebe33b841044ba833d537f05505bbe151e1e Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Tue, 7 Jul 2026 14:04:00 -0500 Subject: [PATCH 3/3] fix(scanner): dedupe ScanStrict's merge by call site, not fingerprint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review found and reproduced a real data-loss bug: mergeStrictTypesUsages deduped strict-types findings against Phase 1's result by Fingerprint alone. fingerprint.Generate deliberately omits line/column (it's a cross-tool-contract baseline identity, meant to survive line-number churn from unrelated edits elsewhere in the file) — so two genuinely different call sites in the same file sharing a callType and a static flag key produce the identical fingerprint. A real, new strict-types-only finding that happened to collide with an unrelated Phase 1 finding's fingerprint was silently dropped entirely — not merged, not reported, not counted — directly violating the "strictly additive, never lose a real finding" guarantee ADR 005 promises. Fixed by deduping on (File, Line, Column) instead — the call site's actual position, identical between Phase 1's and the strict pass's independent parses of the same source text, and precise enough that two different call sites can never collide regardless of what flag key or call type they happen to share. Also fixes a stray build artifact accidentally committed in the previous commit (go build ./... run manually inside the fixture directory during verification produces a binary there) and adds a .gitignore entry so it can't happen again. Verified: extended the interface-satisfaction fixture with exactly the collision shape review found (runDirect and runInterfaceCollision, same file, same flag key "collision-flag", one Phase-1-visible and one interface-satisfaction-only) — new test asserts both survive as separate usages. Re-verified end-to-end via the built CLI binary. Full test suite green. Signed-off-by: Krishan Kant Sharma --- internal/scanner/strict.go | 27 ++++++++-- internal/scanner/strict_test.go | 49 ++++++++++++++----- .../strict/interface_satisfaction/.gitignore | 1 + .../strict/interface_satisfaction/main.go | 21 ++++++++ 4 files changed, 83 insertions(+), 15 deletions(-) create mode 100644 internal/scanner/testdata/strict/interface_satisfaction/.gitignore diff --git a/internal/scanner/strict.go b/internal/scanner/strict.go index 9a84bf9..ebeb4b0 100644 --- a/internal/scanner/strict.go +++ b/internal/scanner/strict.go @@ -4,6 +4,7 @@ import ( "go/token" "path/filepath" "sort" + "strconv" "golang.org/x/tools/go/packages" @@ -101,24 +102,42 @@ func strictTypesUsages(pkgs []*packages.Package, absRoot string) []types.FlagUsa } // mergeStrictTypesUsages folds extra into result in place, keeping every -// existing entry byte-for-byte unchanged and adding only fingerprints +// existing entry byte-for-byte unchanged and adding only call sites // result doesn't already have — the additive guarantee ADR 005 promises, // enforced here rather than assumed from strictTypesUsages happening to be // a superset of Scan's own result for the files it covers. +// +// Deduping by (File, Line, Column) — a real call site's precise position, +// identical between Phase 1's and the strict pass's independent parses of +// the same source text — not by Fingerprint: fingerprint.Generate +// (internal/fingerprint) deliberately omits line/column (it's a +// cross-tool-contract baseline identity, meant to survive line-number +// churn from unrelated edits elsewhere in the file), so two genuinely +// different call sites in the same file sharing a callType and a static +// flag key produce the *same* fingerprint. Deduping on fingerprint alone +// would silently drop a real strict-types-only finding whenever it +// happened to collide with an unrelated Phase 1 finding's fingerprint — +// found via independent review, reproduced directly (two same-flag-key +// call sites in one file, one Phase-1-visible, one interface-satisfaction- +// only: the second vanished with no warning). func mergeStrictTypesUsages(result *types.ScanResult, extra []types.FlagUsage) { if len(extra) == 0 { return } + callSiteKey := func(u types.FlagUsage) string { + return u.File + ":" + strconv.Itoa(u.Line) + ":" + strconv.Itoa(u.Column) + } seen := make(map[string]bool, len(result.Usages)) for _, u := range result.Usages { - seen[u.Fingerprint] = true + seen[callSiteKey(u)] = true } added := false for _, u := range extra { - if seen[u.Fingerprint] { + key := callSiteKey(u) + if seen[key] { continue } - seen[u.Fingerprint] = true + seen[key] = true // Only stamped on entries that actually survive the dedup above — // an entry strictTypesUsages also happened to (re-)find that Phase // 1 already reported keeps its original DetectedBy ("", meaning diff --git a/internal/scanner/strict_test.go b/internal/scanner/strict_test.go index 7e5d420..4202630 100644 --- a/internal/scanner/strict_test.go +++ b/internal/scanner/strict_test.go @@ -18,8 +18,12 @@ func TestScanStrict_positiveInterfaceSatisfaction(t *testing.T) { if err != nil { t.Fatalf("Scan error = %v", err) } - if len(phase1.Usages) != 0 { - t.Fatalf("Scan (Phase 1) found %d usage(s), want 0 — interface satisfaction is exactly what Phase 1 cannot see: %+v", len(phase1.Usages), phase1.Usages) + // Phase 1 sees runDirect's collision-flag call (a plain, syntactically + // resolvable client.BoolVariation) but neither interface-satisfaction + // call site (run's interface-satisfaction-flag, runInterfaceCollision's + // collision-flag). + if len(phase1.Usages) != 1 || phase1.Usages[0].FlagKey != "collision-flag" { + t.Fatalf("Scan (Phase 1) found %+v, want exactly one collision-flag usage", phase1.Usages) } strict, err := ScanStrict(dir, cfg) @@ -29,18 +33,41 @@ func TestScanStrict_positiveInterfaceSatisfaction(t *testing.T) { if len(strict.Warnings) != 0 { t.Fatalf("ScanStrict warnings = %+v, want none — the fixture module builds cleanly", strict.Warnings) } - if len(strict.Usages) != 1 { - t.Fatalf("ScanStrict found %d usage(s), want exactly 1 — an unrelated type satisfying the same interface shape (not *ld.LDClient) must not be detected: %+v", len(strict.Usages), strict.Usages) + // 3 total: interface-satisfaction-flag (strict-types only) + two + // collision-flag call sites (runDirect, Phase 1; runInterfaceCollision, + // strict-types only) — NOT collapsed into one despite sharing a + // fingerprint (same callType/flagKey/file — fingerprint.Generate + // deliberately omits line/column). should-not-be-detected must still + // be absent. + if len(strict.Usages) != 3 { + t.Fatalf("ScanStrict found %d usage(s), want exactly 3: %+v", len(strict.Usages), strict.Usages) } - got := strict.Usages[0] - if got.FlagKey != "interface-satisfaction-flag" { - t.Errorf("usages[0].FlagKey = %q, want interface-satisfaction-flag (not should-not-be-detected)", got.FlagKey) + + byFlagKeyAndSource := map[string]int{} + for _, u := range strict.Usages { + if u.FlagKey == "should-not-be-detected" { + t.Errorf("usages contains should-not-be-detected: %+v", u) + } + byFlagKeyAndSource[u.FlagKey+"/"+u.DetectedBy]++ + } + want := map[string]int{ + "interface-satisfaction-flag/strict-types": 1, + "collision-flag/": 1, // Phase 1's own copy, DetectedBy left unchanged + "collision-flag/strict-types": 1, } - if got.DetectedBy != "strict-types" { - t.Errorf("usages[0].DetectedBy = %q, want strict-types", got.DetectedBy) + if len(byFlagKeyAndSource) != len(want) { + t.Fatalf("usages = %+v, want %+v", byFlagKeyAndSource, want) } - if got.SDK != "go-server-sdk-v7" { - t.Errorf("usages[0].SDK = %q, want go-server-sdk-v7", got.SDK) + for k, wantCount := range want { + if byFlagKeyAndSource[k] != wantCount { + t.Errorf("count[%q] = %d, want %d (full: %+v)", k, byFlagKeyAndSource[k], wantCount, strict.Usages) + } + } + + for _, u := range strict.Usages { + if u.SDK != "go-server-sdk-v7" { + t.Errorf("usage %+v SDK = %q, want go-server-sdk-v7", u, u.SDK) + } } } diff --git a/internal/scanner/testdata/strict/interface_satisfaction/.gitignore b/internal/scanner/testdata/strict/interface_satisfaction/.gitignore new file mode 100644 index 0000000..91d0acb --- /dev/null +++ b/internal/scanner/testdata/strict/interface_satisfaction/.gitignore @@ -0,0 +1 @@ +/flaglint-strict-fixture-interface-satisfaction diff --git a/internal/scanner/testdata/strict/interface_satisfaction/main.go b/internal/scanner/testdata/strict/interface_satisfaction/main.go index 3853853..adc9511 100644 --- a/internal/scanner/testdata/strict/interface_satisfaction/main.go +++ b/internal/scanner/testdata/strict/interface_satisfaction/main.go @@ -35,7 +35,28 @@ func runUnrelated() { _, _ = evaluator.BoolVariation("should-not-be-detected", nil, false) } +// runDirect and runInterfaceCollision both evaluate the exact same flag +// key with the exact same call type in this same file — a fingerprint +// (internal/fingerprint) is keyed on exactly that (callType, flagKey, +// file), deliberately omitting line/column, so these two distinct real +// call sites produce identical fingerprints. This reproduces a real bug +// caught during independent review: deduping ScanStrict's merge on +// fingerprint alone silently dropped runInterfaceCollision's genuinely +// new finding because it collided with runDirect's Phase-1-visible one — +// the merge must dedupe on the call site's actual position instead. +func runDirect(client *ld.LDClient) { + _, _ = client.BoolVariation("collision-flag", nil, false) +} + +func runInterfaceCollision() { + client, _ := ld.MakeClient("sdk-key", 5*time.Second) + var evaluator FlagEvaluator = client + _, _ = evaluator.BoolVariation("collision-flag", nil, false) +} + func main() { run() runUnrelated() + runDirect(nil) + runInterfaceCollision() }