Fix four analyzer rule gates (#577, #576, #579, #578)

[erikdarlingdata]

What does this PR do?

Fixes #577, #576, #579 and #578. Each is a gate that decides whether an analyzer rule fires.

1. [BUG] Row-estimate rule 5 warns about resource allocation on operators that never executed #577, rule 5 (row estimate mismatch). The rule warned on an operator with zero actual rows even when the operator never executed (ActualExecutions = 0). An operator that never ran returned zero rows because it never ran, so the zero says nothing about the estimate. Rule 5 now skips such operators, as rules 11, 12 and 29 already do. I checked the other node rules that read actual-run values (rules 8, 14, 16, 26, 28, 32 and 35). Each one needs a non-zero actual value before it fires, so none of them has this problem. After the review, a later commit removed the fallback ? node.ActualExecutions : 1 in rule 5. The new gate means that the fallback can never run.
2. [BUG] Disabling serial-plan rule 3 also suppresses scalar-UDF rule 6 #576, rule 6 (scalar UDF). The rule dropped its warning whenever NonParallelPlanReason was one of the three reasons that rule 3 explains. It assumed that rule 3 had fired. But rule 3 can be disabled, and it skips statements that cost under 1, TRIVIAL plans and 0 ms runs. In those cases the UDF warning disappeared and no warning replaced it. Rule 6 now drops its warning only when rule 3's "Serial Plan" finding is on the statement. Statement rules run before node rules, so the finding is already there when rule 6 runs.
3. [BUG] OPTIMIZE FOR UNKNOWN rule 27 treats string literals and comments as query hints #579, rule 27 (OPTIMIZE FOR UNKNOWN). The rule matched the raw statement text. So the phrase counted as a hint even inside a string literal or a comment. The new MaskCommentsAndLiterals helper replaces string-literal contents and whole comments with spaces before the match. It handles '...' with doubled quotes, -- comments and nested /* */ comments. It steps over [...] and "..." identifiers unchanged, so a quote inside [it's] does not start a string. The scan follows the one in ParameterSubstitution. Seven other checks had the same problem, and they now use the helper too:
   • rule 3: MAXDOP 1 in a comment made rule 3 say "MAXDOP is set to 1 using a query hint"
   • rule 20: RECOMPILE in a comment removed the local-variables warning
   • rule 28: NOT IN in a comment counted toward the NOT IN pattern
   • rule 37: a cursor declaration in a comment was flagged, and LOCAL in a comment counted as the qualifier
   • rule 26: a keyword in a comment was named as the cause of a row goal (message text only)
   • rule 38: MAXDOP 2 in a comment removed the Standard Edition DOP warning (found by the review of this PR)
   • the app's parameters panel: OPTIMIZE FOR UNKNOWN in a comment chose the wrong annotation
4. [BUG] Missing-index rule 30 conflates same-named tables in different databases #578, rule 30 (missing index quality). The rule grouped suggestions by schema and table. So dbo.T in database A and dbo.T in database B looked like duplicates. The rule then recommended that you consolidate them. The group key now includes the database. The message text did not change.

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 helper is internal in PlanAnalyzer.Helpers.cs. PlanViewer.Web.csproj already links that file, so the web build needs no new include. The app can call it because Core already has InternalsVisibleTo for PlanViewer.App.

How was this tested?

• New AnalyzerRuleGateTests: 31 cases, built in code like the issues' repros. The first 26 ran against the unfixed analyzer, and 15 failed. They are every case that reproduces a bug. The later 5 cover rule 38 and the helper itself. The other 11 are controls for behavior that must not change. For example, a real hint still warns, and OPTION (RECOMPILE) still silences rule 20.
• Full suite on the original base: 807 passed, 0 failed, 1 skipped. The warning and comparison golden files did not change, so none of the 60 fixture plans had one of these bugs.
• After a rebase onto dev with Parse IF-condition query plans and MULTIPLE PLAN hashes (#580) #581: the new tests, the parser tests from Parse IF-condition query plans and MULTIPLE PLAN hashes (#580) #581, both golden-file tests and PlanAnalyzerTests pass (159 of 159).
• The Release build has 0 compiler warnings. Windows only.

Checklist

• I have read the contributing guide
• My code builds with zero warnings (Release build through dotnet test)
• 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 the diff against dev (the MaskCommentsAndLiterals helper, its six call sites, and the rule-5/rule-6/rule-30 gate fixes). The logic is solid — bracket/quoted-identifier stepping, doubled-quote escaping, nested block comments, and the -- end-of-line handling all look correct, and the new AnalyzerRuleGateTests cover both the buggy and control cases well (including the nested-comment and bracket-with-embedded-quote edge cases).

One gap: the PR says it audited "the raw statement text" checks and fixed six of them, but Rule 38 (PlanAnalyzer.Statement.cs:410-411, hasMaxdop2Hint) has the identical bug and wasn't touched — it still matches MAXDOP\s+2\b against the unmasked stmt.StatementText. A -- MAXDOP 2 mention in a comment or string literal would suppress the "Standard Edition DOP Limitation" warning the same way MAXDOP 1 in a comment used to suppress rule 3's warning before this fix. Left an inline note attempt but GitHub rejected it since that line isn't part of this diff — flagging here instead. Worth a one-line fix while MaskCommentsAndLiterals is fresh in context:

var hasMaxdop2Hint = Regex.IsMatch(MaskCommentsAndLiterals(stmt.StatementText), @"MAXDOP\s+2\b", RegexOptions.IgnoreCase);

Everything else in the diff (rule 5's ActualExecutions > 0 gate, rule 6's Serial Plan presence check, rule 30's database-qualified grouping key) matches the existing conventions used by rules 11/12/29 and rule 3, respectively, and the "statement rules run before node rules" ordering claim checks out in PlanAnalyzer.cs.

[erikdarlingdata]

Fixed in dcec793. Rule 38 now masks comments and string literals before it matches MAXDOP 2, and a new test covers it. I also searched the whole solution for the same pattern and found one more copy: the app's parameters panel matched OPTIMIZE FOR UNKNOWN in the raw text to choose its annotation. The same commit fixes that.

[claude]

Reviewed the diff (analyzer rule gates for #577, #576, #579, #578 plus the new AnalyzerRuleGateTests). This looks solid — a few things I specifically checked and confirmed:

• Rule 6 ordering claim: verified PlanAnalyzer.cs calls AnalyzeStatement before AnalyzeNodeTree per statement (PlanAnalyzer.cs:110-113), so rule 3's "Serial Plan" finding is on stmt.PlanWarnings by the time rule 6 checks for it. And rule 3's else branch (the non-MaxDOPSetToOne path) does add a "Serial Plan" warning for all three reasons rule 6 checks (TSQLUserDefinedFunctionsNotParallelizable, CLRUserDefinedFunctionRequiresDataAccess, CouldNotGenerateValidParallelPlan), so the new .Any(w => w.WarningType == "Serial Plan") gate is correct.
• MaskCommentsAndLiterals: hand-traced the nested-block-comment and mixed literal/comment/bracket cases against the new test table — the depth-tracking for /* /* */ */, doubled-quote escaping for ', ", [...], and unterminated-string/comment handling all check out. It correctly leaves "..."/[...] content unblanked (identifiers) while only blanking '...' contents and whole comments, consistent with ParameterSubstitution's existing scanner.
• Rule 30 key fix: MissingIndex.Database defaults to "" (never null), so the widened grouping key is safe, and ResultMapper.cs already used the DB-qualified key elsewhere — this closes the one place that didn't.
• Repo conventions: all four touched Core files are already linked into PlanViewer.Web.csproj, so no new <Compile Include> was needed; no version-sensitive files touched; no new NoWarn/pragma; PlanViewer.App already has InternalsVisibleTo from Core so the new internal MaskCommentsAndLiterals call from PlanViewerControl.Parameters.cs is valid.

No correctness, injection, or convention issues found. One non-blocking nit: in PlanAnalyzer.Node.cs Rule 5, the var executions = node.ActualExecutions > 0 ? node.ActualExecutions : 1; fallback (line ~180) is now dead code, since the new outer gate already requires node.ActualExecutions > 0 — harmless, not worth blocking on.

[claude]

Reviewed the diff (fetch of dev for a local build/test run wasn't available in this sandbox, so this is a careful static read plus tracing through MaskCommentsAndLiterals by hand against the test cases).

Summary: looks correct. Walked through each of the four fixes:

• Rule 5 ([BUG] Row-estimate rule 5 warns about resource allocation on operators that never executed #577): gating on node.ActualExecutions > 0 before either branch (zero-actual-rows or ratio comparison) is right — an unexecuted operator's zero rows is not evidence of anything, and dropping the ? node.ActualExecutions : 1 fallback afterward is correctly unreachable now, not a behavior change.
• Rule 6 ([BUG] Disabling serial-plan rule 3 also suppresses scalar-UDF rule 6 #576): switching the suppression check from "is NonParallelPlanReason one of the three UDF reasons" to "...AND rule 3 actually added a Serial Plan warning" is correct — traced through Rule03_SerialPlan's gate (cost < 1, TRIVIAL, 0ms elapsed) and confirmed it's the only source of that warning type, and that AnalyzeStatement runs before AnalyzeNodeTree per statement, so the ordering assumption holds.
• MaskCommentsAndLiterals ([BUG] OPTIMIZE FOR UNKNOWN rule 27 treats string literals and comments as query hints #579): traced the char-array state machine by hand against the new test cases (nested /* /* */ */, unterminated string, doubled-quote escapes, bracket/double-quote identifiers left unblanked vs. '...' contents blanked). Termination is guaranteed (every branch advances i), bounds checks on the doubled-close-quote lookahead are correct, and it matches ParameterSubstitution's existing delimiter/comment scanning conventions. One thing worth being aware of (not asking for a change): "..." is treated as a stepped-over delimited identifier, not a string literal, so a hint keyword inside a double-quoted string literal under QUOTED_IDENTIFIER OFF would still be masked-out incorrectly and could produce a false positive/negative on the affected rules. That's a deliberate, documented tradeoff (comment says as much) and matches ParameterSubstitution's own assumption, so it's consistent with the rest of the codebase rather than a new gap.
• Rule 30 ([BUG] Missing-index rule 30 conflates same-named tables in different databases #578): grouping key now includes mi.Database; MissingIndex.Database defaults to "" so ungrouped/missing-database plans behave the same as before. Message text intentionally unchanged, matching the PR description.

Conventions checked: no TRY_CONVERT introduced, no string-built T-SQL, PlanAnalyzer.Helpers.cs/.Node.cs/.Statement.cs/.Detection.cs were already linked into PlanViewer.Web.csproj (no new file added there, so no missing-link risk), MaskCommentsAndLiterals is internal and PlanViewer.Core.csproj already has InternalsVisibleTo for PlanViewer.App. PlanViewer.Ssms isn't touched, so the manual version-bump convention doesn't apply here.

Test coverage: the new AnalyzerRuleGateTests directly exercises each fixed gate plus the helper itself (nesting, escapes, unterminated string), and includes negative/control cases (real hints still fire, OPTION(RECOMPILE) still silences rule 20). Good coverage for what changed.

I wasn't able to run dotnet build/dotnet test in this sandbox (network/build commands required approval I couldn't grant myself), so I can't independently confirm the "0 warnings, 807/159 tests passing" claims in the PR description beyond the static trace above.