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..ebeb4b0 --- /dev/null +++ b/internal/scanner/strict.go @@ -0,0 +1,165 @@ +package scanner + +import ( + "go/token" + "path/filepath" + "sort" + "strconv" + + "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 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[callSiteKey(u)] = true + } + added := false + for _, u := range extra { + key := callSiteKey(u) + if seen[key] { + continue + } + 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 + // 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..4202630 --- /dev/null +++ b/internal/scanner/strict_test.go @@ -0,0 +1,91 @@ +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) + } + // 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) + 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) + } + // 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) + } + + 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 len(byFlagKeyAndSource) != len(want) { + t.Fatalf("usages = %+v, want %+v", byFlagKeyAndSource, want) + } + 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) + } + } +} + +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/.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/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..adc9511 --- /dev/null +++ b/internal/scanner/testdata/strict/interface_satisfaction/main.go @@ -0,0 +1,62 @@ +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) +} + +// 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() +} 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..edb6be0 --- /dev/null +++ b/internal/typecheck/load.go @@ -0,0 +1,122 @@ +// 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 { + // 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{ + 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.