Sweep the review's minor findings: analyzer honesty, error display, lifecycle warts - #488
Conversation
…riable columns Three verified findings from the adversarial review, all in the Core analysis path. "Implicit Conversion" is both rule 29's legacy-listed type and what the parser stamps on the engine's own PlanAffectingConvert record (#436's Source split), and both the legacy pass and TryOverrideSeverity matched by type name alone. The engine's record rendered "[SQL Server] [legacy]" - a migration badge for OUR un-migrated rules, on a warning that is not ours - and a user's rule-number severity override landed on engine warnings the rule never produced, with the Contains matching spreading it wider (every engine Spill variant onto rule 7, "Memory Grant" onto rule 9). Both passes now skip anything stamped Source=SqlServer. ColumnReferenceRegex excluded @ from the first bracket part to keep variables ([@p]) from reading as columns, but a table-variable COLUMN renders [@tv].[col] - so a real column-side CONVERT_IMPLICIT on one lost its Non-SARGable warning. The "].[" sequence is what a bare variable can never have, so it alone draws the line now. ParameterSubstitution's assignment guard (#482) missed three shapes that produced misleading copied SQL: SELECT TOP (1) @A = col (the back-scan met TOP's ")" and gave up, substituting @A into "NULL = col"), EXEC @rc = proc and the FIRST named argument of EXEC dbo.p @debug = @debug (what precedes them is a procedure name, not a keyword; inside EXEC grammar "=" only ever means assignment, so a statement leading with EXEC/EXECUTE settles it), and FETCH ... INTO @A, @b (assignment with no "=" at all - the INTO leading the list answers it, for every member of the list). Reads keep substituting: WHERE @A = 1, UPDATE ... SET col = @p, TOP (@n), positional EXEC arguments and IN lists are all pinned by tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
The defect #452 fixed at the session-level Query Store site, fixed at the five sites that still had it: two fetch paths, the metric refresh, the database check, and the time slicer all cut exception text to 60-80 characters + "..." before handing it to StatusText. The interesting half of a SQL error - the login failure, the firewall hint - is rarely in its first 60 characters, and the cut threw it away for good. The sites now pass the full message, and the strip mirrors whatever it shows into its tooltip, which is #452's recovery path. One PropertyChanged subscription in the constructor rather than a tooltip set at each error site: this control writes StatusText from a dozen places across five partials, and a tip set only on errors would go stale the moment "Fetching plans..." overwrote the text but not the tip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
… saves Two file-fidelity findings from the review. PerformanceStudio.exe query.sql showed "The XML is not valid" where the query should have been: the constructor's argv handling was the one path still hard-wired to LoadPlanFile, while the second-instance pipe, drag-and-drop, and session restore all routed through OpenFileByExtension - whose .sql consequence RestoreOpenPlans' own doc comment spells out verbatim. The argv path now takes the same router, split out as OpenFromStartupArgs so a test can hand it an argv of its choosing. Opening a .sql read its BOM correctly (File.ReadAllText honors it) and the first save wrote UTF-8 without one - a UTF-16 file from SSMS was silently transcoded in place, every byte changed, nothing asked. The mark's encoding is captured at open onto the session next to SourceFilePath and handed to AtomicFile at save. A BOM sniff rather than StreamReader.CurrentEncoding, which cannot tell a UTF-8-BOM file from a plain one and whose default instance would stamp marks onto files that never had them. BOM-less files and scratch queries keep the UTF-8-without-BOM every save always wrote, pinned by test alongside the UTF-16 round trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
Detaching a session never recomputed its Compare button: #447 made the count window-wide, the sub-tab watcher only fires on sub-tab changes, and a detach makes none - so the torn-off window kept offering a comparison whose second plan it could no longer reach, until the next plan landed. DetachTabToWindow now runs the session's own recompute, which with no MainWindow above it counts the session's own plans - the only honest answer in a detached window. Redock needs no twin call; adding the tab back fires the window's collection watcher. MainWindow.OnClosing latches _closeConfirmed only after the unsaved-work walk answers yes, so a second close arriving mid-walk (a double-clicked X, an Alt+F4 behind a Save As picker) started a second concurrent walk: duplicate prompts about the same tabs, and a Cancel one walk never heard. One in-progress latch, the same reentrancy class and the same shape as the About window's update link (#485 review). DetachedWindowHelper had the same gap - its closeConfirmed also latches only on a yes, and the Save As its prompt can raise is not modal to the window - so it carries the same latch. CreateTab subscribed a close-glyph refresher onto the session and nothing ever took it off; the session outlives its tab on the detach path, so every detach/redock cycle pinned one more dead TabItem (closure over its close button, visual tree and all) to the session for good. The unhook is remembered per-tab in a ConditionalWeakTable - a Dictionary would be its own leak for tabs that close normally - and detach removes exactly its own subscription; redock re-subscribes through CreateTab, and the glyph still tracks dirty state afterwards, by test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
| or SET), but EXEC grammar has no reads-then-"=" shape at all, so inside one the "=" | ||
| alone settles it. Later named arguments were already caught by the comma rule; this | ||
| makes the first one match its siblings. */ | ||
| if (assignsThroughEquals) |
There was a problem hiding this comment.
assignsThroughEquals is computed once for the whole statement (StatementLeadsWithExec, line 56) and then applied to every @token = pair found anywhere in an EXEC statement's text, regardless of nesting. The doc comment's premise — "EXEC grammar has no other use for = at all" — only holds for the top-level argument list; it isn't true for expressions passed as argument values.
Concrete case that mis-fires:
EXEC dbo.p @flag = CASE WHEN @x = 1 THEN 1 ELSE 0 END
@x here is a read (compared to 1 inside the CASE), not a named-argument target. But @x is immediately followed by a lone =, so followedByLoneEquals is true, assignsThroughEquals is true (the statement leads with EXEC), and this branch returns true — @x is treated as an assignment target and its captured value is silently not substituted, even though nothing else in the statement assigns to @x.
Same shape via a scalar subquery argument: EXEC dbo.p @flag = (SELECT TOP 1 id FROM t WHERE @y = t.x).
This isn't a crash or an injection risk — the fallback is to leave @x/@y as the bare name, which is the same "safe but incomplete" behavior the code already accepts for OPTION(RECOMPILE)/local variables. But it's a real regression in precision introduced by this PR: the old per-token back-scan (checking for a literal preceding SELECT/SET/comma) wouldn't have matched @x here, so this specific gap is new. Since EXEC arguments can be arbitrary scalar expressions (CASE, subqueries, arithmetic), consider narrowing assignsThroughEquals to only fire when the token immediately preceding the @name (skipping whitespace) is the procedure-name token, a comma at the argument-list's own nesting depth, or EXEC/EXECUTE itself — rather than applying it statement-wide.
There was a problem hiding this comment.
Resolved in 51051e8 as documentation rather than narrowing, because the counterexamples cannot compile: T-SQL restricts an EXEC argument's value to a literal, a variable, NULL, or DEFAULT — CASE expressions and subqueries there are syntax errors — so no legal compiled statement text can carry a read-shaped '@x =' inside the region the flag governs. The grammar fact now lives in StatementLeadsWithExec's doc where the next reader will ask the same (good) question. A hand-crafted plan file can contain anything, but its author controls the whole statement text, so per-token precision against it protects nothing. Happy to take the narrowing anyway if you see a compilable shape I'm missing — that would flip this from doc-fix to code-fix.
|
Reviewed this PR (parameter-substitution EXEC/FETCH/TOP handling, save-encoding preservation, close-reentrancy guards, tab-glyph-subscription leak fix, engine-warning-vs-legacy-rule misattribution, and status-strip truncation removal). Overall this is a well-scoped, well-tested set of fixes — each change is paired with a targeted regression test (
No other blocking findings. |
…xth truncation site The gate review proposed narrowing assignsThroughEquals to the argument list's own nesting, citing reads like EXEC dbo.p @Flag = CASE WHEN @x = 1. That statement cannot compile: T-SQL restricts an EXEC argument's value to a literal, a variable, NULL, or DEFAULT - expressions are a syntax error - so no legal compiled statement puts a read to the right of a named argument, and the only @name = pairs in the region the flag governs are assignments. Written into StatementLeadsWithExec's doc so the question is settled where the next reader will ask it. A crafted plan file can hold anything, but its author controls the whole statement text, so per-token precision against it protects nothing. Also picks up the SIXTH error pre-truncation site the sweep noticed out of scope (PlanViewerControl.Interaction.cs SavePlan catch) with the same mirror the other five got: full message out, tooltip carries the clip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
|
Reviewed the full diff against dev (d0939cb). This is a tight, well-scoped bug-fix sweep and I didn't find correctness, injection, or convention issues worth flagging. Specifics I traced through in detail:
Test coverage is thorough and specifically targets the fixed defects (verified several by hand-tracing the algorithm against the new assertions rather than just reading them). |
#488 gave DetachedWindowHelper the same walk-in-progress latch MainWindow.OnClosing got, for the same gap, but only the MainWindow half was pinned. Deleting the detached closeGuardPending check outright left the suite green — which is how a latch gets tidied away by someone who reasonably believes the tests are watching it. Same shape as the existing test, against the detached window: close once and the question comes up, close again and no second prompt stacks, dismiss it and the window stays with the latch cleared, close once more and a fresh question starts. Proved red against the latch's absence before it went green. Claude-Session: https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
What does this PR do?
Closes the nine Minor findings from the 2026-09-02 adversarial review, in four commits:
[legacy]or captured by user severity overrides (the type-name collision also misrouted engine Spill/Memory Grant warnings onto rules 7/9 — the Source-skip covers all of it);[@tv].[col]now reads as the column reference it is, restoring Non-SARGable detection on table-variable columns; andIsAssignmentTargetlearns the three verified misses —SELECT TOP (1) @a = col(skip one balanced group backwards),EXEC @rc = proc/ first named EXEC argument (inside EXEC,=only ever assigns), andFETCH … INTO @a, @b— while every read shape still substitutes.PerformanceStudio.exe query.sqlnow routes by extension instead of greeting the user with "The XML is not valid" (extracted to a testableOpenFromStartupArgs), and a file's encoding survives open→save round-trips via BOM sniff (UTF-16 stays UTF-16; BOM-less and scratch stay UTF-8-no-BOM).DirtyStateChangedsubscription comes off at detach instead of leaking a dead TabItem per detach/redock cycle.All nine came from the review; three fixes were verified red against pre-fix code by revert-and-rerun. Conflict note: the cherry-pick onto dev was resolved so #487's test-host gates stay at the constructor call sites while
OpenFromStartupArgscarries only the routing.How was this tested?
Nineteen new test executions across
WindowCloseReentryTests(new),QueryStoreErrorDisplayTests(new), and extensions to the analyzer, substitution, compare-availability, detached-unsaved, and open/save suites — items 6/7/8 all got real headless tests, not inspection. Full suite at dev tip: 444 tests, 443 passed, 1 platform skip, 0 failed, on Windows.🤖 Generated with Claude Code
https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n