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

src/PlanViewer.Core/Output/ResultMapper.cs

@@ -24,14 +24,13 @@ internal static AnalysisResult MapCancellable(
             SqlServerBuild = plan.Build
         };
 
-        foreach (var batch in plan.Batches)
+        /* #455: includes statements nested inside a stored procedure or UDF body. Without this the
+           summary counted the EXEC and nothing else, so a procedure carrying dozens of statement
+           plans reported total_statements 1 and max_estimated_cost 0. */
+        foreach (var stmt in PlanStatements.EnumerateAll(plan))
         {
             cancellationToken.ThrowIfCancellationRequested();
-            foreach (var stmt in batch.Statements)
-            {
-                cancellationToken.ThrowIfCancellationRequested();
-                result.Statements.Add(MapStatement(stmt, cancellationToken));
-            }
+            result.Statements.Add(MapStatement(stmt, cancellationToken));
         }
 
         result.Summary = BuildSummary(result, cancellationToken);

src/PlanViewer.Core/Services/PlanAnalyzer.cs

@@ -45,10 +45,11 @@ internal static void AnalyzeCancellable(
         CancellationToken cancellationToken)
     {
         var cfg = config ?? AnalyzerConfig.Default;
-        foreach (var batch in plan.Batches)
         {
-            cancellationToken.ThrowIfCancellationRequested();
-            foreach (var stmt in batch.Statements)
+            /* #455: every statement, including the ones inside a stored procedure or UDF body. This
+               used to walk batch.Statements alone, so an EXEC <procedure> plan analyzed as a single
+               statement with nothing to say about it. */
+            foreach (var stmt in PlanStatements.EnumerateAll(plan))
             {
                 cancellationToken.ThrowIfCancellationRequested();
                 AnalyzeStatement(stmt, cfg, serverMetadata);

src/PlanViewer.Core/Services/PlanStatements.cs

@@ -0,0 +1,68 @@
+using System.Collections.Generic;
+using PlanViewer.Core.Models;
+
+namespace PlanViewer.Core.Services;
+
+/// <summary>
+/// Walks every statement in a plan, including the ones nested inside a stored procedure or a
+/// user-defined function (#455).
+///
+/// <para><b>Why this exists.</b> A plan captured around <c>EXEC &lt;procedure&gt;</c> puts the
+/// procedure's statements under a <c>StoredProc</c> element, and the parser has always read them —
+/// into <see cref="PlanStatement.StoredProcPlan"/>. Nothing downstream looked there. The analyzer and
+/// the result mapper both walked <c>batch.Statements</c> only, so a plan with eighty-seven statement
+/// plans inside a procedure analyzed as one statement, zero warnings, zero cost, and exited 0. The
+/// output was well-formed and entirely wrong, which is the worst way for this to fail.</para>
+///
+/// <para>The traversal itself was not missing — <c>PlanOperations.ValidateComplexity</c> has always
+/// descended, which is how the complexity limit counted statements the analysis never saw. This puts
+/// that same walk in one place so the two cannot disagree again.</para>
+/// </summary>
+public static class PlanStatements
+{
+    /// <summary>
+    /// Every statement in the plan, outermost first, each nested body following the statement that
+    /// owns it.
+    /// </summary>
+    public static IEnumerable<PlanStatement> EnumerateAll(ParsedPlan plan)
+    {
+        foreach (var batch in plan.Batches)
+        {
+            foreach (var statement in EnumerateAll(batch.Statements))
+                yield return statement;
+        }
+    }
+
+    /// <summary>
+    /// <paramref name="statements"/> and everything nested beneath them.
+    ///
+    /// <para>An explicit stack rather than recursion, because procedure bodies nest — a procedure
+    /// calling a procedure calling a function — and #430 was a crash caused by assuming a plan's
+    /// shapes are shallow.</para>
+    /// </summary>
+    public static IEnumerable<PlanStatement> EnumerateAll(IReadOnlyList<PlanStatement> statements)
+    {
+        var pending = new Stack<PlanStatement>();
+        for (var i = statements.Count - 1; i >= 0; i--)
+            pending.Push(statements[i]);
+
+        while (pending.TryPop(out var statement))
+        {
+            yield return statement;
+
+            /* Pushed in reverse so the bodies come back out in source order, and pushed AFTER the
+               statement is yielded so a body follows the EXEC that owns it rather than preceding it. */
+            for (var i = statement.UdfPlans.Count - 1; i >= 0; i--)
+                PushAll(statement.UdfPlans[i].Statements, pending);
+
+            if (statement.StoredProcPlan is not null)
+                PushAll(statement.StoredProcPlan.Statements, pending);
+        }
+    }
+
+    private static void PushAll(IReadOnlyList<PlanStatement> statements, Stack<PlanStatement> pending)
+    {
+        for (var i = statements.Count - 1; i >= 0; i--)
+            pending.Push(statements[i]);
+    }
+}

src/PlanViewer.Core/Services/ShowPlanParser.cs

@@ -278,6 +278,52 @@ private static List<PlanStatement> ParseStatementAndChildren(
             }
         }
 
+        /* #455: sub-plans are read BEFORE the no-QueryPlan early return below, because an
+           EXEC <procedure> statement has no QueryPlan of its own - every plan lives in the body -
+           so it took that early return and never reached this code, seventy lines further down. The
+           parser looked like it descended into procedures and in the one case that matters never
+           did. The same was true of a UDF call whose statement carries no plan of its own. */
+        // XSD gap: UDF sub-plans
+        foreach (var udfEl in stmtEl.Elements(Ns + "UDF"))
+        {
+            var udfInfo = new FunctionPlanInfo
+            {
+                ProcName = udfEl.Attribute("ProcName")?.Value ?? "",
+                IsNativelyCompiled = udfEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1"
+            };
+            var udfStmts = udfEl.Element(Ns + "Statements");
+            if (udfStmts != null)
+            {
+                foreach (var childStmt in udfStmts.Elements())
+                {
+                    var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken);
+                    udfInfo.Statements.AddRange(parsed);
+                }
+            }
+            stmt.UdfPlans.Add(udfInfo);
+        }
+
+        // XSD gap: StoredProc sub-plan
+        var storedProcEl = stmtEl.Element(Ns + "StoredProc");
+        if (storedProcEl != null)
+        {
+            var spInfo = new FunctionPlanInfo
+            {
+                ProcName = storedProcEl.Attribute("ProcName")?.Value ?? "",
+                IsNativelyCompiled = storedProcEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1"
+            };
+            var spStmts = storedProcEl.Element(Ns + "Statements");
+            if (spStmts != null)
+            {
+                foreach (var childStmt in spStmts.Elements())
+                {
+                    var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken);
+                    spInfo.Statements.AddRange(parsed);
+                }
+            }
+            stmt.StoredProcPlan = spInfo;
+        }
+
         if (queryPlanEl == null)
         {
             // Statements with no QueryPlan (e.g., DECLARE/ASSIGN) still get a synthetic
@@ -333,46 +379,6 @@ private static List<PlanStatement> ParseStatementAndChildren(
             stmt.RootNode = stmtNode;
         }
 
-        // XSD gap: UDF sub-plans
-        foreach (var udfEl in stmtEl.Elements(Ns + "UDF"))
-        {
-            var udfInfo = new FunctionPlanInfo
-            {
-                ProcName = udfEl.Attribute("ProcName")?.Value ?? "",
-                IsNativelyCompiled = udfEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1"
-            };
-            var udfStmts = udfEl.Element(Ns + "Statements");
-            if (udfStmts != null)
-            {
-                foreach (var childStmt in udfStmts.Elements())
-                {
-                    var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken);
-                    udfInfo.Statements.AddRange(parsed);
-                }
-            }
-            stmt.UdfPlans.Add(udfInfo);
-        }
-
-        // XSD gap: StoredProc sub-plan
-        var storedProcEl = stmtEl.Element(Ns + "StoredProc");
-        if (storedProcEl != null)
-        {
-            var spInfo = new FunctionPlanInfo
-            {
-                ProcName = storedProcEl.Attribute("ProcName")?.Value ?? "",
-                IsNativelyCompiled = storedProcEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1"
-            };
-            var spStmts = storedProcEl.Element(Ns + "Statements");
-            if (spStmts != null)
-            {
-                foreach (var childStmt in spStmts.Elements())
-                {
-                    var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken);
-                    spInfo.Statements.AddRange(parsed);
-                }
-            }
-            stmt.StoredProcPlan = spInfo;
-        }
 
         return stmt;
     }

src/PlanViewer.Web/PlanViewer.Web.csproj

@@ -23,6 +23,7 @@
     <Compile Include="..\PlanViewer.Core\Services\ShowPlanParser.Warnings.cs" Link="Core\Services\ShowPlanParser.Warnings.cs" />
     <Compile Include="..\PlanViewer.Core\Services\ShowPlanParser.Costs.cs" Link="Core\Services\ShowPlanParser.Costs.cs" />
     <Compile Include="..\PlanViewer.Core\Services\ShowPlanParser.Helpers.cs" Link="Core\Services\ShowPlanParser.Helpers.cs" />
+    <Compile Include="..\PlanViewer.Core\Services\PlanStatements.cs" Link="Core\Services\PlanStatements.cs" />
     <Compile Include="..\PlanViewer.Core\Services\PlanAnalyzer.cs" Link="Core\Services\PlanAnalyzer.cs" />
     <Compile Include="..\PlanViewer.Core\Services\PlanAnalyzer.Statement.cs" Link="Core\Services\PlanAnalyzer.Statement.cs" />
     <Compile Include="..\PlanViewer.Core\Services\PlanAnalyzer.Node.cs" Link="Core\Services\PlanAnalyzer.Node.cs" />

tests/PlanViewer.Core.Tests/PlanTestHelper.cs

@@ -44,15 +44,15 @@ public static List<PlanWarning> AllWarnings(ParsedPlan plan)
     {
         var warnings = new List<PlanWarning>();
 
-        foreach (var batch in plan.Batches)
+        /* #455: every statement, including the ones inside a stored procedure or UDF body. This
+           walked batch.Statements alone, exactly like the analyzer did, so the golden master could
+           not have caught the analyzer skipping procedure bodies - the two shared a blind spot. */
+        foreach (var stmt in PlanViewer.Core.Services.PlanStatements.EnumerateAll(plan))
         {
-            foreach (var stmt in batch.Statements)
-            {
-                warnings.AddRange(stmt.PlanWarnings);
+            warnings.AddRange(stmt.PlanWarnings);
 
-                if (stmt.RootNode != null)
-                    CollectNodeWarnings(stmt.RootNode, warnings);
-            }
+            if (stmt.RootNode != null)
+                CollectNodeWarnings(stmt.RootNode, warnings);
         }
 
         return warnings;

tests/PlanViewer.Core.Tests/StoredProcedurePlanTests.cs

@@ -0,0 +1,106 @@
+using System.Linq;
+using PlanViewer.Core.Output;
+using PlanViewer.Core.Services;
+
+namespace PlanViewer.Core.Tests;
+
+/// <summary>
+/// #455: a plan captured around <c>EXEC &lt;procedure&gt;</c> analyzed as one statement, zero
+/// warnings, zero cost, exit 0 — on a file containing dozens of statement plans.
+///
+/// <para>The reported diagnosis was that the parse never descended into the procedure. It is subtler
+/// than that and the distinction decides the fix: the parser had always read <c>StoredProc</c>
+/// sub-plans, but that code sat BELOW an early return taken when a statement carries no
+/// <c>QueryPlan</c> of its own — which is exactly what an <c>EXEC</c> statement is. The descent
+/// existed and was unreachable in the only case it was written for.</para>
+///
+/// <para>What made it dangerous is that the output was well-formed and plausible. Nothing said the
+/// analysis had stopped early; it looked like a clean plan with nothing to report, which invites
+/// "no warnings found" about a procedure that in fact has plenty.</para>
+/// </summary>
+public class StoredProcedurePlanTests
+{
+    /// <summary>
+    /// The three numbers from the report, asserted together. Any one alone could be innocent; the
+    /// combination — one statement, no warnings, no cost — is the signature of a parse that did not
+    /// reach the real statements.
+    /// </summary>
+    [Fact]
+    public void AnExecProcedurePlanAnalyzesTheProcedureBody()
+    {
+        var plan = PlanTestHelper.LoadAndAnalyze("exec_stored_procedure_plan.sqlplan");
+        var result = ResultMapper.Map(plan, "exec_stored_procedure_plan.sqlplan");
+
+        Assert.True(result.Summary.TotalStatements > 1,
+            $"the procedure body's statements are missing (got {result.Summary.TotalStatements})");
+        Assert.True(result.Summary.TotalWarnings > 0,
+            "a body with a non-SARGable predicate and a table variable cannot be warning-free");
+        Assert.True(result.Summary.MaxEstimatedCost > 0,
+            "every statement carrying a plan has a cost; zero means none were seen");
+    }
+
+    /// <summary>
+    /// The specific cause, pinned so a future edit cannot reintroduce it by moving sub-plan parsing
+    /// back below the early return: the EXEC statement itself has no QueryPlan, and must still
+    /// carry its body.
+    /// </summary>
+    [Fact]
+    public void TheExecStatementHasNoPlanOfItsOwnAndStillCarriesItsBody()
+    {
+        var plan = PlanTestHelper.LoadAndAnalyze("exec_stored_procedure_plan.sqlplan");
+
+        var exec = plan.Batches.SelectMany(b => b.Statements).Single();
+
+        Assert.NotNull(exec.StoredProcPlan);
+        Assert.NotEmpty(exec.StoredProcPlan!.Statements);
+    }
+
+    /// <summary>
+    /// The traversal the analyzer, the mapper and the test helper now share. They each walked
+    /// batch.Statements separately before, which is how the analyzer and the golden master came to
+    /// have the same blind spot and neither could catch the other.
+    /// </summary>
+    [Fact]
+    public void EnumerateAllReachesNestedStatements()
+    {
+        var plan = PlanTestHelper.LoadAndAnalyze("exec_stored_procedure_plan.sqlplan");
+
+        var shallow = plan.Batches.SelectMany(b => b.Statements).Count();
+        var all = PlanStatements.EnumerateAll(plan).Count();
+
+        Assert.Equal(1, shallow);
+        Assert.True(all > shallow, $"nested statements were not reached (shallow {shallow}, all {all})");
+    }
+
+    /// <summary>
+    /// The outer statement comes back before its body. Order matters for output: a reader scanning
+    /// the statement list should meet the EXEC before the statements it caused.
+    /// </summary>
+    [Fact]
+    public void TheCallingStatementComesBeforeItsBody()
+    {
+        var plan = PlanTestHelper.LoadAndAnalyze("exec_stored_procedure_plan.sqlplan");
+
+        var all = PlanStatements.EnumerateAll(plan).ToList();
+        var exec = plan.Batches.SelectMany(b => b.Statements).Single();
+
+        Assert.Same(exec, all[0]);
+    }
+
+    /// <summary>
+    /// Plans without a procedure are untouched. The fix only ever adds statements that were being
+    /// dropped, so a plain batch must enumerate exactly as it always did.
+    /// </summary>
+    [Theory]
+    [InlineData("row_goal_plan.sqlplan")]
+    [InlineData("key_lookup_plan.sqlplan")]
+    [InlineData("spill_plan.sqlplan")]
+    public void APlanWithNoProcedureEnumeratesUnchanged(string planFile)
+    {
+        var plan = PlanTestHelper.LoadAndAnalyze(planFile);
+
+        Assert.Equal(
+            plan.Batches.SelectMany(b => b.Statements).Count(),
+            PlanStatements.EnumerateAll(plan).Count());
+    }
+}

tests/PlanViewer.Core.Tests/WarningBaseline.txt

@@ -75,6 +75,17 @@ Exchange Spill | Critical | Exchange spill — 5,000,000 writes to TempDB. The p
 Expensive Operator | Critical | Sort took 65,000ms (81.2% of statement elapsed) but no specific rule identified a fix. Worth investigating: is the row volume necessary? Are upstream estimates driving this operator harder than it should be?
 Bare Scan | Warning | Clustered index scan reads the full table with no predicate, outputting 1 column(s): Test.Col1. Consider a nonclustered index on the output columns (as key or INCLUDE) so SQL Server can read a narrower structure. For analytical workloads, a columnstore index may be a better fit.
 
+### exec_stored_procedure_plan.sqlplan
+Non-SARGable Predicate | Warning | Implicit conversion (CONVERT_IMPLICIT) prevents an index seek. Match the parameter or variable data type to the column data type.\nPredicate: CONVERT_IMPLICIT(nvarchar(20),[ps455].[dbo].[Orders].[Code],0)=[@code]
+Filter Operator | Warning | Filter operator discarding rows late in the plan.\nPredicate: [ps455].[dbo].[Orders].[Amount]>(0.5)
+Table Variable | Critical | This query modifies a table variable, which forces the entire plan to run single-threaded. SQL Server cannot use parallelism for modifications to table variables. Replace with a #temp table to allow parallel execution.
+Table Variable | Critical | Modifying a table variable forces the entire plan to run single-threaded. Replace with a #temp table to allow parallel execution.
+Top Above Scan | Warning | Top reads from Clustered Index Scan on ps455.dbo.Orders (Node 2). An index on the ORDER BY columns could eliminate the scan and sort entirely.
+Row Goal | Info | Row goal active: estimate reduced from 20,000 to 100 (200x reduction) due to TOP. The optimizer chose this plan shape expecting to stop reading early. If the query reads all rows anyway, the plan choice may be suboptimal.
+Table Variable | Warning | Table variable detected. Table variables lack column-level statistics, which causes bad row estimates, join choices, and memory grant decisions. Replace with a #temp table.
+Scan With Predicate | Warning | Scan with residual predicate — SQL Server is reading every row and filtering after the fact. Check that you have appropriate indexes.\nPredicate: @t.[Id] as [t].[Id] IS NOT NULL
+Table Variable | Warning | Table variable detected. Table variables lack column-level statistics, which causes bad row estimates, join choices, and memory grant decisions. Replace with a #temp table.
+
 ### implicit_convert_seek_plan.sqlplan
 Implicit Conversion | Critical | Implicit conversion prevented an index seek, forcing a scan instead. Fix the data type mismatch: ensure the parameter or variable type matches the column type exactly. Seek Plan: CONVERT_IMPLICIT(nvarchar(40),[TestDB].[dbo].[Users].[DisplayName],0)=[@d]
 Bare Scan | Warning | Clustered index scan reads the full table with no predicate, outputting 1 column(s): Users.Id. Consider a nonclustered index on the output columns (as key or INCLUDE) so SQL Server can read a narrower structure. For analytical workloads, a columnstore index may be a better fit.