feat(scanner): implement migrationInventory for cross-tool parity#38
feat(scanner): implement migrationInventory for cross-tool parity#38flaglint wants to merge 1 commit into
Conversation
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 <krishansharma0327@gmail.com>
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Summary
Closes #37 — implements flaglint-js's
migrationInventoryfield for real cross-tool parity, ahead of the spec's own "optional until v1.2" deadline for other implementations.types.MigrationInventoryItemmirrors flaglint-js's shape field-for-field (see docs/adr/003-cross-tool-contract.md for the full mapping) — one deliberate omission: flaglint-js'sisAwaitedhas no Go equivalent (no async/await).valueTypeis always derived directly from the Go SDK method name (every method is already type-specific, unlike flaglint-js's genericvariation()), so"unknown-fallback"is never actually produced by flaglint-go itself.manualReviewReason/safelyAutomatableotherwise mirror flaglint-js's logic exactly: bulk-inventory-call, dynamic-key, detail-method, else safely automatable.detect()call site that already builds eachFlagUsage—parsedFilenow retains each file's raw source bytes (previously discarded after parsing) sobuildMigrationInventoryItemcan extract exact expression text via byte offsets.--strict-typesusages get full migration items (their call sites are ordinaryclient.Method(key, ctx, fallback)shapes). Forwarding-function usages deliberately get none — their call site doesn't directly show the LD method's own arguments, so an item would misrepresent what's actually rewritable.Also fixes a latent pre-existing bug found while verifying this: the "empty arrays must not be null" tests checked for
"field":null(no space) butjson.MarshalIndentemits"field": null(with a space) — vacuously true regardless of actual values. Fixed to match real formatting.Verification
*Ctx-variant argument shifting, plus assertions on the existing interface-satisfaction and forwarding-call--strict-typestests proving the inclusion/exclusion split.