feat: control-flow graph foundation for conditional analysis#64
Open
antst wants to merge 10 commits into
Open
Conversation
Elevate the per-function CFG (built via golang.org/x/tools/go/cfg, previously used only to annotate branches) into a retained, queryable reachability + dominance model, and migrate the conditional-analysis consumers onto it instead of the former source-text-position heuristic. - Conditional status codes (#39 / #50 / #57): a status contributes iff its assignment can reach the response write and is not overwritten on every path by a later, call-dominating assignment. if/else and switch arms fan out; an early return before the write is excluded; an unconditional overwrite shadows earlier assignments; a value assigned in a loop body reaches a write after the loop (the analysis terminates across the back-edge). The positionAfter / positionLineCol / lastUncond heuristic is removed. - Helper-internal type-switch binding: when a handler funnels a value into a shared responder that type-switches on it, the call-site argument is bound to the matching arm (structural type identity: pointer/slice/map/generic-aware). When the concrete type is not statically known, or the arms are not cleanly captured (nested control flow), it degrades to the unconditionally-reachable result + warning, or safely over-approximates -- never mis-binding. - Method dispatch via `if r.Method == http.MethodPost` now splits into one operation per method, the same as a `switch r.Method`. - Branch-dependent response bodies are attributed to the status on which they are written, never merged. New: internal/metadata/{dominators,reachability}.go (Cooper-Harvey-Kennedy dominators + reachability query layer), internal/spec/warnings.go (FR-008/FR-012 warning sink). The existing framework golden corpus and the determinism suite stay byte-identical; each behavior ships with its own fixture (cfg_helper_typeswitch, cfg_loop_status, cfg_method_if_dispatch, cfg_branch_bodies). Packages stay >=95% statement coverage.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds retained per-function control-flow graphs, dominator and reachability queries, CFG-backed metadata annotation, CFG-driven conditional status extraction, warning sinks, additional tests, new e2e fixtures, and changelog notes. ChangesControl-Flow Graph Foundation
Sequence Diagram(s)sequenceDiagram
participant GenerateMetadataWithLogger
participant BuildFunctionCFGs
participant Metadata
participant ResponsePatternMatcherImpl
participant WarningSink
GenerateMetadataWithLogger->>BuildFunctionCFGs: funcDecls + declInfo
BuildFunctionCFGs->>Metadata: store FunctionCFGs and cfgPosToFn
ResponsePatternMatcherImpl->>Metadata: FnKeyForPos / BlockFor / Reaches / Dominates
Metadata-->>ResponsePatternMatcherImpl: CFG query results
ResponsePatternMatcherImpl->>WarningSink: Warn(pos, msg) on degraded analysis
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/spec/extractor.go (1)
965-999: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve shared responses when splitting by method.
This only copies responses that carry method
CaseValuesinto each split route. Any unconditional or otherwise shared response on the original route disappears once two methods are detected, so a handler with GET/POST-specific 2xx responses plus a common 400/500 will emit per-method operations that silently lose the shared statuses. Each split route should include the common responses in addition to its method-specific ones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/spec/extractor.go` around lines 965 - 999, The method-splitting logic in the route extraction path drops shared responses when building each per-method RouteInfo, so common statuses vanish once split routes are emitted. Update the response partitioning around the methodResponses construction and the RouteInfo assembly so each split route keeps the shared/unconditional responses from the original route in addition to the method-specific ones; use the existing route.Response, methodResponses, and RouteInfo creation flow to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/metadata/cfg.go`:
- Around line 231-237: `extractMethodGuard` is too permissive and can
misclassify non-HTTP business logic as route method dispatch. Update the branch
tagging in `annotateBranches`/`extractMethodGuard` so it only sets `CaseValues`
when the left side is resolved via `*types.Info` to `net/http.Request.Method`,
and only treat selector-based method constants as valid when they come from the
`net/http` package. Add regression coverage for false positives like `s.Method
== "GET"` and `foo.MethodPost == r.Method` to ensure `splitByConditionalMethods`
only fans out real HTTP method guards.
In `@internal/metadata/dominators.go`:
- Around line 132-145: In blockDominates, validate the current block index
before checking reflexive equality so invalid indices cannot return true. Update
the loop in blockDominates(idom, a, b) to reject out-of-range x values first,
then handle x == a only after bounds are confirmed, preserving the out-of-range
=> false contract and preventing stale BlockLoc false positives.
In `@internal/metadata/reachability.go`:
- Around line 91-99: The Reaches fast path in Metadata should not short-circuit
solely on from.Block == to.Block; it must first verify that the block exists in
the CFG before comparing node indices. Update Reaches to guard the same-block
branch with the same bounds/existence checks used for normal reachability,
likely via the fnCFG result and the block lookup helpers around blockReaches, so
invalid blocks like a missing Block 9 correctly return false instead of true.
In `@internal/spec/extractor.go`:
- Around line 1605-1615: In the arm-selection logic in extractor.go, the
imprecise binding path currently filters all typed arms even when arm.dflt is
empty, which can drop every response for helper type-switches. Update the branch
around the arm.dflt and arm.typed handling so that when !precise and there is no
default arm, you keep the typed arms unfiltered and still emit the existing
warning; use the surrounding arm, ae.kept, ae.filtered, and e.warn logic to
locate the change.
- Around line 1384-1402: classifyHelperWrites only checks direct children, so
nested response writes under intermediate calls are missed and fallback writes
can leak into extracted responses. Update Extractor.classifyHelperWrites to
recurse through descendant nodes (while still using matchesAnyResponse and
edge/branch checks) so it can detect helper writes like
json.NewEncoder(w).Encode(...) before deciding whether hasUnconditional() should
block an if err != nil fallback. Keep the existing unconditional vs conditional
classification rules in helperWrites, but apply them across the full subtree
rooted at the node.
---
Outside diff comments:
In `@internal/spec/extractor.go`:
- Around line 965-999: The method-splitting logic in the route extraction path
drops shared responses when building each per-method RouteInfo, so common
statuses vanish once split routes are emitted. Update the response partitioning
around the methodResponses construction and the RouteInfo assembly so each split
route keeps the shared/unconditional responses from the original route in
addition to the method-specific ones; use the existing route.Response,
methodResponses, and RouteInfo creation flow to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4c77a072-9606-459b-9e62-742e8099ac95
⛔ Files ignored due to path filters (16)
testdata/cfg_branch_bodies/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_branch_bodies/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_branch_bodies/go.modis excluded by!testdata/**testdata/cfg_branch_bodies/main.gois excluded by!testdata/**testdata/cfg_helper_typeswitch/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_helper_typeswitch/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_helper_typeswitch/go.modis excluded by!testdata/**testdata/cfg_helper_typeswitch/main.gois excluded by!testdata/**testdata/cfg_loop_status/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_loop_status/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_loop_status/go.modis excluded by!testdata/**testdata/cfg_loop_status/main.gois excluded by!testdata/**testdata/cfg_method_if_dispatch/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_if_dispatch/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_if_dispatch/go.modis excluded by!testdata/**testdata/cfg_method_if_dispatch/main.gois excluded by!testdata/**
📒 Files selected for processing (18)
CHANGELOG.mdinternal/engine/engine_e2e_test.gointernal/metadata/cfg.gointernal/metadata/cfg_test.gointernal/metadata/dominators.gointernal/metadata/expression_coverage_test.gointernal/metadata/metadata.gointernal/metadata/reachability.gointernal/metadata/reachability_test.gointernal/metadata/types.gointernal/spec/branch_bodies_test.gointernal/spec/conditional_status_test.gointernal/spec/extractor.gointernal/spec/extractor_additional_test.gointernal/spec/pattern_matchers.gointernal/spec/typeswitch_binding_test.gointernal/spec/warnings.gointernal/spec/warnings_test.go
Comment on lines
+1384
to
+1402
| func (e *Extractor) classifyHelperWrites(node TrackerNodeInterface) helperWrites { | ||
| var hw helperWrites | ||
| for _, child := range node.GetChildren() { | ||
| if child.GetEdge() == nil || !e.matchesAnyResponse(child) { | ||
| continue | ||
| } | ||
| br := child.GetEdge().Branch | ||
| switch { | ||
| case br == nil: | ||
| hw.unconditional = append(hw.unconditional, child) | ||
| case br.BlockKind == "if-then" || br.BlockKind == "if-else": | ||
| // Only an if-guarded write is a #27-style defensive fallback. A | ||
| // switch-case/select-case write is a type-switch (or method) arm owned by | ||
| // the type-switch-binding pass, so it must NOT be filtered here as a | ||
| // fallback — doing so would drop a precisely-bound arm. | ||
| hw.conditional = append(hw.conditional, child) | ||
| } | ||
| } | ||
| return hw |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recurse when classifying helper writes.
classifyHelperWrites only inspects direct children. That misses the common shape where the helper’s primary response is nested under an intermediate call (for example json.NewEncoder(w).Encode(...)). In that case hasUnconditional() stays false, so an if err != nil fallback write in the same helper is never filtered and leaks into the caller’s extracted responses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/spec/extractor.go` around lines 1384 - 1402, classifyHelperWrites
only checks direct children, so nested response writes under intermediate calls
are missed and fallback writes can leak into extracted responses. Update
Extractor.classifyHelperWrites to recurse through descendant nodes (while still
using matchesAnyResponse and edge/branch checks) so it can detect helper writes
like json.NewEncoder(w).Encode(...) before deciding whether hasUnconditional()
should block an if err != nil fallback. Keep the existing unconditional vs
conditional classification rules in helperWrites, but apply them across the full
subtree rooted at the node.
Triaged the six CodeRabbit findings on the CFG-foundation PR — five fixed, one evaluated and rejected as a regression. Fixed: - extractMethodGuard: gate on type info so an `if <x>.Method == M` is only treated as method dispatch when `<x>.Method` resolves to net/http.Request.Method (via types.Unalias, so `type Req = http.Request` aliases are recognized) and a MethodXxx constant resolves from net/http. Prevents unrelated business logic (`s.Method == "GET"`, `foo.MethodPost == r.Method`) from fabricating route splits. - splitByConditionalMethods: carry UNCONDITIONAL responses (a common 400/500 written regardless of method) onto every split operation. A `switch r.Method` default / bare `else` arm is the "other methods" negation and is excluded. - applyTypeSwitchBinding: when the binding is imprecise AND the helper has no default arm, keep all typed arms + warn rather than emit zero responses. - blockDominates / Reaches: bounds-check block indices before the reflexive / same-block fast paths so an out-of-range BlockLoc degrades to false. Rejected (kept as-is, with a rationale comment): - "Recurse classifyHelperWrites to catch nested writes": evaluated and reverted. Recursing makes the #27 fallback filter treat a sub-handler's nested success write (json.NewEncoder(w).Encode) as the helper's unconditional primary and then drop the sub-handler's real `if err { http.Error(4xx) }` response (regression on enum_validation: POST loses its 400). Direct-children scanning is correct for both the #27 helper and the sub-handler shapes. Also documents a known limitation: a method-dispatch FALLBACK arm (switch default or bare `else`) is not emitted as its own operation (no single HTTP method to attach to) — consistent for switch and if. Golden-neutral; >=95% coverage.
The full review of #64 (c02ec4e) found a reproduced regression: when an `if r.Method ==` dispatch (new in 009) is combined with an independent conditional response (e.g. a pre-dispatch `if bad { 500 }`), splitting the handler per method DROPPED that response from every operation — whereas main, which does not split the if-form, kept it. Root cause: splitByConditionalMethods classified a response as shared only when Branch == nil, so a non-method conditional landed in neither the per-method buckets nor `shared`. Replace the Branch==nil-only rule with a per-function CFG classifier (spec 009) that places each non-method conditional via dominance/reachability: - nested inside a method arm (an arm block dominates it) -> that method only; a combined `case "GET", "HEAD":` arm serves every method it names - dispatch fallback -> excluded, so a 405 never leaks onto a handled method. A `switch` `default:` is recognised structurally (empty case values) so a stray `fallthrough` into it cannot leak; an `if r.Method ==` bare `else` is recognised as descending from the dispatch root while sharing no control-flow path with any arm - independent (outside the dispatch, or reachable together with the arms) -> carried onto every method operation dispatchRoot is the common dominator (LCA) of the method arm blocks, robust to go/cfg's switch lowering (the arms' immediate dominators differ along the test chain). Adds metadata.IDom plus parent-statement position registration in cfgPosToFn (transient) so the spec layer can resolve a branch back to its function and query dominance. dispatchArms maps a block to ALL the methods it serves (sorted) so a nested response in a combined case is attributed deterministically. With no CFG model the classifier conservatively excludes non-method conditionals (pre-009 behavior). Golden-neutral except the intended fix: across all testdata the only output change vs c02ec4e is the new cfg_method_if_independent fixture (the independent 500 now appears on both methods). response_patterns is unchanged (the 405 default stays excluded). Promotes enum_validation to an e2e golden as a regression guard for the #27/#5 sub-handler 4xx-kept case (POST /users/ -> [200,204,400]).
extractCaseValues read only string-literal case expressions, so a
`switch r.Method { case http.MethodGet: … }` (net/http method CONSTANTS)
produced empty CaseValues and never split into per-method operations — while
the equivalent `if r.Method == http.MethodGet` guard did (extractMethodGuard
already resolves the constants). The switch and if forms are meant to behave
identically (FR-003); the asymmetry was flagged across the #64 reviews.
Resolve selector-constant cases through the same httpMethodName helper the
if-form uses (type-gated to net/http), additively — string literals are still
returned verbatim. The new cfg_method_switch_dispatch fixture is the
switch-form mirror of cfg_method_if_dispatch and now splits identically
(GET 200 / POST 201).
Golden-neutral for existing fixtures (only response_patterns uses switch
method dispatch, and with string literals — unaffected). enum_validation is
unchanged: its switch dispatches to sub-handler METHODS whose responses are
interprocedural, so the method context does not reach them — a deeper,
separate limitation (noted in the e2e table) that constant resolution alone
does not address.
…lden The full-review gate on the method-split classifier found two issues: 1. Cross-function block-index leak (regression). A non-method response inferred from a HELPER carries that helper's CFG-local branch block index; the classifier queried it against the HANDLER's CFG, where it aliases an unrelated block. A handler that splits per method AND calls a helper writing a conditional response leaked that response onto the wrong method (verified: a GET-only helper 599 appeared on POST). Fix: the classifier reasons only about branches in its own function — a response whose ParentStmtPos resolves to a different fnKey is conservatively excluded (the pre-CFG behavior). Correct per-method attribution of an interprocedural conditional needs call-site context, a separate concern. Guarded by cfg_method_helper_response + TestSplit_ForeignBranchExcluded. Handler-local conditionals (the F1 case) are unaffected. 2. enum_validation enshrined wrong output. Promoting it to an e2e golden baked in a phantom single-`post` operation for paths the source serves as GET/PUT/DELETE (its switch dispatches to sub-handler METHODS, an unsolved interprocedural case) — violating the golden rule (verify output against source). Dropped from the e2e table; its golden files removed. The #5-revert stays guarded at the unit level (TestClassifyHelperWrites_Partition); a clean e2e #5 guard needs the interprocedural fix first. Also adds cfg_method_switch_fallthrough — a real e2e guard that a `fallthrough` into a `switch r.Method` default does not leak the 405 onto the handled methods (previously only a synthetic-CFG unit test covered it).
The full-review re-gate found a regression: in a `validate-then-respond` method
arm (`case GET: if bad{404;return}; 200`), go/cfg drops the success (200) into a
merge block that loses GET's method context, so GET drops out of the per-method
responses. With GET gone, the dispatch root was computed over only the surviving
arms and no longer dominated the GET region — so the 404 orphaned there was
misclassified as an independent conditional and LEAKED onto the other methods.
Record every method-dispatch arm block (a `switch r.Method` case or `if r.Method ==`
then-block, INCLUDING arms that produced no response) on the per-function CFG
(FunctionCFG.MethodArms, transient) and expose them via metadata.MethodArms. The
classifier's dispatch root is the common dominator of the responding arms PLUS any
recorded arm that is a SIBLING of them (mutually exclusive — same switch/if-chain),
which recovers a dropped arm while NOT pulling in arms of a separate dispatch (a
second `switch r.Method`, or a value-switch with method-named cases): those run in
sequence with the responding arms, so including them would inflate the root and
over-exclude independent conditionals between the dispatches.
The scoped root is the true switch/if tag, which dominates the dropped GET region —
so the orphaned 404 is correctly excluded (== c02, no leak), while the pre-dispatch
F1 independent (not dominated by the tag) and an independent between two dispatches
still share. Guarded by TestSplit_DroppedArmConditionalExcluded,
TestDispatchRootBlocks_SiblingScoping, and cfg_method_two_dispatch.
The deeper limitation (the GET op itself vanishing for validate-then-respond arms)
is unchanged — it needs the nested-return context survival on the reaching-defs /
interprocedural roadmap; this commit only stops the leak.
splitRouteFnKey returned the first fnKey from an unordered map iteration over the method responses. When a handler's method dispatch spans more than one function (e.g. a sub-dispatch in a helper), the method branches resolve to different fnKeys, so the choice — and, via the cross-function guard, whether an independent response is shared or excluded — depended on Go's randomized map order, flapping the emitted spec run-to-run (reproduced: 2 distinct output hashes over 8 runs of one handler). Count the function each method branch resolves to and return the most common, with a lexicographic tiebreak — deterministic regardless of map order. Single-function handlers (the overwhelming common case) are unchanged. Guarded by TestSplitRouteFnKey_MultiFunction.
Replace the post-hoc reconstruction of method-dispatch grouping in
splitByConditionalMethods with identity recorded at CFG-annotation time.
Root cause: the classifier reconstructed "which arms form one dispatch"
after the fact — from method-named CaseValues (MethodArms) plus a
sibling-scoping mutual-exclusivity proxy (dispatchRootBlocks). That shallow
model could not distinguish three shapes, each of which produced a regression:
- a combined `case "GET","POST":` + `default:` — go/cfg lowers the combined
case to ONE block dominated by itself, so the default was not seen as
dominated by the reconstructed root and its 405 leaked onto GET and POST;
- two dispatches in mutually-exclusive branches — sibling-scoping pulled the
other branch's arms into the root, inflating it and over-excluding an
independent conditional in one branch;
- a value-switch whose cases merely NAME methods — recorded as method arms
because detection keyed on CaseValues, not the switch tag.
Fix: a methodDispatchGroups AST pre-pass tags every arm of one dispatch (each
`switch r.Method` case INCLUDING the default, or each `if r.Method ==` chain
arm) with a group id — the dispatch statement's position — gated on the switch
TAG type-resolving to net/http.Request.Method. annotateBranches records
BranchContext.DispatchGroup and FunctionCFG.DispatchArms[group]. The classifier
selects the primary dispatch (primaryDispatch: the (fn, group) the most method
responses belong to, deterministic) and scopes the dispatch root to exactly
that group's arms (metadata.DispatchArms) — the exact switch/if tag, never
inflated by a separate dispatch, never collapsed by a combined case.
Removes MethodArms, splitRouteFnKey, dispatchRootBlocks and their
mutual-exclusivity-proxy limitation.
New fixture cfg_method_combined_default locks in the combined-case+default fix
(GET[200], POST[200]; the default 405 stays the dispatch fallback). No existing
golden changed — output is byte-identical across the whole corpus apart from
the added fixture.
Coverage: metadata 98.0%, spec 96.0%. Full suite green.
Spec: 009-cfg-foundation (US2/FR-003).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/spec/extractor.go (1)
1049-1079: 🎯 Functional Correctness | 🟡 Minor
resultslice order is nondeterministic due to map iteration.The
splitByConditionalMethodsfunction constructs the returned[]*RouteInfoslice by ranging over themethodResponsesmap (lines 1050–1079). This causes random iteration order. The caller intraverseForRoutesappends these results to theroutesslice without subsequent sorting.This non-determinism can cause flakiness in tests or downstream consumers relying on stable slice ordering, contradicting the project's determinism goals. Add an explicit sort of the
resultslice (e.g., by Path and Method) before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/spec/extractor.go` around lines 1049 - 1079, The slice returned by splitByConditionalMethods is built by ranging over methodResponses, so the order of RouteInfo entries is nondeterministic. Add an explicit sort of the result slice before returning it, using stable fields such as RouteInfo.Path and RouteInfo.Method, so traverseForRoutes receives deterministic route ordering.
🧹 Nitpick comments (1)
internal/metadata/reachability.go (1)
121-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
DispatchArmsreturns the stored slice by reference.The returned slice aliases
fc.DispatchArms[group]. A consumer that sorts or appends in place could mutate the retained CFG state. If callers only read, this is benign; otherwise return a copy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/metadata/reachability.go` around lines 121 - 127, `Metadata.DispatchArms` currently returns the backing slice from `fc.DispatchArms[group]`, so callers can accidentally mutate retained CFG state by sorting or appending to it. Update `DispatchArms` to return a copy of the slice instead of the stored reference, while keeping the existing nil handling and lookup behavior through `fnCFG`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/spec/extractor.go`:
- Around line 1050-1057: The splitByConditionalMethods merge is reusing shared
*ResponseInfo pointers across method-specific routes, so later ApplyOverrides
mutations can leak between methods. Update splitByConditionalMethods to
deep-copy each ResponseInfo when building the merged responses map (not just the
map itself), and keep the method-specific override precedence intact when
combining shared and per-method entries. Use the ResponseInfo handling in
ApplyOverrides and the merged response construction in splitByConditionalMethods
as the key places to adjust.
---
Outside diff comments:
In `@internal/spec/extractor.go`:
- Around line 1049-1079: The slice returned by splitByConditionalMethods is
built by ranging over methodResponses, so the order of RouteInfo entries is
nondeterministic. Add an explicit sort of the result slice before returning it,
using stable fields such as RouteInfo.Path and RouteInfo.Method, so
traverseForRoutes receives deterministic route ordering.
---
Nitpick comments:
In `@internal/metadata/reachability.go`:
- Around line 121-127: `Metadata.DispatchArms` currently returns the backing
slice from `fc.DispatchArms[group]`, so callers can accidentally mutate retained
CFG state by sorting or appending to it. Update `DispatchArms` to return a copy
of the slice instead of the stored reference, while keeping the existing nil
handling and lookup behavior through `fnCFG`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 563b9d19-d06b-495b-a9c9-bd3578d31206
⛔ Files ignored due to path filters (24)
testdata/cfg_method_combined_default/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_combined_default/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_combined_default/go.modis excluded by!testdata/**testdata/cfg_method_combined_default/main.gois excluded by!testdata/**testdata/cfg_method_helper_response/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_helper_response/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_helper_response/go.modis excluded by!testdata/**testdata/cfg_method_helper_response/main.gois excluded by!testdata/**testdata/cfg_method_if_independent/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_if_independent/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_if_independent/go.modis excluded by!testdata/**testdata/cfg_method_if_independent/main.gois excluded by!testdata/**testdata/cfg_method_switch_dispatch/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_switch_dispatch/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_switch_dispatch/go.modis excluded by!testdata/**testdata/cfg_method_switch_dispatch/main.gois excluded by!testdata/**testdata/cfg_method_switch_fallthrough/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_switch_fallthrough/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_switch_fallthrough/go.modis excluded by!testdata/**testdata/cfg_method_switch_fallthrough/main.gois excluded by!testdata/**testdata/cfg_method_two_dispatch/expected_openapi.jsonis excluded by!testdata/**testdata/cfg_method_two_dispatch/expected_openapi_legacy.jsonis excluded by!testdata/**testdata/cfg_method_two_dispatch/go.modis excluded by!testdata/**testdata/cfg_method_two_dispatch/main.gois excluded by!testdata/**
📒 Files selected for processing (12)
internal/engine/engine_e2e_test.gointernal/metadata/cfg.gointernal/metadata/cfg_test.gointernal/metadata/dominators.gointernal/metadata/expression_coverage_test.gointernal/metadata/reachability.gointernal/metadata/reachability_test.gointernal/metadata/types.gointernal/spec/extractor.gointernal/spec/extractor_helper_test.gointernal/spec/split_classifier_test.gointernal/spec/typeswitch_binding_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/metadata/types.go
- internal/metadata/dominators.go
- internal/spec/typeswitch_binding_test.go
- internal/metadata/reachability_test.go
- internal/metadata/cfg.go
The DispatchGroupID refactor (5c473a3) regressed two method-dispatch shapes, both confirmed against the prior commit via isolated worktree builds: - responses split across an `if r.Method ==` arm AND a `switch r.Method` with a default: the root was scoped to a single "primary" group, whose lone arm does not dominate the OTHER group's `default`, so its 405 leaked onto GET and POST; - `switch m := r.Method` (a non-bare-selector tag): switch detection was gated on the tag type-resolving to net/http.Request.Method, so this common idiom recorded no dispatch group and its `default` 405 leaked. Root cause: the new dispatch-root scoping was NARROWER than the removed dispatchRootBlocks, and the switch tag-gate was inconsistent with the CaseValues-driven split decision. Fix: - methodDispatchGroups recognises a method-dispatch switch by its CASE VALUES naming HTTP methods (switchHasMethodCase) — the SAME test the route splitter uses to attribute a case to a method — independent of the tag. So `switch r.Method`, `switch m := r.Method`, and method-named value-switches are grouped exactly as they are split; string-literal cases need no type info. - the classifier scopes dispatchRoot to the common dominator of the arms of EVERY dispatch group that CONTRIBUTED a method response (contributingDispatchArms), not a single primaryGroup — so a split across two dispatches spans both (the second dispatch's default stays dominated and excluded), while a dispatch that wrote no response is left out (an independent conditional sequenced between two dispatches is not over-excluded). Also addresses the CodeRabbit review on the push (review 4572374357): - splitByConditionalMethods emits routes in deterministic METHOD order and gives each split route its OWN copy of every ResponseInfo, so a per-route ApplyOverrides (BodyType/BodyTypeRef mutated in place) cannot leak an override across methods; - documents DispatchArms' returned slice as read-only. Fixtures cfg_method_if_switch_default and cfg_method_switch_copy lock in the two fixes. No existing golden changed. Coverage metadata 98.0%, spec 96.0%; full suite green (incl. -race + determinism). Spec: 009-cfg-foundation (US2/FR-003).
Addresses the local /code-review gate findings on the dispatch-root fix (a0f175d) — all comment/test only, no behavior change: - cfg.go: the arm-recording comment claimed a value-switch with method-named cases is "correctly NOT recorded" (true under the removed tag-gate, false now that switchHasMethodCase detects by case values) — corrected to match. - extractor.go: contributingDispatchArms' doc over-promised that an independent conditional between two dispatches is never over-excluded; now documents the real limitation (two responding dispatches → the union root can be the function entry → an independent mutually exclusive with all arms is over-excluded; pre-existing, needs reaching-defs). - renamed-symbol comment leftovers (primaryDispatch → primaryDispatchFn). - TestSplit_ResponsesCopiedPerRoute now also covers the method-specific (combined-case) ResponseInfo copy site, not only the shared one. - TestContributingDispatchArms now genuinely pins the group-0 and foreign-fn skips (gives those branches groups whose arms WOULD leak if a skip were dropped). Full suite green (incl. -race + determinism); coverage unchanged (metadata 98.0%, spec 96.0%).
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
1 participant
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
Elevates the per-function CFG (built via
golang.org/x/tools/go/cfg, previously used only to annotate branches) into a retained, queryable reachability + dominance model, and migrates the conditional-analysis consumers onto it instead of the former source-text-position heuristic.What changed
if/elseandswitcharms fan out; an earlyreturnbefore the write is excluded; an unconditional overwrite shadows earlier assignments; a value assigned inside a loop body reaches a write after the loop (analysis terminates across the back-edge). ThepositionAfter/positionLineCol/lastUncondheuristic is removed.switch v.(type)s on it, the call-site argument is bound to the matching arm — structural type identity (pointer / slice / map / generic aware). When the concrete type is not statically known, or the arms are not cleanly captured (nested control flow), it degrades to the unconditionally-reachable result + warning, or safely over-approximates — never mis-binding.if r.Method == http.MethodPostnow splits into one operation per method, the same as aswitch r.Method.New files
internal/metadata/dominators.go(Cooper-Harvey-Kennedy iterative dominators),internal/metadata/reachability.go(theReaches/Dominates/BlockForquery layer),internal/spec/warnings.go(FR-008/FR-012 warning sink).Validation
cfg_helper_typeswitch,cfg_loop_status,cfg_method_if_dispatch,cfg_branch_bodies.go test ./...,TestGolden_AllFrameworks(+ legacy),TestGolden_Deterministic— all green.gofmt/go vet/golangci-lint— clean (0 issues).internal/metadata98.2%,internal/spec96.0% (≥95% target).Review
Converged through the inner-loop → full-review gate. The type-switch helper binding (the most complex sub-feature) was hardened to be conservative-safe — verified across a 22-fixture "never worse than
main" battery — and the dominator algorithm was checked against a brute-force oracle over 6000+ random graphs. Documented limitations (named-interface argument, union-of-mutually-exclusive-branch phantom, single-block-loop reachability, a rareselectposition collision) are pre-existing or rare and noted in code comments.Summary by CodeRabbit