diff --git a/docs/adr/005-strict-types-pass.md b/docs/adr/005-strict-types-pass.md new file mode 100644 index 0000000..f1f5080 --- /dev/null +++ b/docs/adr/005-strict-types-pass.md @@ -0,0 +1,171 @@ +# ADR 005 — `--strict-types`: Type-Checked Identity Resolution (Phase 2) + +Date: 2026-07 +Status: Accepted + +## Decision + +Add an opt-in `--strict-types` pass that loads the target module with real +`go/types` information (via `golang.org/x/tools/go/packages`) and uses it +to prove client identity in cases Phase 1's syntactic tracing structurally +cannot — interface satisfaction and transitive factory wrapping. This pass +is strictly *additive*: it can only add findings Phase 1 missed, never +remove or contradict one Phase 1 already made. Phase 1 remains the +default; `flaglint-go audit` continues to work unconditionally, with no +build required. + +This ADR splits what was originally described in +[ADR 002](002-client-identity-model.md) as one "Phase 2" into two +sub-phases with different footprints: + +- **Phase 2a** (this ADR's actual scope): static type resolution. Closes + issues [#15](https://github.com/flaglint/flaglint-go/issues/15) + (interface satisfaction) and + [#16](https://github.com/flaglint/flaglint-go/issues/16) (transitive + factory wrapping). Both are pure "what is this expression's real type" + questions — `go/types` answers them directly, no data-flow analysis + needed. +- **Phase 2b** (explicitly deferred, tracked as its own future ADR): + interprocedural data-flow. Closes + [#26](https://github.com/flaglint/flaglint-go/issues/26) (a method + value captured in one function, passed as an argument, and invoked from + inside a *different* function). `go/types` alone gives you a + parameter's *declared type*, not proof that a specific call site passed + a specific real client's method into it — that requires tracing data + flow across a call graph (`go/ssa`/`go/callgraph` or a bespoke + lightweight tracer), a meaningfully larger and riskier undertaking than + Phase 2a. Building it prematurely, bundled with Phase 2a, would risk + either delaying real, contained value (#15/#16) or under-baking the + harder problem. They ship separately. + +## Context + +Phase 1 (ADR 002) and its whole-scan extension (ADR 004) prove identity +entirely from syntax — import tracing, constructor-call binding, struct +fields, composite literals, factory functions, parameter types. This +works without a build and is fast, but it has a real ceiling: some +patterns are not expressible without knowing an expression's *actual* +static type, which only a type checker can answer. + +- **Interface satisfaction** (#15): a client passed around only through + an interface type it happens to satisfy. Phase 1 sees the interface + type name, not the concrete `*ld.LDClient` underneath it — there is no + syntactic proof available at all. +- **Transitive factory wrapping** (#16): a factory function returning a + *wrapper* type (not `*ld.LDClient` directly) that itself has a client + field, resolved through composite-literal/struct-field bindings + elsewhere. Phase 1's factory registration (`returnsSDKClient`, + factory.go) only recognizes a return type that is *literally* + `*ld.LDClient` — one hop, by design (see ADR 004) — deliberately not + chasing arbitrary wrapper chains without real type information to keep + that mechanism provably safe. + +Both are genuine, plausible real-world patterns, not contrived edge +cases — the same field-testing discipline that found #5/#17/#18/#20/#6 +this project has followed throughout would very plausibly turn up live +examples of these too, the way it did for everything fixed so far. + +## Package Loading + +`--strict-types` loads the scanned directory as a Go module via +`golang.org/x/tools/go/packages` (`packages.Load` with `NeedTypes`, +`NeedTypesInfo`, `NeedSyntax`, `NeedImports` at minimum). This is a +materially different operating mode from Phase 1: + +- It requires the module to actually type-check. Dependency resolution + may need network access (or a warm module cache/vendor directory). +- It is meaningfully slower — type-checking a full dependency graph is a + different order of magnitude of work than parsing `.go` files in + isolation. Expect single-digit-to-tens of seconds on a large repo with + a cold module cache, versus Phase 1's low seconds. +- It can fail — a mid-refactor branch, a partial checkout, or a repo with + broken dependencies won't type-check. This is precisely the situation + ADR 002 designed Phase 1 to keep working through; `--strict-types` does + not get to break that guarantee for the *rest* of the tool. + +**Failure mode: fail soft, per-package.** `packages.Load` reports errors +per-package, not just globally. A package that fails to load or +type-check is skipped for Phase 2a purposes — its Phase 1 findings stand +unchanged, and a warning is emitted (mirroring the existing +`ScanWarning` mechanism) naming the package and the reason. The scan +completes with whatever additional findings *were* provable, rather than +failing outright because one package elsewhere in a large repo doesn't +build. `validate`'s exit-code contract is unaffected: a `--strict-types` +package-load failure is a warning, not a policy violation, and never +promotes a `scan`/`audit` invocation to a nonzero exit on its own. + +## Identity Resolution Model + +Phase 2a's rule mirrors Phase 1's non-negotiable one (ADR 002): still +never inferred from a name, only now proven through the type checker +instead of syntax. Concretely, for each expression Phase 1 could not +already resolve: + +- **Interface satisfaction**: `types.Info.TypeOf(expr)` on the receiver; + if its underlying concrete type is exactly the SDK's `*LDClient` (or, + for an interface-typed variable, if `go/types` can determine the + *only* concrete type ever assigned to it through the package is + `*LDClient` — the general "any possible concrete type" case is + undecidable in general and out of scope; only the syntactically obvious + single-assignment case is handled), the receiver is bound. +- **Transitive factory wrapping**: for a function whose declared return + type is *not* literally `*ld.LDClient` (Phase 1's factory boundary), + walk the returned type's fields via `go/types` structurally — if + exactly one field (at any depth) resolves to `*ld.LDClient`, the + factory is registered for that field path, extending Phase 1's + one-hop-only factory model to arbitrary depth now that a real type + graph is available to walk safely. + +As in Phase 1, an unresolvable or ambiguous case is left unbound rather +than guessed — false negative over false positive remains non-negotiable +here too; `go/types` raises the ceiling of what can be *proven*, it does +not loosen the bar for what counts as proof. + +## CLI and API Surface + +`--strict-types` is a boolean flag on `scan`, `audit`, and `validate` — +not a `flaglint.config.json`/`.flaglintrc` field, unlike everything in +`internal/config.Config`. That struct exists specifically to mirror +flaglint-js's config schema for cross-tool parity (ADR 003); `go/types` +loading is a Go-only concept with no JS equivalent, so it doesn't belong +there. + +At the API level, `scanner.Scan` (Phase 1, exported today) keeps its +exact current signature and behavior, unconditionally available with no +new dependency implications for callers that don't ask for Phase 2. A new +`scanner.ScanStrict(dir string, cfg config.Config) (types.ScanResult, +error)` runs Phase 1 first, then augments the result with Phase 2a +findings before returning — CLI commands call `ScanStrict` instead of +`Scan` only when `--strict-types` is passed. `types.ScanResult` gains +enough to distinguish a Phase 2a finding from a Phase 1 one (for +reporting/debugging), following the existing additive-fields discipline +already used for the JSON output contract (ADR 003). + +## Consequences + +- `golang.org/x/tools` becomes a real dependency (already what `gopls` + and most Go tooling in this space is built on — a reasonable, + well-established choice, not a niche one). +- Phase 1 remains fully independent of this pass at the call level; + `flaglint-go audit` without `--strict-types` has zero behavior change + and no new failure modes. +- CI/release tooling is unaffected — `--strict-types` is exercised by its + own test fixtures (real, buildable Go modules under `testdata/`, since + type-checking needs real buildable code, unlike Phase 1's testdata + which is deliberately never built) but doesn't change how the rest of + the suite runs. +- Phase 2b (interprocedural, closing #26) is explicitly not part of this + ADR's scope — a follow-up ADR should make its own case for the + data-flow approach once Phase 2a has shipped and proven out the package + loading and fallback machinery in practice. + +## What This ADR Does Not Decide + +- Phase 2b's design (data-flow/call-graph approach for #26) — deferred to + its own future ADR, as above. +- Whether `--strict-types` results should be cacheable across runs (a + real performance question once package loading cost is measured against + actual repos, not decided speculatively here). +- Whether `--strict-types` should ever become the default — no; Phase 1's + no-build guarantee is a permanent product commitment (ADR 002), not a + temporary bootstrapping state. diff --git a/docs/adr/README.md b/docs/adr/README.md index 68b0366..3a6375c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -9,3 +9,4 @@ Never delete or edit a past ADR. Add a new one to supersede it. | [002](002-client-identity-model.md) | Client Identity Model | | [003](003-cross-tool-contract.md) | Cross-Tool Contract with flaglint-js | | [004](004-whole-scan-identity-resolution.md) | Whole-Scan Identity Resolution (Phase 1.5) | +| [005](005-strict-types-pass.md) | `--strict-types`: Type-Checked Identity Resolution (Phase 2) |