Leave the sp_executesql declaration list out of substituted statement text

src/PlanViewer.Core/Services/ParameterSubstitution.cs

@@ -47,62 +47,76 @@ public static ParameterSubstitutionResult Apply(
         if (values.Count == 0)
             return new ParameterSubstitutionResult(statementText, 0);
 
+        /* A plan from the plan cache or Query Store keeps an sp_executesql statement's declaration
+           list in front of it: "(@p1 int, @p2 int)SELECT …". The names in that list declare the
+           parameters, they do not read them. Substituted, they became "(10 int, 20 int)SELECT …",
+           which is neither the plan's text nor runnable. So only the statement after the list gets
+           values, and when it gets any, the list is dropped: this text is meant to run, and a
+           declaration list is not T-SQL on its own. Nothing substituted leaves the text as it was.
+           A list that never closes is text the plan cut off at 4,000 characters inside the list,
+           so there is no statement to put values into. */
+        var bodyStart = DeclarationListEnd(statementText);
+        if (bodyStart < 0)
+            return new ParameterSubstitutionResult(statementText, 0);
+
+        var text = bodyStart == 0 ? statementText : statementText[bodyStart..].TrimStart();
+
         /* One fact about the whole statement, settled up front: inside an EXEC statement, every
            token sitting to the left of an "=" is an assignment target — the return-status variable
            or a named argument's name — because EXEC grammar has no other use for "=" at all. A
            per-token back-scan cannot see this for the FIRST named argument (what precedes it is
            the procedure name, not a keyword), which is how "EXEC dbo.p @debug = @debug" got its
            left-hand side substituted into "EXEC dbo.p 1 = @debug". */
-        var assignsThroughEquals = StatementLeadsWithExec(statementText);
+        var assignsThroughEquals = StatementLeadsWithExec(text);
 
-        var sb = new StringBuilder(statementText.Length);
+        var sb = new StringBuilder(text.Length);
         var substitutions = 0;
         var i = 0;
 
-        while (i < statementText.Length)
+        while (i < text.Length)
         {
-            var c = statementText[i];
+            var c = text[i];
 
             /* Regions where an @name is text, not a parameter. A string literal is the case that
                matters in practice — LIKE 'kexin%' sits right next to the parameters in the #466
                repro — but a delimited identifier can hold anything, and a comment is not code. */
             if (c == '\'' || c == '"')
             {
-                i = CopyDelimited(statementText, i, c, c, sb);
+                i = CopyDelimited(text, i, c, c, sb);
                 continue;
             }
 
             if (c == '[')
             {
-                i = CopyDelimited(statementText, i, '[', ']', sb);
+                i = CopyDelimited(text, i, '[', ']', sb);
                 continue;
             }
 
-            if (c == '-' && i + 1 < statementText.Length && statementText[i + 1] == '-')
+            if (c == '-' && i + 1 < text.Length && text[i + 1] == '-')
             {
-                i = CopyLineComment(statementText, i, sb);
+                i = CopyLineComment(text, i, sb);
                 continue;
             }
 
-            if (c == '/' && i + 1 < statementText.Length && statementText[i + 1] == '*')
+            if (c == '/' && i + 1 < text.Length && text[i + 1] == '*')
             {
-                i = CopyBlockComment(statementText, i, sb);
+                i = CopyBlockComment(text, i, sb);
                 continue;
             }
 
             /* A whole token, or nothing. "@1" inside "@11" is a different parameter, and "@0" at the
                tail of an identifier such as "t@0" is part of that identifier. The scan below claims
                the longest run of identifier characters, which handles the first; the preceding
                character is checked here, which handles the second. */
-            if (c == '@' && !IsIdentifierPart(i > 0 ? statementText[i - 1] : '\0'))
+            if (c == '@' && !IsIdentifierPart(i > 0 ? text[i - 1] : '\0'))
             {
                 var end = i + 1;
-                while (end < statementText.Length && IsIdentifierPart(statementText[end]))
+                while (end < text.Length && IsIdentifierPart(text[end]))
                     end++;
 
-                var token = statementText[i..end];
+                var token = text[i..end];
                 if (values.TryGetValue(token, out var value)
-                    && !IsAssignmentTarget(statementText, i, end, assignsThroughEquals))
+                    && !IsAssignmentTarget(text, i, end, assignsThroughEquals))
                 {
                     sb.Append(value);
                     substitutions++;
@@ -125,6 +139,37 @@ tail of an identifier such as "t@0" is part of that identifier. The scan below c
             : new ParameterSubstitutionResult(sb.ToString(), substitutions);
     }
 
+    /// <summary>
+    /// Where the statement starts in text that opens with an <c>sp_executesql</c> declaration list,
+    /// such as <c>(@p1 int, @p2 decimal(18,2))SELECT …</c>. Returns 0 when the text has no list,
+    /// and -1 when the list never closes: a plan cuts statement text off at 4,000 characters, and
+    /// the declarations for a long IN list can fill all of them. The list ends at the parenthesis
+    /// that closes its first one, so the parentheses of a type are counted, not taken for the end.
+    /// A statement cannot begin with <c>(@</c>, so that opening always means a list.
+    /// <c>ReproScriptBuilder</c> strips the list with this too. (The web project compiles this
+    /// file without it, so the name is not a cref.)
+    /// </summary>
+    internal static int DeclarationListEnd(string text)
+    {
+        var i = 0;
+        while (i < text.Length && char.IsWhiteSpace(text[i]))
+            i++;
+
+        if (i + 1 >= text.Length || text[i] != '(' || text[i + 1] != '@')
+            return 0;
+
+        var depth = 0;
+        for (; i < text.Length; i++)
+        {
+            if (text[i] == '(')
+                depth++;
+            else if (text[i] == ')' && --depth == 0)
+                return i + 1;
+        }
+
+        return -1; // the list never closes: the text was cut off inside it
+    }
+
     /// <summary>
     /// True when the parameter token spanning <paramref name="start"/> to <paramref name="end"/> is
     /// being assigned TO rather than read from, in which case its value must not be written over it.

src/PlanViewer.Core/Services/ReproScriptBuilder.cs

@@ -365,39 +365,13 @@ private static List<string> ExtractSetOptionsFromPlan(string planXml)
     /// <summary>
     /// Strips the parameter declaration prefix from query text captured via sp_executesql.
     /// Query text like "(@p1 int, @p2 nvarchar(50))SELECT ..." becomes "SELECT ...".
-    /// Uses same approach as sp_QueryReproBuilder: find the closing ) followed by non-comma.
+    /// The list is found by the same parser that keeps parameter substitution out of it.
     /// </summary>
     private static string StripParameterPrefix(string queryText)
     {
-        if (!queryText.StartsWith("(@", StringComparison.Ordinal))
-        {
-            return queryText;
-        }
-
-        /* Find the closing parenthesis that ends the parameter list.
-           Look for ) followed by a character that's not a comma (which would indicate
-           we're still inside nested parentheses in a type like decimal(18,2)). */
-        int depth = 0;
-        for (int i = 0; i < queryText.Length; i++)
-        {
-            char c = queryText[i];
-            if (c == '(')
-            {
-                depth++;
-            }
-            else if (c == ')')
-            {
-                depth--;
-                if (depth == 0)
-                {
-                    /* Found the closing paren — return everything after it, trimmed */
-                    return queryText[(i + 1)..].TrimStart();
-                }
-            }
-        }
-
-        /* Couldn't find balanced parens — return original */
-        return queryText;
+        /* No list (0), or a list that never closes (-1): the text stays as it is, as before. */
+        var bodyStart = ParameterSubstitution.DeclarationListEnd(queryText);
+        return bodyStart <= 0 ? queryText : queryText[bodyStart..].TrimStart();
     }
 
     /// <summary>

tests/PlanViewer.Core.Tests/ComparisonBaseline.txt

@@ -312,7 +312,7 @@ Plan A: in_list_dynamic_seek_plan.sqlplan
 Plan B: in_list_dynamic_seek_plan.sqlplan
 
 --- Statement 1 ---
-(10 int, 20 int)SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20)
+SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20)
 
   Estimated cost:     0.0033 -> 0.0033  (0.0% costlier)
   Estimated rows:     2 -> 2  (0.0% more)
@@ -697,7 +697,7 @@ Plan A: non_sargable_compound_predicate_plan.sqlplan
 Plan B: non_sargable_compound_predicate_plan.sqlplan
 
 --- Statement 1 ---
-(52 tinyint,2 int)SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2)
+SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2)
 
   Estimated cost:     0.0033 -> 0.0033  (0.0% costlier)
   Estimated rows:     1 -> 1  (0.0% more)
@@ -1500,7 +1500,7 @@ Plan A: in_list_dynamic_seek_plan.sqlplan
 Plan B: isnull_plan.sqlplan
 
 --- Statement 1 ---
-(10 int, 20 int)SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20)
+SELECT t.Id, t.A FROM dbo.T AS t WHERE t.A IN (10, 20)
 
   Estimated cost:     0.0033 -> 3,119.42  (9,999% costlier)
   Estimated rows:     2 -> 1  (50.0% fewer)
@@ -1904,7 +1904,7 @@ Plan A: non_sargable_compound_predicate_plan.sqlplan
 Plan B: non_sargable_function_plan.sqlplan
 
 --- Statement 1 ---
-(52 tinyint,2 int)SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2)
+SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2)
 
   Estimated cost:     0.0033 -> 3,097.52  (9,999% costlier)
   Estimated rows:     1 -> 1  (0.0% more)

tests/PlanViewer.Core.Tests/ParameterSubstitutionTests.cs

@@ -390,4 +390,70 @@ parameter name survives anywhere in the text. */
         Assert.Contains("like 'kexin%'", result.Text);
         Assert.DoesNotContain("@", result.Text);
     }
+
+    [Fact]
+    public void DeclarationList_IsLeftOutAndNotSubstituted()
+    {
+        /* A plan from the plan cache or Query Store keeps an sp_executesql statement's declaration
+           list in front of it. Substituted like the rest, "(@p1 int, @p2 int)" became
+           "(10 int, 20 int)", which is neither the plan's text nor runnable. */
+        var result = ParameterSubstitution.Apply(
+            "(@p1 int, @p2 int)SELECT t.Id FROM dbo.T AS t WHERE t.A IN (@p1, @p2)",
+            new List<PlanParameter> { Param("@p1", "(10)"), Param("@p2", "(20)") });
+
+        Assert.Equal("SELECT t.Id FROM dbo.T AS t WHERE t.A IN (10, 20)", result.Text);
+        Assert.Equal(2, result.SubstitutionCount);
+    }
+
+    [Fact]
+    public void DeclarationListWithParenthesizedTypes_EndsAtItsOwnClosingParenthesis()
+    {
+        /* The comma inside decimal(18,2) and the parentheses of both types are part of the list.
+           Stopping at the first closing parenthesis would leave ",@b nvarchar(50))" in the text. */
+        var result = ParameterSubstitution.Apply(
+            "(@a decimal(18,2),@b nvarchar(50))SELECT * FROM t WHERE x = @a AND y = @b",
+            new List<PlanParameter> { Param("@a", "(1.50)"), Param("@b", "N'abc'") });
+
+        Assert.Equal("SELECT * FROM t WHERE x = 1.50 AND y = N'abc'", result.Text);
+    }
+
+    [Fact]
+    public void DeclarationList_StaysWhenNothingIsSubstituted()
+    {
+        /* With no value to put back, the text is shown as the plan recorded it, list included. */
+        const string text = "(@p1 int)SELECT 1";
+        var result = ParameterSubstitution.Apply(text, new List<PlanParameter> { Param("@p1", "(10)") });
+
+        Assert.Equal(text, result.Text);
+        Assert.Equal(0, result.SubstitutionCount);
+    }
+
+    [Fact]
+    public void DeclarationListCutOffByTruncation_IsLeftAsItIs()
+    {
+        /* A plan cuts statement text off at 4,000 characters, and the declarations for a long IN
+           list can fill all of them. Then the text is only declarations, with no statement after
+           them, and putting values into it would make "(1 int,2 int,…" again. */
+        const string text = "(@p0 int,@p1 int,@p2 in";
+        var result = ParameterSubstitution.Apply(
+            text, new List<PlanParameter> { Param("@p0", "(1)"), Param("@p1", "(2)") });
+
+        Assert.Equal(text, result.Text);
+        Assert.Equal(0, result.SubstitutionCount);
+    }
+
+    [Fact]
+    public void AutoParameterizedPlan_LosesItsDeclarationList()
+    {
+        /* The #556 reproduction, an auto-parameterized plan: its text starts with
+           "(@1 tinyint,@2 int)", and the comparison report printed "(52 tinyint,2 int)SELECT". */
+        var plan = PlanTestHelper.LoadAndAnalyze("non_sargable_compound_predicate_plan.sqlplan");
+        var statement = PlanTestHelper.FirstStatement(plan);
+        Assert.StartsWith("(@1 tinyint,@2 int)", statement.StatementText);
+
+        var result = ParameterSubstitution.Apply(statement.StatementText, statement.Parameters);
+
+        Assert.StartsWith("SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52", result.Text);
+        Assert.DoesNotContain("@", result.Text);
+    }
 }

tests/PlanViewer.Core.Tests/ReproScriptBuilderTests.cs

@@ -0,0 +1,46 @@
+using PlanViewer.Core.Services;
+
+namespace PlanViewer.Core.Tests;
+
+/// <summary>
+/// The repro script declares the parameters itself, so the declaration list that a cached
+/// sp_executesql statement starts with must not reach the script's query text. The list is found
+/// by the parser that parameter substitution uses (<c>ParameterSubstitution.DeclarationListEnd</c>).
+/// </summary>
+public class ReproScriptBuilderTests
+{
+    private const string Plan = """
+        <ShowPlanXML xmlns="http://schemas.microsoft.com/sqlserver/2004/07/showplan">
+          <BatchSequence><Batch><Statements>
+            <StmtSimple>
+              <QueryPlan>
+                <ParameterList>
+                  <ColumnReference Column="@id" ParameterDataType="decimal(18,2)" ParameterCompiledValue="(42.50)" />
+                </ParameterList>
+              </QueryPlan>
+            </StmtSimple>
+          </Statements></Batch></BatchSequence>
+        </ShowPlanXML>
+        """;
+
+    [Fact]
+    public void BuildReproScript_DeclarationList_IsLeftOutOfTheQueryText()
+    {
+        var sql = ReproScriptBuilder.BuildReproScript(
+            "(@id decimal(18,2))SELECT * FROM dbo.T WHERE Id = @id", "db", Plan, null);
+
+        Assert.Contains("SELECT * FROM dbo.T WHERE Id = @id", sql);
+        Assert.DoesNotContain("(@id decimal(18,2))SELECT", sql);
+        Assert.Contains("@id = 42.50", sql);
+    }
+
+    [Fact]
+    public void BuildReproScript_DeclarationListCutOffByTruncation_IsKeptAsItIs()
+    {
+        /* The parser reports a list that never closes as -1. The text stays as it was, as it did
+           before the parser was shared, and the -1 must never reach a slice. */
+        var sql = ReproScriptBuilder.BuildReproScript("(@id decimal(18,2),@x nvarch", "db", Plan, null);
+
+        Assert.Contains("(@id decimal(18,2),@x nvarch", sql);
+    }
+}