Repro script: keep @0/@1 parameters, check data types by shape (#590)

[erikdarlingdata]

What does this PR do?

Fixes #590.

The repro script for an auto-parameterized plan failed with Must declare the scalar variable "@1". SQL Server names the parameters of simple and forced parameterization @0, @1, and so on. IsValidParameterName required a letter after the @, so it dropped those parameters. The script then ran the statement without declaring them.

The changes, all in ReproScriptBuilder.cs:

• IsValidParameterName allows a digit right after the @.
• The checks for names, types and values use \A and \z instead of ^ and $. In .NET, $ also matches before a final line break, so a name like "@x\n" passed.
• IsValidDataType checks the shape of the type, not only its characters. A type is one to three names separated by dots, each plain or in brackets. An optional (n), (max), (p,s) or (n,name) can follow. A type with anything else, such as int) SELECT 2, is dropped with the existing warning. Before, it went into the script, and SQL Server refused the declaration.
• Each parameter name is declared once. A batch's plan lists each statement's parameters, so a name can appear more than once:
  • Keeping @0 and @1 made that common, because each auto-parameterized statement numbers its own. A batch of two such statements then declared @1 twice, and the script failed with The variable name '@1' has already been declared.
  • A parameter that two statements use, such as a shared @id, had the same failure before this PR.
  • A name that the statements give different types, such as @1 smallint and @1 tinyint, is left out with a warning. Those statements are usually literal text that does not use the name, so the script runs the batch as it is.
• Numbers in a type or a value must use the digits 0 to 9. \d also matches digits from other scripts, which T-SQL does not read as numbers.

This is the same idea as erikdarlingdata/PerformanceMonitor#4567, with two differences:

• The (n,name) form keeps vector(3,float16), which SQL Server 2025 writes into plans. The pattern in PerformanceMonitor#4567 drops that parameter.
• Only plain spaces can come between the parts of a type. Line breaks cannot, so a type cannot put GO on a line of its own.

Which component(s) does this affect?

• Desktop App (PlanViewer.App)
• Core Library (PlanViewer.Core)
• CLI Tool (PlanViewer.Cli)
• SSMS Extension (PlanViewer.Ssms)
• Tests
• Documentation

The desktop app's Copy Repro button, the MCP get_repro_script tool and the actual-plan runner all use the repro script.

How was this tested?

• The types SQL Server writes into plans come from SQL Server 2025. I ran parameterized statements there and read ParameterDataType from the cached plans:
  • An alias type shows as its base type, and a typed xml parameter shows as xml.
  • A table-valued parameter is not in the ParameterList.
  • The others shown include sys.geography, sys.hierarchyid, vector(3), vector(3,float16) and json.
• End to end on SQL Server 2025, with plans captured from the plan cache:
  • A simple-parameterization plan (@1 tinyint) and a forced-parameterization plan (@0 nvarchar(4000), @1 int).
  • Before: both scripts failed with Must declare the scalar variable.
  • After: both scripts declare and assign every parameter, and both run and return the row.
  • A vector(3,float16) parameter keeps its declaration. Its value is ? with a warning, as before, because the plan writes the vector value without quotes.
  • A two-statement batch whose estimated plan has @1 smallint and @1 tinyint runs as literal text, with the new warning.
  • A two-statement sp_executesql batch that uses @id in both statements declares @id once and runs. Before this PR, both scripts failed with Msg 134.
• New cases in ReproScriptBuilderSafetyTests:
  • Parameters named @0 and @1 are declared and assigned, and the script parses.
  • A name that ends in a line break is dropped, and a value that ends in one becomes ?.
  • 15 data types are kept, 14 of them taken from the SQL Server 2025 plans.
  • 8 malformed data types are dropped, including two with other scripts' digits and varchar(max,2).
  • A parameter that two statements use is declared once, and @1 with two types is left out with the warning.
  • A value written in Arabic-Indic digits becomes ?.
• Against the old checks, 7 of the first new cases fail. The 15 kept types pass there too, so they guard against the new check being too strict. The 6 cases added in review round 1 fail against this PR's first commit.
• Full suite on Windows: 975 total, 974 passed, 0 failed, 1 skipped. The Release build has 0 warnings.

Review round 1

• Fixed: a batch of two auto-parameterized statements declared @1 twice (above). The same fix covers a named parameter that two statements use.
• Fixed: \d let digits from other scripts into a type or a value. Both checks now use [0-9].
• Fixed: (max) accepted a second part, as in varchar(max,2).
• Fixed: the comment on IsValidDataType now says where spaces are allowed.
• Fixed: an older test for a hostile parameter name now checks the "omitted" warning too.

Review round 2

• Fixed: a parameter listed by two statements took the first entry's value. When that statement had no compiled value, the parameter was set to ? although the other statement had one. The script now uses the first value it can put in the script.
• Added: when the statements have different compiled values for a parameter, the script uses the first usable one and a warning says so.
• Fixed: when every parameter was left out, the script said No parameters found in plan cache. It now says No parameters declared: see the warnings above.
• Two new tests cover the values, and the test for @1 with two types checks the new comment. Both new tests fail against the round-1 commit.

Checklist

• I have read the contributing guide
• My code builds with zero warnings (Release build, --no-incremental)
• All tests pass (dotnet test)
• I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR

[claude]

Reviewed (post-merge). Traced the three regex changes against the untrusted-plan-XML threat model:

• IsValidParameterName/IsValidDataType/IsSafeLiteral all switched to \A/\z and [0-9] — verified this correctly rejects the line-break and non-ASCII-digit bypasses the new tests target (@id\n, Arabic-Indic/full-width digits), and the new IsValidDataType shape-check correctly rejects the malformed cases (int) SELECT 2 ..., unclosed parens, 4-part names, GO embedded via 
) while still accepting the real SQL Server 2025 plan types listed in the theory tests, including vector(3,float16).
• The dedup/conflict logic (parameterGroups → conflictingNames / declarableGroups → safeParameters → differingValueNames) is sound: grouping preserves first-seen order, the conflicting-type and differing-value warnings are computed independently and don't double-count against droppedCount, and every interpolated value (names, types, warning text with parameter names) is already restricted to the whitelist regexes or passes through CommentSafe before landing in the script. No path where an attacker-controlled name/type/value reaches the generated T-SQL unescaped.
• Test coverage is well-targeted — each new fact/theory maps to a specific fix (auto-param names, cross-statement dedup, type conflicts, value conflicts, line-break/Unicode-digit bypasses) and several assert against the pre-fix regression via ParseScript.

No correctness or injection issues found. One non-blocking observation: conflictingNames/differingValueNames compare DataType/CompiledValue via exact string equality, so a hypothetical formatting difference between statements for an otherwise-identical type (e.g. extra whitespace) would cause an unnecessary "left out" warning rather than declaring it — low-severity since real plan XML formats a given type consistently, and the fail-safe direction (over-dropping rather than mis-declaring) is the right one anyway.