Run every registered code action, not just the applied one - #1353
Merged
Conversation
The "Remove the ToString call" branch did not exclude Insert, unlike the string.Format and string.Join branches next to it, so sb.Insert(0, value.ToString()) was reported. The fix then threw InvalidCastException: it reads Arguments[0], which is the index for Insert, not the value, and the analyzer had looked at Arguments[1]. Fixing the argument index alone would not be enough. The fix builds an Append call, so it would turn an Insert into an Append and drop the index, and the types the analyzer accepts are the ones that have an Append overload: StringBuilder and ReadOnlyMemory<char> have no Insert counterpart, so removing the ToString call would not compile. Exclude Insert, as the two sibling branches already do. Supporting it would need its own set of overload types and its own fix.
VerifyFix only runs when a test calls ShouldFixCodeWith or ShouldBatchFixCodeWith, so WithCodeFixProvider is a no-op for every other test. 355 of the 989 test methods that expect a diagnostic configure a provider that is never invoked, even though those snippets are inputs the provider does run on in an IDE. When no expected fixed code is declared, call RegisterCodeFixesAsync on each reported diagnostic the provider declares as fixable, and let an exception fail the test. The registered actions are not applied, since the test did not say what the fixed code should be. VerifyDiagnostics now returns the documents and diagnostics it already computed so the registration reuses them instead of recreating the project, which keeps the cost of the suite unchanged (1m00s before and after, roslyn5.9). No existing test fails, on any of the five Roslyn versions: this is a regression guard rather than a fix for a current crash.
Registering the fixes only runs RegisterCodeFixesAsync; the delegate a provider registers, where the fix actually builds the new document, is never invoked. And a provider often registers several actions, of which VerifyFix applies exactly one, so the others were never exercised either. Compute the operations of every registered action, in both paths, and let an exception fail the test. The changes are still not applied when the test declares no expected fixed code, as there is nothing to compare them to.
meziantou
force-pushed
the
test/execute-configured-code-fixers
branch
from
August 27, 2026 03:34
f0497c4 to
f6ac4b6
Compare
This was referenced Aug 27, 2026
Closed
Bump Meziantou.Analyzer from 3.0.139 to 3.0.189
Analogy-LogViewer/Analogy.LogViewer.NLog.Targets#556
Closed
Closed
Bump Meziantou.Analyzer from 3.0.139 to 3.0.189
Analogy-LogViewer/Analogy.AspNetCore.LogProvider#542
Closed
Closed
This was referenced Aug 31, 2026
Open
Open
Open
Open
Open
Open
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two gaps meant most code fix providers were never really exercised.
1. A provider configured without an expected fixed code was never invoked at all.
ValidateAsynconly callsVerifyFixwhenExpectedFixedCode is not null, so.WithCodeFixProvider<T>()is a no-op otherwise. 355 of the 989 test methods that expect a diagnostic configure a provider that never runs:DoNotUseBlockingCallInAsyncContextAnalyzer_AsyncContextTestsDoNotUseImplicitCultureSensitiveToStringAnalyzerTestsAvoidUnusedInternalTypesAnalyzerTestsUseIFormatProviderAnalyzerTestsUseStringComparerAnalyzerTestsNamedParameterAnalyzerTests2. Only one action per diagnostic was ever run. A provider commonly registers several actions;
VerifyFixappliesactions[codeFixIndex ?? 0]and ignores the rest, so the siblings were unexercised even in tests that do assert a fixed code. And registering a fix does not run it — the delegate passed toCodeAction.Create, where the fix actually builds the new document, only runs when its operations are computed. That delegate is where the casts, the argument indexing and the rewriting live.Change
The changes are still not applied when the test declares no expected fixed code — there is nothing to compare them against. Diagnostics are matched to their document by syntax tree, so a diagnostic from an API-reference document is not handed to a provider along with the wrong document.
VerifyDiagnosticsnow returns the documents and diagnostics it already computed so nothing is recomputed.GetSortedDiagnosticshad one caller and is inlined into it.It found a real bug immediately
The first run surfaced an
InvalidCastExceptioninOptimizeStringBuilderUsageFixer.RemoveToStringforsb.Insert(0, 10.ToString())— the fixer readsArguments[0], which is the index forInsert, not the value. Fixed in #1354, which this is stacked on.Note the earlier version of this PR, which only called
RegisterCodeFixesAsync, would not have caught it: the crash is in the action body.One test had to change
Test_LongElseIfChainWhereEveryBranchJumps_AllElsesReported(added in #1352) now builds an analyzer-onlyProjectBuilder. Anelse ifchain is nested syntax, so its 1000 branches are a 1000-level deep tree, and the formatter Roslyn runs while a code action computes its changes recurses once per level and throwsInsufficientExecutionStackException. That is a Roslyn limit on deeply nested syntax, not a defect in the fixer, and the test exists to guard the analyzer's complexity.What this does and does not catch
Catches a provider that throws on a shape no fix assertion covers — unguarded casts, argument-index arithmetic,
Single()on an empty sequence.Does not catch a provider that produces wrong-but-compiling output; that still needs a real
ShouldFixCodeWith. The MA0090 bug in #1351 was of that second kind and would not have been caught here.Cost
Full
roslyn5.9suite: 59.9s on the base branch, 1m02s with this change — about +3s (5%).All five versions pass: 3708 (4.8) / 3756 (4.14) / 3783 (5.0) / 3807 (5.6) / 3840 (5.9), zero failures.
Not in this PR
Two related harness gaps, left out to keep this reviewable:
ValidateAsync's opening guards are dead code —DiagnosticAnalyzerandExpectedDiagnosticResultsareIList<>initialized to[], sois nullis never true and a test that forgets.WithAnalyzer<T>()passes silently.ParseSourceCodedrops a malformed[|…|]marker without warning, turning a positive test into a vacuous negative one.A stricter follow-up is also possible: assert that a registered action actually changes the document, which would enforce the
AGENTS.mdrule against registering a fix that returns the document unchanged. That one will surface real failures, so it is worth doing separately.