feat(scanner): --strict-types Phase 2a — interface satisfaction, closing #15#28
Merged
Merged
Conversation
…ing #15 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 <krishansharma0327@gmail.com>
|
Warning Review limit reached
Next review available in: 54 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 ignored due to path filters (1)
📒 Files selected for processing (18)
✨ 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 |
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 <krishansharma0327@gmail.com>
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 <krishansharma0327@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements ADR 005's Phase 2a: an opt-in
--strict-typespass that loads the target module with realgo/typesinformation and proves client identity through an interface — something Phase 1's syntactic tracing structurally cannot see (var evaluator FlagEvaluator = client). Closes #15.What changed
internal/typecheck(new): a small, LaunchDarkly-agnostic wrapper aroundgolang.org/x/tools/go/packages.Load(dir)loads every package with full type info, excluding (never failing on) anything that doesn't type-check cleanly — transitively too, viaIllTyped— with a human-readable reason per exclusion.resolveAssignedBinding(identity.go) gained a static-type fallback: when the syntax-only checks don't match and real type info is available, check the expression's actual static type against the SDK's*LDClientdirectly. Reusing this one function means every existing Phase 1 mechanism (block-scoped binding tracking, method values, the works) gains type-backed coverage uniformly.scanner.ScanStrict(strict.go): runsScan(Phase 1, unchanged) first, then re-runs the whole-scan analysis over go/packages-loaded ASTs (which carry type info Phase 1's own ASTs never do), merging in only fingerprints Phase 1 didn't already find — enforced by explicit dedup, not assumed. Every merged-in finding is taggeddetectedBy: "strict-types".--strict-typeswired as a CLI flag (not a config-file field — Go-only, no flaglint-js equivalent) onscan/audit/validatevia one sharedrunScanhelper.A real bug found during verification
Running
--strict-typesagainst a real 4500+-file repo (weaviate) initially produced 321typecheck-failurewarnings. Root cause: flaglint-go's owngo.moddeclares 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 howgo/typesrepresents Go 1.24+ generic type aliases, since type-checking runs in-process. Fixed by forcingGODEBUG=gotypesalias=1viaos.Setenvbefore loading (packages.Config.Envalone doesn't reach it — that only affects subprocessesLoadspawns for discovery, not this process's own in-process type-checking), unless the caller already set one explicitly. Verified: 321 warnings before, 0 after, same repo.Verification
testdata/strict/interface_satisfaction, with a local-replace stub SDK — no network access or the real SDK dependency needed in CI) proves the interface-satisfaction repro is detected byScanStrictand invisible to plainScan, plus a false-positive guard (unrelated type satisfying the same interface shape must not be detected).go.modfalls back to Phase 1's result plus a warning, never a hard failure.