From d4619a385b1fd81844be3e69cc76227682406482 Mon Sep 17 00:00:00 2001 From: Krishan Kant Sharma Date: Wed, 8 Jul 2026 09:21:52 -0500 Subject: [PATCH] feat(scanner): implement migrationInventory for cross-tool parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #37. flaglint-js's ScanResult carries an optional migrationInventory field — a richer, migration-focused record of each call site meant to drive a future migrate rewriter. The cross-tool spec marks this optional for other implementations "until spec v1.2", but since flaglint-js already emits it, this implements it now for real parity (ADR 003) rather than waiting — flaglint-go still has no migrate/--apply command itself (ADR 002's Phase 1 scope); this is the data foundation a future command would need, not the command. Mirrors flaglint-js's MigrationInventoryItem field-for-field (types.go): file/line/column, launchDarklyMethod, callExpression + rangeStart/ rangeEnd (raw source text and byte-offset range of the whole call), flagKeyExpression/staticFlagKey, isDynamic, valueType, fallbackExpression, evaluationContextExpression, safelyAutomatable, manualReviewReason. One deliberate omission: flaglint-js's isAwaited has no Go equivalent (no async/await) and is never emitted. Unlike flaglint-js — whose generic variation()/isFeatureEnabled() calls need valueType inferred from the fallback argument's runtime type — every Go SDK method name is already type-specific (BoolVariation/ StringVariation/IntVariation/Float64Variation/JSONVariation), so valueType is always derived directly from the method name (migration.go's migrationValueTypes map); "unknown-fallback" is kept in the type for cross-tool consistency but flaglint-go itself never produces it. manualReviewReason otherwise mirrors flaglint-js's logic exactly: bulk-inventory-call for AllFlagsState, dynamic-key when the flag key isn't a literal, detail-method for *VariationDetail(Ctx) methods, else safelyAutomatable: true. Wired into fileDetector.detect (scanner.go) at the same call site that already builds each FlagUsage, reusing the same call/spec/flagKey/ isDynamic already computed there — buildMigrationInventoryItem (migration.go) extracts each argument's exact source text via byte offsets against the file's raw source bytes, now retained in parsedFile (previously discarded once go/parser succeeded). Both --strict-types-derived usage sources get different treatment, matching what's actually safe to claim: - interface satisfaction / transitive factory wrapping (strictTypesUsages, a re-run of the same runWholeScanAnalysis/detect() path) get full migration items for free, since their call sites are ordinary client.Method(key, context, fallback) shapes — only how the receiver's identity gets proven differs. strict.go now separately re-reads each file's source bytes for this pass (go/packages' own loaded ASTs don't retain the raw text any more than Scan()'s own parser.ParseFile does). - forwarding-function usages (detectForwardingCallUsages) get no migration item at all: their call site doesn't directly show the LD method's own (key, context, fallback) arguments (it shows the wrapper call, e.g. callDirect(client.BoolVariation, key, def)), so an item for one would misrepresent what's actually safely rewritable. mergeStrictTypesUsages now merges migration items index-aligned with the usages they came from, when present. Also fixes a latent, pre-existing bug found while verifying this: the existing "empty arrays must not be null" tests (reporter and CLI) checked for `"field":null` (no space) but json.MarshalIndent always emits `"field": null` (with a space) — the checks were vacuously true regardless of the actual field values. Fixed both to match real formatting and construct explicit non-nil-but-empty inputs where the old code relied on Go's zero-value nil slice, which was never a valid stand-in for what Scan() itself actually guarantees. Verified: full test suite green, including new regression coverage (TestScanFile_positiveVarietyMigrationInventory covering dynamic-key, detail-method, bulk-call, and *Ctx-variant argument-shifting cases; new assertions in the existing interface-satisfaction and forwarding-call --strict-types tests proving the inclusion/exclusion split above). Manually confirmed exact field values via the built binary against a synthetic fixture covering all five method shapes, and confirmed no crashes/regressions scanning weaviate and mint-app (item count matches usage count in both, values sane). Signed-off-by: Krishan Kant Sharma --- docs/adr/003-cross-tool-contract.md | 24 +++++ internal/cli/cli_test.go | 4 +- internal/reporter/reporter_test.go | 22 ++++- internal/scanner/migration.go | 132 ++++++++++++++++++++++++++++ internal/scanner/scanner.go | 48 +++++++--- internal/scanner/scanner_test.go | 108 ++++++++++++++++++++++- internal/scanner/strict.go | 52 +++++++++-- internal/scanner/strict_test.go | 26 ++++++ internal/types/types.go | 67 ++++++++++++++ 9 files changed, 460 insertions(+), 23 deletions(-) create mode 100644 internal/scanner/migration.go diff --git a/docs/adr/003-cross-tool-contract.md b/docs/adr/003-cross-tool-contract.md index 00dff32..6deeb61 100644 --- a/docs/adr/003-cross-tool-contract.md +++ b/docs/adr/003-cross-tool-contract.md @@ -140,6 +140,30 @@ have: audit does not yet implement staleness heuristics (keyword/path/minFileCount). This is a placeholder for parity, not a promise it will be populated soon. +`ScanResult.migrationInventory` mirrors flaglint-js's `MigrationInventoryItem` +field-for-field (`file`, `line`, `column`, `launchDarklyMethod`, +`callExpression`, `rangeStart`, `rangeEnd`, `flagKeyExpression`, +`staticFlagKey`, `isDynamic`, `valueType`, `fallbackExpression`, +`evaluationContextExpression`, `safelyAutomatable`, `manualReviewReason`) — +richer, migration-focused detail a future `migrate` command would need to +safely rewrite a call, not just report it (flaglint-go has no `migrate` +command yet — see ADR 002's Phase 1 scope). One deliberate omission: +flaglint-js's `isAwaited?: boolean` has no Go equivalent (no async/await) and +is never emitted. Unlike flaglint-js (whose generic `variation()`/ +`isFeatureEnabled()` calls require inferring `valueType` from the fallback +argument's runtime type), every Go SDK method name is already type-specific, +so `valueType` is always derived directly from the method name — flaglint-go +never actually produces `"unknown-fallback"` as a `manualReviewReason` as a +result (kept in the type for cross-tool consistency regardless). A +`--strict-types`-only usage detected via the "forwarding function" pattern +(ADR 006) has no corresponding `migrationInventory` item: its call site +doesn't directly show the LD method's own `(key, context, fallback)` +arguments, so an item for it would misrepresent what's actually safe to +rewrite. Every other usage — Phase 1 and interface-satisfaction/transitive- +factory-wrapping `--strict-types` findings alike — gets one, since their call +sites are all ordinary `client.Method(key, context, fallback)` shapes +regardless of how the receiver's identity was proven. + ## Consequences - Any PR to flaglint-go that changes fingerprint format, exit codes, config diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index a570f5f..88068cd 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -219,7 +219,9 @@ func TestCLI_emptyScanProducesEmptyArraysNotNull(t *testing.T) { if err != nil { t.Fatalf("scan failed: %v", err) } - for _, field := range []string{`"uniqueFlags":null`, `"usages":null`, `"warnings":null`} { + // json.MarshalIndent inserts a space after the colon, so the check must + // match that exactly, not compact-JSON spacing. + for _, field := range []string{`"uniqueFlags": null`, `"usages": null`, `"warnings": null`, `"migrationInventory": null`} { if strings.Contains(string(out), field) { t.Errorf("output contains %q — empty results must serialize as [], not null:\n%s", field, out) } diff --git a/internal/reporter/reporter_test.go b/internal/reporter/reporter_test.go index 2a64b7c..c56d88a 100644 --- a/internal/reporter/reporter_test.go +++ b/internal/reporter/reporter_test.go @@ -89,7 +89,21 @@ func TestRender_unsupportedFormat(t *testing.T) { } func TestRender_json_emptyArraysAreNotNull(t *testing.T) { - out, err := Render(types.ScanResult{}, Options{Format: FormatJSON}) + // Explicit empty (non-nil) slices, matching what Scan() itself actually + // produces for a clean scan — NOT a bare types.ScanResult{}, whose + // zero-value slice fields are nil by construction and would marshal as + // `null` regardless of any tag on these fields; that would make this + // test unable to ever catch the real regression it exists to guard + // against: an accidentally-added `omitempty` tag on one of these + // fields, which turns a non-nil empty slice's `[]` output into total + // field omission instead. + result := types.ScanResult{ + UniqueFlags: []string{}, + Usages: []types.FlagUsage{}, + Warnings: []types.ScanWarning{}, + MigrationInventory: []types.MigrationInventoryItem{}, + } + out, err := Render(result, Options{Format: FormatJSON}) if err != nil { t.Fatalf("Render() error = %v", err) } @@ -97,8 +111,10 @@ func TestRender_json_emptyArraysAreNotNull(t *testing.T) { // decoder would happily turn a `null` array back into an empty slice, // masking the exact bug this guards (nil slices marshaling as `null` // instead of `[]`, which breaks a consumer's `.map()`/`jq` pipeline on - // the JSON *text* itself). - for _, field := range []string{`"uniqueFlags":null`, `"usages":null`, `"warnings":null`} { + // the JSON *text* itself). json.MarshalIndent inserts a space after + // the colon, so the check must match that exactly, not compact-JSON + // spacing. + for _, field := range []string{`"uniqueFlags": null`, `"usages": null`, `"warnings": null`, `"migrationInventory": null`} { if strings.Contains(out, field) { t.Errorf("output contains %q — empty slices must marshal as [], not null, to match flaglint-js's array semantics:\n%s", field, out) } diff --git a/internal/scanner/migration.go b/internal/scanner/migration.go new file mode 100644 index 0000000..eeb91d7 --- /dev/null +++ b/internal/scanner/migration.go @@ -0,0 +1,132 @@ +// Package scanner (this file): builds MigrationInventoryItem records — +// migrationInventory is a richer, migration-focused view of a call site +// than FlagUsage, additive to flaglint-go's JSON output for cross-tool +// parity with flaglint-js (see docs/adr/003-cross-tool-contract.md and +// flaglint-js's src/scanner/index.ts, which this deliberately mirrors +// field-for-field). flaglint-go has no `migrate` command yet (ADR 002's +// Phase 1 scope), so nothing in this package *consumes* these records — +// they exist so a future migrate command (or another tool reading this +// JSON) has the same rewrite-relevant detail flaglint-js already provides. +package scanner + +import ( + "go/ast" + "go/token" + + "github.com/flaglint/flaglint-go/internal/types" +) + +// migrationValueTypes maps each known LaunchDarkly Go SDK method name to +// the OpenFeature evaluation-method category its return value maps to. +// Unlike flaglint-js (whose generic variation()/isFeatureEnabled() calls +// require inferring this from the fallback argument's runtime type), every +// Go SDK method name already fully determines this — no fallback +// inspection needed, and "unknown-fallback" (a real flaglint-js reason) +// can never actually be produced here as a result. +var migrationValueTypes = map[string]types.MigrationValueType{ + "BoolVariation": types.MigrationValueBoolean, "BoolVariationCtx": types.MigrationValueBoolean, + "BoolVariationDetail": types.MigrationValueBoolean, "BoolVariationDetailCtx": types.MigrationValueBoolean, + "StringVariation": types.MigrationValueString, "StringVariationCtx": types.MigrationValueString, + "StringVariationDetail": types.MigrationValueString, "StringVariationDetailCtx": types.MigrationValueString, + "IntVariation": types.MigrationValueNumber, "IntVariationCtx": types.MigrationValueNumber, + "IntVariationDetail": types.MigrationValueNumber, "IntVariationDetailCtx": types.MigrationValueNumber, + "Float64Variation": types.MigrationValueNumber, "Float64VariationCtx": types.MigrationValueNumber, + "Float64VariationDetail": types.MigrationValueNumber, "Float64VariationDetailCtx": types.MigrationValueNumber, + "JSONVariation": types.MigrationValueObject, "JSONVariationCtx": types.MigrationValueObject, + "JSONVariationDetail": types.MigrationValueObject, "JSONVariationDetailCtx": types.MigrationValueObject, +} + +// migrationDetailMethods is every "*VariationDetail(Ctx)" method name — +// always manual-review (returns ldreason.EvaluationDetail, no direct +// OpenFeature equivalent), matching flaglint-js's detail-method reason. +var migrationDetailMethods = map[string]bool{ + "BoolVariationDetail": true, "BoolVariationDetailCtx": true, + "StringVariationDetail": true, "StringVariationDetailCtx": true, + "IntVariationDetail": true, "IntVariationDetailCtx": true, + "Float64VariationDetail": true, "Float64VariationDetailCtx": true, + "JSONVariationDetail": true, "JSONVariationDetailCtx": true, +} + +// exprText extracts node's exact source text and byte-offset range from +// src, using fset to convert AST positions to offsets. Returns ok == false +// for a nil node or a position outside src's bounds (never expected in +// practice — pos/end always come from nodes parsed out of src itself — but +// checked rather than assumed, since a malformed range would otherwise +// panic on the slice below). +func exprText(fset *token.FileSet, src []byte, node ast.Expr) (text string, start, end int, ok bool) { + if node == nil { + return "", 0, 0, false + } + startPos := fset.Position(node.Pos()) + endPos := fset.Position(node.End()) + if startPos.Offset < 0 || endPos.Offset > len(src) || startPos.Offset > endPos.Offset { + return "", 0, 0, false + } + return string(src[startPos.Offset:endPos.Offset]), startPos.Offset, endPos.Offset, true +} + +// buildMigrationInventoryItem builds one MigrationInventoryItem for call, +// a confirmed LaunchDarkly Go SDK call site (spec/callTypeName already +// resolved by the caller — see fileDetector.detect, scanner.go). src is +// this file's raw source bytes (parsedFile.src), needed to extract each +// expression's exact source text alongside its already-parsed AST shape. +func buildMigrationInventoryItem(fset *token.FileSet, src []byte, relPath string, call *ast.CallExpr, spec methodSpec, callTypeName string, pos token.Position, flagKey string, isDynamic bool) types.MigrationInventoryItem { + item := types.MigrationInventoryItem{ + File: relPath, + Line: pos.Line, + Column: pos.Column, + LaunchDarklyMethod: types.CallType(callTypeName), + IsDynamic: isDynamic, + } + if text, start, end, ok := exprText(fset, src, call); ok { + item.CallExpression = text + item.RangeStart = start + item.RangeEnd = end + } + + // Bulk call (AllFlagsState) — no flag key; args[0] is the evaluation + // context, matching flaglint-js's identical handling of its bulk-call + // equivalent. + if spec.keyArgIndex == -1 { + item.ValueType = types.MigrationValueUnknown + if len(call.Args) > 0 { + if text, _, _, ok := exprText(fset, src, call.Args[0]); ok { + item.EvaluationContextExpression = text + } + } + item.ManualReviewReason = types.MigrationReasonBulkInventoryCall + return item + } + + if spec.keyArgIndex < len(call.Args) { + if text, _, _, ok := exprText(fset, src, call.Args[spec.keyArgIndex]); ok { + item.FlagKeyExpression = text + } + } + if !isDynamic { + item.StaticFlagKey = flagKey + } + if ctxIdx := spec.keyArgIndex + 1; ctxIdx < len(call.Args) { + if text, _, _, ok := exprText(fset, src, call.Args[ctxIdx]); ok { + item.EvaluationContextExpression = text + } + } + if fallbackIdx := spec.keyArgIndex + 2; fallbackIdx < len(call.Args) { + if text, _, _, ok := exprText(fset, src, call.Args[fallbackIdx]); ok { + item.FallbackExpression = text + } + } + + item.ValueType = migrationValueTypes[callTypeName] + + switch { + case isDynamic: + item.ManualReviewReason = types.MigrationReasonDynamicKey + case migrationDetailMethods[callTypeName]: + item.ManualReviewReason = types.MigrationReasonDetailMethod + default: + item.SafelyAutomatable = true + } + + return item +} diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index 9cd34a3..d86ca17 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -34,6 +34,13 @@ type parsedFile struct { file *ast.File imports sdkImports + // src is this file's raw source bytes, kept alongside its already- + // parsed AST so migrationInventory (migration.go) can extract each + // call/argument's exact source text — go/parser discards the original + // text once parsing succeeds, so this has to be retained separately if + // anything downstream needs it. + src []byte + // 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- @@ -87,21 +94,23 @@ func Scan(root string, cfg config.Config) (types.ScanResult, error) { relPath: rel, dir: filepath.Dir(full), file: file, + src: src, imports: traceSDKImports(file), }) } - allUsages := runWholeScanAnalysis(fset, parsed) + allUsages, allMigrationInventory := runWholeScanAnalysis(fset, parsed) return types.ScanResult{ - ScannedAt: time.Now().UTC().Format(time.RFC3339), - ScanRoot: absRoot, - ScannedFiles: len(relFiles), - TotalUsages: len(allUsages), - UniqueFlags: uniqueFlags(allUsages), - Usages: allUsages, - ScanDurationMs: time.Since(start).Milliseconds(), - Warnings: warnings, + ScannedAt: time.Now().UTC().Format(time.RFC3339), + ScanRoot: absRoot, + ScannedFiles: len(relFiles), + TotalUsages: len(allUsages), + UniqueFlags: uniqueFlags(allUsages), + Usages: allUsages, + ScanDurationMs: time.Since(start).Milliseconds(), + Warnings: warnings, + MigrationInventory: allMigrationInventory, }, nil } @@ -114,7 +123,7 @@ func Scan(root string, cfg config.Config) (types.ScanResult, error) { // client behind a struct field, a factory/getter function, or a // multi-level field chain declared in a different file (sometimes a // different package) than where it's used. -func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.FlagUsage { +func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) ([]types.FlagUsage, []types.MigrationInventoryItem) { // Pre-pass 1: package identity. ownPkgKey identifies each file's own // package (for same-package bare factory calls); importPathToPkgKey and // importPathToPkgName let OTHER files resolve a qualified @@ -305,10 +314,11 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag // literal in one file/function can make a field binding visible to a // different file processed earlier in this loop. var allUsages []types.FlagUsage + var allMigrationInventory []types.MigrationInventoryItem for _, pf := range parsed { ctx := ctxs[pf.file] pkg := pkgBindings(base, ctx.ownPkgKey) - d := &fileDetector{fset: fset, relPath: pf.relPath} + d := &fileDetector{fset: fset, relPath: pf.relPath, src: pf.src} for _, s := range fileScopes[pf.file] { // packageVarTypes merged in first (mergeBindings's later // argument wins on collision) so a local parameter of the @@ -318,12 +328,16 @@ func runWholeScanAnalysis(fset *token.FileSet, parsed []parsedFile) []types.Flag d.detect(s.body, mergeBindings(pkg, s.paramBindings), declared, ctx.structFieldTypes, ctx) } allUsages = append(allUsages, d.usages...) + allMigrationInventory = append(allMigrationInventory, d.migrationInventory...) } if allUsages == nil { allUsages = []types.FlagUsage{} } - return allUsages + if allMigrationInventory == nil { + allMigrationInventory = []types.MigrationInventoryItem{} + } + return allUsages, allMigrationInventory } // pkgBindings returns byPkg's inner map for pkgKey, creating it on first @@ -406,6 +420,14 @@ type fileDetector struct { relPath string dynamicIndex int usages []types.FlagUsage + + // src is this file's raw source bytes (parsedFile.src), used to build + // each usage's corresponding MigrationInventoryItem — see migration.go. + // May be nil (e.g. a read failure in strict.go's own re-read for the + // --strict-types pass): buildMigrationInventoryItem's exprText calls + // degrade safely to missing expression text in that case, never a panic. + src []byte + migrationInventory []types.MigrationInventoryItem } // bindings is the scope's initial bindings (whole-scan package bindings @@ -490,6 +512,7 @@ func (d *fileDetector) detect(scope ast.Node, bindings, declared, structFieldTyp SDK: sdk, Risk: spec.risk, }) + d.migrationInventory = append(d.migrationInventory, buildMigrationInventoryItem(d.fset, d.src, d.relPath, call, spec, callTypeName, pos, "*", false)) return } @@ -519,6 +542,7 @@ func (d *fileDetector) detect(scope ast.Node, bindings, declared, structFieldTyp SDK: sdk, Risk: riskFor(spec, isDynamic), }) + d.migrationInventory = append(d.migrationInventory, buildMigrationInventoryItem(d.fset, d.src, d.relPath, call, spec, callTypeName, pos, flagKey, isDynamic)) }) } diff --git a/internal/scanner/scanner_test.go b/internal/scanner/scanner_test.go index c79bcfc..82f5bae 100644 --- a/internal/scanner/scanner_test.go +++ b/internal/scanner/scanner_test.go @@ -3,6 +3,7 @@ package scanner import ( "go/parser" "go/token" + "os" "path/filepath" "testing" @@ -22,12 +23,26 @@ func parseFixture(t *testing.T, name string) []types.FlagUsage { } func parseFixtures(t *testing.T, names ...string) []types.FlagUsage { + t.Helper() + usages, _ := parseFixturesWithMigration(t, names...) + return usages +} + +// parseFixturesWithMigration is parseFixtures plus the migrationInventory +// items runWholeScanAnalysis produces alongside usages — its own function +// (rather than changing parseFixtures' return shape) so every existing +// call site that only cares about usages stays unchanged. +func parseFixturesWithMigration(t *testing.T, names ...string) ([]types.FlagUsage, []types.MigrationInventoryItem) { t.Helper() fset := token.NewFileSet() var parsed []parsedFile for _, name := range names { path := filepath.Join("testdata", "fixtures", name) - file, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + src, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile(%s) error = %v", name, err) + } + file, err := parser.ParseFile(fset, path, src, parser.SkipObjectResolution) if err != nil { t.Fatalf("ParseFile(%s) error = %v", name, err) } @@ -39,6 +54,7 @@ func parseFixtures(t *testing.T, names ...string) []types.FlagUsage { relPath: name, dir: absDir, file: file, + src: src, imports: traceSDKImports(file), }) } @@ -353,6 +369,96 @@ func TestScanFile_positiveVariety(t *testing.T) { } } +func TestScanFile_positiveVarietyMigrationInventory(t *testing.T) { + usages, items := parseFixturesWithMigration(t, "positive_variety.go") + if len(usages) != 5 || len(items) != 5 { + t.Fatalf("got %d usages / %d migration items, want 5/5: %+v / %+v", len(usages), len(items), usages, items) + } + + checkRange := func(t *testing.T, item types.MigrationInventoryItem) { + t.Helper() + if item.RangeStart >= item.RangeEnd { + t.Errorf("RangeStart/RangeEnd = %d/%d, want start < end", item.RangeStart, item.RangeEnd) + } + if item.RangeEnd-item.RangeStart != len(item.CallExpression) { + t.Errorf("range length = %d, want len(CallExpression) = %d", item.RangeEnd-item.RangeStart, len(item.CallExpression)) + } + } + + // 1: dynamic identifier key — manual review, dynamic-key. + item := items[0] + checkRange(t, item) + if item.CallExpression != `client.BoolVariation(flagName, nil, false)` { + t.Errorf("items[0].CallExpression = %q", item.CallExpression) + } + if item.FlagKeyExpression != "flagName" || item.StaticFlagKey != "" || !item.IsDynamic { + t.Errorf("items[0] key fields = %+v, want dynamic flagName with no static key", item) + } + if item.EvaluationContextExpression != "nil" || item.FallbackExpression != "false" { + t.Errorf("items[0] context/fallback = %+v", item) + } + if item.ValueType != types.MigrationValueBoolean { + t.Errorf("items[0].ValueType = %q, want boolean", item.ValueType) + } + if item.SafelyAutomatable || item.ManualReviewReason != types.MigrationReasonDynamicKey { + t.Errorf("items[0] = %+v, want safelyAutomatable=false, reason=dynamic-key", item) + } + + // 2: dynamic fmt.Sprintf key — same reason, string value type. + item = items[1] + checkRange(t, item) + if item.FlagKeyExpression != `fmt.Sprintf("flag-%d", 1)` { + t.Errorf("items[1].FlagKeyExpression = %q", item.FlagKeyExpression) + } + if item.ValueType != types.MigrationValueString { + t.Errorf("items[1].ValueType = %q, want string", item.ValueType) + } + if item.SafelyAutomatable || item.ManualReviewReason != types.MigrationReasonDynamicKey { + t.Errorf("items[1] = %+v, want safelyAutomatable=false, reason=dynamic-key", item) + } + + // 3: *Detail method — manual review regardless of a static key. + item = items[2] + checkRange(t, item) + if item.StaticFlagKey != "detail-flag" || item.IsDynamic { + t.Errorf("items[2] key fields = %+v, want static detail-flag", item) + } + if item.SafelyAutomatable || item.ManualReviewReason != types.MigrationReasonDetailMethod { + t.Errorf("items[2] = %+v, want safelyAutomatable=false, reason=detail-method", item) + } + + // 4: bulk call — no flag key at all, evaluation context is args[0]. + item = items[3] + checkRange(t, item) + if item.FlagKeyExpression != "" || item.StaticFlagKey != "" || item.IsDynamic { + t.Errorf("items[3] key fields = %+v, want no flag key at all", item) + } + if item.EvaluationContextExpression != "nil" { + t.Errorf("items[3].EvaluationContextExpression = %q, want nil (args[0])", item.EvaluationContextExpression) + } + if item.ValueType != types.MigrationValueUnknown { + t.Errorf("items[3].ValueType = %q, want unknown", item.ValueType) + } + if item.SafelyAutomatable || item.ManualReviewReason != types.MigrationReasonBulkInventoryCall { + t.Errorf("items[3] = %+v, want safelyAutomatable=false, reason=bulk-inventory-call", item) + } + + // 5: *Ctx variant — key/context/fallback are shifted one position later + // (a leading context.Context argument), and this one IS safely + // automatable (static key, non-detail, non-bulk, non-dynamic). + item = items[4] + checkRange(t, item) + if item.FlagKeyExpression != `"ctx-flag"` || item.StaticFlagKey != "ctx-flag" || item.IsDynamic { + t.Errorf("items[4] key fields = %+v, want static ctx-flag from arg index 1", item) + } + if item.EvaluationContextExpression != "nil" || item.FallbackExpression != "false" { + t.Errorf("items[4] context/fallback = %+v, want args[2]/args[3]", item) + } + if !item.SafelyAutomatable || item.ManualReviewReason != "" { + t.Errorf("items[4] = %+v, want safelyAutomatable=true, no review reason", item) + } +} + func TestScanFile_positivePackageVarIIFE(t *testing.T) { usages := parseFixture(t, "positive_package_var_iife.go") if len(usages) != 1 { diff --git a/internal/scanner/strict.go b/internal/scanner/strict.go index 7051197..069e50d 100644 --- a/internal/scanner/strict.go +++ b/internal/scanner/strict.go @@ -3,6 +3,7 @@ package scanner import ( "go/token" gotypes "go/types" + "os" "path/filepath" "sort" "strconv" @@ -55,8 +56,12 @@ func ScanStrict(root string, cfg config.Config) (types.ScanResult, error) { }) } - mergeStrictTypesUsages(&result, strictTypesUsages(pkgs, absRoot)) - mergeStrictTypesUsages(&result, forwardingCallUsages(pkgs, absRoot)) + strictUsages, strictMigrationItems := strictTypesUsages(pkgs, absRoot) + mergeStrictTypesUsages(&result, strictUsages, strictMigrationItems) + // forwardingCallUsages' call sites don't directly show the LD method's + // own (key, context, fallback) arguments (see MigrationInventory's doc + // comment, types.go) — no migration items to merge for these. + mergeStrictTypesUsages(&result, forwardingCallUsages(pkgs, absRoot), nil) return result, nil } @@ -115,7 +120,7 @@ func forwardingCallUsages(pkgs []*packages.Package, absRoot string) []types.Flag // 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 { +func strictTypesUsages(pkgs []*packages.Package, absRoot string) ([]types.FlagUsage, []types.MigrationInventoryItem) { var fset *token.FileSet var parsed []parsedFile for _, pkg := range pkgs { @@ -134,17 +139,27 @@ func strictTypesUsages(pkgs []*packages.Package, absRoot string) []types.FlagUsa if err != nil { continue } + // Read separately from go/packages' own loading (a second read + // of a file already-parsed) — migrationInventory needs each + // call/argument's exact source text, which go/packages' parsed + // ASTs don't retain any more than Scan's own parser.ParseFile + // does. A read failure here isn't reported as a scan warning: + // it only degrades this one file's migrationInventory items to + // missing expression text (exprText's nil-src bounds check + // handles that safely), never the FlagUsage detection itself. + src, _ := os.ReadFile(pos.Filename) parsed = append(parsed, parsedFile{ relPath: rel, dir: filepath.Dir(pos.Filename), file: file, + src: src, imports: traceSDKImports(file), typesInfo: pkg.TypesInfo, }) } } if len(parsed) == 0 { - return nil + return nil, nil } sort.Slice(parsed, func(i, j int) bool { return parsed[i].relPath < parsed[j].relPath }) return runWholeScanAnalysis(fset, parsed) @@ -169,7 +184,15 @@ func strictTypesUsages(pkgs []*packages.Package, absRoot string) []types.FlagUsa // 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) { +// extraMigrationItems, when non-nil, is index-aligned with extra (both +// built by the same runWholeScanAnalysis/detect() walk, in the same +// order) — the migration item for extra[i] is extraMigrationItems[i] if +// present. forwardingCallUsages has no migration-item equivalent at all +// (its call sites don't directly show the LD method's own (key, context, +// fallback) arguments — see MigrationInventory's doc comment, types.go), +// so its caller passes nil here; the length check below just means no +// item is merged for any of its usages, never a panic. +func mergeStrictTypesUsages(result *types.ScanResult, extra []types.FlagUsage, extraMigrationItems []types.MigrationInventoryItem) { if len(extra) == 0 { return } @@ -181,7 +204,8 @@ func mergeStrictTypesUsages(result *types.ScanResult, extra []types.FlagUsage) { seen[callSiteKey(u)] = true } added := false - for _, u := range extra { + migrationAdded := false + for i, u := range extra { key := callSiteKey(u) if seen[key] { continue @@ -195,6 +219,22 @@ func mergeStrictTypesUsages(result *types.ScanResult, extra []types.FlagUsage) { u.DetectedBy = "strict-types" result.Usages = append(result.Usages, u) added = true + if i < len(extraMigrationItems) { + result.MigrationInventory = append(result.MigrationInventory, extraMigrationItems[i]) + migrationAdded = true + } + } + if migrationAdded { + sort.Slice(result.MigrationInventory, func(i, j int) bool { + a, b := result.MigrationInventory[i], result.MigrationInventory[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 + }) } if !added { return diff --git a/internal/scanner/strict_test.go b/internal/scanner/strict_test.go index 131ebb3..c01bbf8 100644 --- a/internal/scanner/strict_test.go +++ b/internal/scanner/strict_test.go @@ -70,6 +70,23 @@ func TestScanStrict_positiveInterfaceSatisfaction(t *testing.T) { t.Errorf("usage %+v SDK = %q, want go-server-sdk-v7", u, u.SDK) } } + + // Interface-satisfaction usages go through the same detect() call- + // argument extraction as any Phase 1 usage (only how the receiver's + // identity gets proven differs — the call site itself is an ordinary + // e.BoolVariation(key, ctx, default)), so they get full + // MigrationInventoryItems too, not just FlagUsages. + if len(strict.MigrationInventory) != len(strict.Usages) { + t.Fatalf("MigrationInventory has %d item(s), want %d (one per usage): %+v", len(strict.MigrationInventory), len(strict.Usages), strict.MigrationInventory) + } + for _, item := range strict.MigrationInventory { + if item.CallExpression == "" { + t.Errorf("item %+v has no CallExpression", item) + } + if item.ValueType != types.MigrationValueBoolean { + t.Errorf("item %+v ValueType = %q, want boolean", item, item.ValueType) + } + } } func TestScanStrict_positiveTransitiveFactoryWrapping(t *testing.T) { @@ -197,6 +214,15 @@ func TestScanStrict_positiveForwardingCall(t *testing.T) { t.Errorf("missing expected usage for flag key %q", k) } } + + // A forwarding-function call site (e.g. callDirect(client.BoolVariation, + // key, def)) doesn't directly show the LD method's own (key, context, + // fallback) arguments the way a migrate rewrite would need — deliberately + // excluded from MigrationInventory (see its doc comment, types.go), so + // none of these 3 strict-types-only usages should have produced one. + if len(strict.MigrationInventory) != 0 { + t.Errorf("MigrationInventory = %+v, want empty — forwarding-call usages have no migration item", strict.MigrationInventory) + } } func TestScanStrict_positiveFlagDescriptorChain(t *testing.T) { diff --git a/internal/types/types.go b/internal/types/types.go index 62d621f..56bbf68 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -80,6 +80,64 @@ type ScanWarning struct { Reason string `json:"reason,omitempty"` // human-readable cause — only set for "typecheck-failure" } +// MigrationValueType is the OpenFeature evaluation-method category a call's +// return value maps to — which getXxxValue method a future migrate command +// would rewrite the call to use. Unlike flaglint-js (whose generic +// variation()/isFeatureEnabled() calls require inferring this from the +// fallback argument's literal type), every Go SDK method name is already +// type-specific, so this is always derived directly from CallType, never +// from a fallback expression. +type MigrationValueType string + +const ( + MigrationValueBoolean MigrationValueType = "boolean" + MigrationValueString MigrationValueType = "string" + MigrationValueNumber MigrationValueType = "number" + MigrationValueObject MigrationValueType = "object" + MigrationValueUnknown MigrationValueType = "unknown" +) + +// MigrationManualReviewReason is why a call site isn't safely automatable by +// a future migrate command. Matches flaglint-js's set exactly (see ADR 003); +// "unknown-fallback" is never produced by flaglint-go itself (Go's method +// names always determine ValueType outright, see MigrationValueType) but is +// kept for cross-tool consistency — the same reason vocabulary either tool's +// migrationInventory can carry. +type MigrationManualReviewReason string + +const ( + MigrationReasonDynamicKey MigrationManualReviewReason = "dynamic-key" + MigrationReasonUnknownFallback MigrationManualReviewReason = "unknown-fallback" + MigrationReasonDetailMethod MigrationManualReviewReason = "detail-method" + MigrationReasonBulkInventoryCall MigrationManualReviewReason = "bulk-inventory-call" +) + +// MigrationInventoryItem is a richer, migration-focused record of one call +// site than FlagUsage — the additional detail a future `migrate` command +// would need to safely rewrite the call, not just report it. Mirrors +// flaglint-js's MigrationInventoryItem field-for-field (see ADR 003), with +// one deliberate omission: flaglint-js's `isAwaited?: boolean` has no Go +// equivalent (no async/await) and is never emitted. +type MigrationInventoryItem struct { + File string `json:"file"` + Line int `json:"line"` + Column int `json:"column"` + LaunchDarklyMethod CallType `json:"launchDarklyMethod"` + CallExpression string `json:"callExpression,omitempty"` + RangeStart int `json:"rangeStart,omitempty"` + RangeEnd int `json:"rangeEnd,omitempty"` + FlagKeyExpression string `json:"flagKeyExpression,omitempty"` + // StaticFlagKey is only set when !IsDynamic — matches flaglint-js's + // `staticFlagKey?: string`. + StaticFlagKey string `json:"staticFlagKey,omitempty"` + IsDynamic bool `json:"isDynamic"` + ValueType MigrationValueType `json:"valueType"` + FallbackExpression string `json:"fallbackExpression,omitempty"` + EvaluationContextExpression string `json:"evaluationContextExpression,omitempty"` + SafelyAutomatable bool `json:"safelyAutomatable"` + ManualReviewReason MigrationManualReviewReason `json:"manualReviewReason,omitempty"` +} + // ScanResult is the top-level output of a scan, shared by audit/scan/validate. type ScanResult struct { ScannedAt string `json:"scannedAt"` @@ -90,4 +148,13 @@ type ScanResult struct { Usages []FlagUsage `json:"usages"` ScanDurationMs int64 `json:"scanDurationMs"` Warnings []ScanWarning `json:"warnings"` + // MigrationInventory carries a MigrationInventoryItem for every Phase 1 + // (pure-syntax) usage — see docs/adr/003-cross-tool-contract.md. A + // --strict-types-only usage (FlagUsage.DetectedBy == "strict-types", + // e.g. a forwarding-function call) is deliberately excluded: its call + // site doesn't directly show the LD method's own (key, context, + // fallback) arguments the way a migrate rewrite would need, so a + // MigrationInventoryItem for it would misrepresent what's actually safe + // to rewrite. + MigrationInventory []MigrationInventoryItem `json:"migrationInventory"` }