Descend into stored procedure bodies when analyzing a plan (#455)

[erikdarlingdata]

Fixes #455.

Reproduced first

SQL Server 2025, a procedure with a four-statement body, SET SHOWPLAN_XML ON around EXEC dbo.ReproProc:

	XML	analyze reported
statements	6 StmtSimple, 4 with a QueryPlan	1
cost	summed StatementSubTreeCost 1.88	0
warnings	body has a non-SARGable predicate and a table variable	0

Exit 0, empty stderr. The plan is committed as a fixture.

The cause is not quite the one reported

The report's reading was that the parse never descends into the procedure. It's subtler, and the distinction decides the fix.

ShowPlanParser has always read StoredProc sub-plans. But that code sits below an early return taken when a statement carries no QueryPlan of its own — and an EXEC statement is precisely a statement with no plan of its own, because every plan lives in the body:

if (queryPlanEl == null)
{
    stmt.RootNode = new PlanNode { ... };   // synthetic node for DECLARE/ASSIGN etc.
    return stmt;                            // ← never reaches the StoredProc block below
}

The descent existed and was unreachable in the only case it was written for. Same for a UDF call whose calling statement carries no plan. Moving sub-plan parsing above that early return fixes it.

Three consumers shared one blind spot

The traversal was never the missing piece — PlanOperations.ValidateComplexity has always descended, which is how the complexity limit counted statements the analysis never saw. Three others walked batch.Statements and are now sharing one traversal:

• PlanAnalyzer — so no rule ever ran on a procedure body.
• ResultMapper — where total_statements: 1 and max_estimated_cost: 0 came from.
• PlanTestHelper.AllWarnings — and this one is worth pausing on. The golden master and the analyzer had the same blind spot, so WarningCharacterizationTests could not have caught the analyzer skipping procedure bodies no matter how many procedure plans were committed. A test that can't see what the code can't see isn't covering it.

What does not change

No committed plan's verdict moves. Regenerating WarningBaseline.txt across the corpus produces additions only — the new fixture, nothing else — because the fix only ever adds statements that were being dropped. The CLI output hash is unchanged for the same reason: a plain batch enumerates exactly as before.

Also caught on the way in

PlanViewer.Web compiles Core sources through an explicit file list, not a glob, so a new Core file breaks the solution build until it's added there. Same shape of trap as the missed call sites in #438 and #439 — something you must remember in a second place — and I walked into it. Noted here so the next person doesn't.

Tests

327 passing, 0 failed. The new tests were checked by reverting the parser: three fail, exactly the three asserting the body is reached, while the ordering test and the three unchanged-plan cases correctly still pass.

🤖 Generated with Claude Code

[claude]

Both recursive calls here (ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken) at what are now lines 299 and 320) omit the depth argument, so it defaults back to 0 every time parsing descends into a StoredProc/UDF body. MaxParseDepth (1000, used as a circuit breaker against StackOverflowException — see the #430 reference in the new PlanStatements.cs) is checked in ParseStatementAndChildren against this reset-to-zero depth, not against the actual C# call-stack depth.

Before this PR, this block only ran when a statement itself carried a QueryPlan (post-early-return), so StoredProc/UDF recursion was rarely exercised for the pathological case (a procedure that itself calls another procedure, N levels deep). Moving this block above the early return is exactly what makes deep procedure-nesting parsing real for the first time — that's the intended fix — but it also means a crafted/malformed plan (proc-calling-proc-calling-proc...) can now drive unbounded native recursion through ParseStatement → ParseStatementAndChildren → ParseStatement → ... without ever tripping the depth guard, since each boundary resets the counter. Plan XML here is untrusted (emailed .sqlplan files), so a sufficiently deep chain is an uncatchable StackOverflowException / process crash, not a graceful InvalidOperationException.

Passing depth: depth + 1 through both calls (matching the StmtCond recursive calls above) would close this.

[claude]

The parser/analyzer fix itself looks right — I traced ParseStatement's new UDF/StoredProc block, PlanStatements.EnumerateAll's stack-based DFS, and the three updated call sites (PlanAnalyzer, ResultMapper, PlanTestHelper.AllWarnings), and the ordering (parent before its nested body, siblings preserved) checks out against the new fixture.

The "three consumers shared one blind spot" list is incomplete — three more in the same pipeline still walk batch.Statements shallowly:

• BenefitScorer.ScoreCancellable (src/PlanViewer.Core/Services/BenefitScorer.cs:33-36) — still foreach (var batch in plan.Batches) foreach (var stmt in batch.Statements). Warnings that PlanAnalyzer now correctly raises on procedure-body statements will never get ScoreStatementWarnings/ScoreNodeTree run on them, so their MaxBenefitPercent stays unset.
• PlanAnalyzer.Helpers.cs ApplySeverityOverrides (line 35-37) — same shallow walk. A user's configured severity override (cfg.Rules.SeverityOverrides) will apply to a warning on a top-level statement but silently not apply to the same warning type raised on a nested procedure-body statement.
• ShowPlanParser.Costs.cs ComputeOperatorCosts (line 16-19) — same shallow walk, called once at parse time. ComputeNodeCosts never runs on a nested statement's RootNode, so every node in a procedure-body statement's tree keeps EstimatedOperatorCost = 0 and CostPercent = 0 — this feeds the cost-percentage bars/highlighting in the UI.

All three run in the exact same PlanAnalysisPipeline.AnalyzeParsed path this PR fixes (parse → PlanAnalyzer.AnalyzeCancellable → ApplySeverityOverrides → BenefitScorer.ScoreCancellable), so the repro procedure from #455 will now show its nested statements and warnings, but those warnings will have MaxBenefitPercent: 0, no severity overrides applied, and every operator in those statements will show 0% cost — inconsistent with how the same warning/operator would render on a top-level statement. StoredProcedurePlanTests only asserts TotalStatements/TotalWarnings/MaxEstimatedCost via ResultMapper, so this gap isn't caught.

Worth at least giving these three the same PlanStatements.EnumerateAll treatment, or filing a fast-follow — right now the fix is incomplete for the actual UI-facing data, not just the summary counts.

Also flagged inline: the two relocated ParseStatementAndChildren calls for UDF/StoredProc sub-plans don't pass depth: depth + 1, so MaxParseDepth's stack-overflow guard resets at every procedure-nesting boundary instead of tracking real recursion depth — and this PR is what makes deep procedure-nesting actually get parsed for the first time.

Two minor nits, not blocking:

• In PlanStatements.EnumerateAll(IReadOnlyList<PlanStatement>), StoredProcPlan children are pushed onto the stack after UdfPlans children, which means (LIFO) they're yielded before the UDF children for a statement that has both (e.g. EXEC proc @x = dbo.ScalarFunc()). Order-only, but worth a comment if intentional.
• PlanOperations.ValidateComplexity still hand-rolls its own statement-stack descent instead of using the new PlanStatements.EnumerateAll — the PR's stated goal was "so the two cannot disagree again," but there are now two traversal implementations again, just harder to notice.