Fix source generator diagnostics to support #pragma warning disable#124994
Fix source generator diagnostics to support #pragma warning disable#124994eiriktsarpalis wants to merge 29 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Enables Roslyn pragma-based suppression (#pragma warning disable) for diagnostics emitted by several incremental source generators by separating source emission from diagnostic emission and recreating diagnostics with SourceFile locations recovered from the current Compilation.
Changes:
- Split generator pipelines into (1) fully-incremental source generation and (2) diagnostics combined with
CompilationProviderfor pragma-suppressible locations. - Add
DiagnosticInfo.CreateDiagnostic(Compilation)(and Regex-specific equivalent) to rebuildLocationasLocationKind.SourceFile. - Update Json source generator incremental test commentary to reflect the new diagnostics pipeline behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.cs | Adds a diagnostics-only pipeline and recreates diagnostics with SourceFile locations. |
| src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorIncrementalTests.cs | Adjusts incremental-model encapsulation test commentary given diagnostics now reference SyntaxTree. |
| src/libraries/System.Text.Json/gen/JsonSourceGenerator.Roslyn4.0.cs | Splits Json SG into separate source + diagnostics pipelines; diagnostics now created with compilation context. |
| src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Roslyn4.0.cs | Splits LoggerMessage SG pipelines and preserves diagnostic deduping while making locations pragma-suppressible. |
| src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs | Splits binder SG pipelines and reports diagnostics using compilation-backed SourceFile locations. |
| src/libraries/Common/src/SourceGenerators/DiagnosticInfo.cs | Adds CreateDiagnostic(Compilation) to recreate pragma-suppressible SourceFile locations from trimmed locations. |
src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Roslyn4.0.cs
Outdated
Show resolved
Hide resolved
...on/tests/System.Text.Json.SourceGeneration.Unit.Tests/JsonSourceGeneratorIncrementalTests.cs
Outdated
Show resolved
Hide resolved
effaba3 to
50000b3
Compare
Split each affected generator's RegisterSourceOutput pipeline into two separate pipelines: 1. Source generation pipeline - fully incremental, uses Select to extract just the equatable model. Only re-fires on structural changes. 2. Diagnostic pipeline - combines with CompilationProvider to recover the SyntaxTree from the Compilation at emission time. Uses Location.Create(SyntaxTree, TextSpan) to produce SourceLocation instances that support pragma suppression checks. Affected generators: - System.Text.Json (JsonSourceGenerator) - Microsoft.Extensions.Logging (LoggerMessageGenerator) - Microsoft.Extensions.Configuration.Binder (ConfigurationBindingGenerator) - System.Text.RegularExpressions (RegexGenerator) The shared DiagnosticInfo type gains a CreateDiagnostic(Compilation) overload that recovers the SyntaxTree from the trimmed ExternalFileLocation's file path, converting it back to a SourceLocation. The RegexGenerator's private DiagnosticData type gets an analogous ToDiagnostic(Compilation) overload. Fixes #92509 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
50000b3 to
e95c4ad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs:66
- The catch block rethrows with
throw ex;, which resets the exception stack trace. Usethrow;(or remove the catch entirely) to preserve the original call stack for diagnosing generator failures.
catch (Exception ex)
{
throw ex;
}
...raries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs
Show resolved
Hide resolved
…tors Verifies that diagnostics from all 4 affected source generators (Regex, JSON, Logger, ConfigBinder) have LocationKind.SourceFile, which is the prerequisite for #pragma warning disable to work. Before this fix, diagnostics had LocationKind.ExternalFile which bypasses Roslyn's pragma suppression checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds RegexGeneratorIncrementalTests with 4 tests following the patterns established by JsonSourceGeneratorIncrementalTests and ConfigBinder's GeneratorTests.Incremental.cs: - SameInput_DoesNotRegenerate: verifies caching on identical compilations - EquivalentSources_Regenerates: documents that semantically equivalent sources trigger regeneration (pre-existing limitation due to Dictionary in model lacking value equality) - DifferentSources_Regenerates: verifies model changes trigger output - SourceGenModelDoesNotEncapsulateSymbolsOrCompilationData: walks the object graph to ensure no Compilation/ISymbol references leak Also adds SourceGenerationTrackingName constant and WithTrackingName() to the source pipeline to enable step tracking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs:66
- The
catch (Exception ex) { throw ex; }pattern resets the original stack trace. If the intent is just to propagate, usethrow;(or remove the try/catch entirely) so failures in the generator preserve useful call stacks.
catch (Exception ex)
{
throw ex;
}
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs
Outdated
Show resolved
Hide resolved
Replace inline lambda callbacks in the diagnostic pipelines with named EmitDiagnostics static methods, complementing the existing EmitSource methods in all 4 generators (JSON, Logger, ConfigBinder, Regex). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
...aries/System.Text.RegularExpressions/tests/FunctionalTests/RegexGeneratorIncrementalTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
Create named IncrementalValueProvider variables for the equatable model projections in all 4 generators, with detailed comments explaining how Roslyn's Select operator uses model equality to guard source production. For the Regex generator, also extract the source emission lambda into a named EmitSource method, complementing EmitDiagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3323beb to
a4e500e
Compare
Create named IncrementalValueProvider variables for the diagnostic projections in all 4 generators, with comments explaining that ImmutableArray<Diagnostic> uses reference equality in the incremental pipeline — the callback fires on every compilation change by design. This also simplifies the EmitDiagnostics signatures to accept just the projected diagnostics rather than the full model+diagnostics tuple. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs:66
- Avoid
throw ex;here; it resets the original stack trace and makes failures harder to diagnose. Usethrow;to preserve the stack trace, or remove the try/catch entirely if it's only rethrowing.
catch (Exception ex)
{
throw ex;
}
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs:66
- The
catch (Exception ex) { throw ex; }rethrow resets the original stack trace and also adds no value here. Usethrow;to preserve the stack (or remove the try/catch entirely and let the exception bubble).
catch (Exception ex)
{
throw ex;
}
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Parser.cs
Show resolved
Hide resolved
…gnostics - Build filtered ImmutableArray<object> in Collect().Select() excluding pure Diagnostic entries and stripping Diagnostic from limited-support tuples - Reinstate ObjectImmutableArraySequenceEqualityComparer for element-wise equality on the source model projection - Lift source model IVP into named variable with explanatory comments - Update downstream tuple patterns and item indices accordingly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs:66
- The
catch (Exception ex) { throw ex; }pattern resets the original stack trace, making failures much harder to diagnose. Usethrow;to preserve the stack trace (or remove the try/catch if it isn't adding handling).
catch (Exception ex)
{
throw ex;
}
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexGeneratorParserTests.cs
Show resolved
Hide resolved
Thread Location separately through the pipeline tuples so it does not participate in record equality. This improves incremental caching since Location instances are not value-comparable. The location is only used for creating Diagnostic objects in the pipeline and is discarded in the Collect phase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…exMethod, Location) tuple The Location is now fully consumed in the first Select for diagnostic creation and not propagated further. The second Select receives either a bare RegexMethod (for full codegen) or a Diagnostic/limited-support tuple to pass through. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs:66
throw ex;resets the stack trace and makes failures harder to diagnose. Either remove this try/catch (letting the original exception propagate) or use a barethrow;to preserve the original stack trace.
catch (Exception ex)
{
throw ex;
}
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Parser.cs
Show resolved
Hide resolved
RegexPatternAndSyntax is not part of the incremental model — it is consumed in the first Select and never reaches the Collect phase. Restoring DiagnosticLocation simplifies the first Select by removing the (RegexPatternAndSyntax, Location) tuple indirection. RegexMethod remains Location-free since it is part of the cached model. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
You can also share your feedback on Copilot code review. Take the survey.
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.cs
Show resolved
Hide resolved
ericstj
left a comment
There was a problem hiding this comment.
Do we need to do anything in the interop generators?
| <Compile Include="$(CommonPath)\Roslyn\DiagnosticDescriptorHelper.cs" Link="Common\Roslyn\DiagnosticDescriptorHelper.cs" /> | ||
| <Compile Include="$(CommonPath)\Roslyn\GetBestTypeByMetadataName.cs" Link="Common\Roslyn\GetBestTypeByMetadataName.cs" /> | ||
| <Compile Include="$(CommonPath)\SourceGenerators\CSharpSyntaxUtilities.cs" Link="Common\SourceGenerators\CSharpSyntaxUtilities.cs" /> | ||
| <Compile Include="$(CommonPath)\SourceGenerators\DiagnosticInfo.cs" Link="Common\SourceGenerators\DiagnosticInfo.cs" /> |
There was a problem hiding this comment.
Can you please remove this file from the other generators' targets files?
| { | ||
| ImmutableArray<(LoggerClassSpec, bool)>.Builder? specs = null; | ||
| ImmutableArray<Diagnostic>.Builder? diagnostics = null; | ||
| HashSet<(string Id, TextSpan? Span, string? FilePath)>? seen = null; |
There was a problem hiding this comment.
Should this include message as well?
| Assert.True(result.Diagnostics.Any(diag => diag.Id == Diagnostics.PropertyNotSupported.Id)); | ||
| } | ||
|
|
||
| [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))] |
There was a problem hiding this comment.
Consider some additional test cases, not sure how much our generator implementation would actually be exposed to these differences of it's more roslyn but sharing some thoughts:
- No pragma does not supporess.
- Multiple diagnostics with only some suppressed.
- Pragma suppress/restore in same file, but outside the reported code span. Perhaps all around it, so it would capture if we somehow got the span line info off-by-1.
ericstj
left a comment
There was a problem hiding this comment.
Overall looks good and surprisingly minimal. Feedback is straightforward fix or non-blocking.
Summary
Source generator diagnostics emitted by incremental generators in dotnet/runtime can now be suppressed using
#pragma warning disableinline directives.Fixes #92509
Problem
Incremental source generators that store diagnostic locations in equatable intermediate representations "trim" the
Locationto avoid holding references to theCompilationobject. The standard workaround —Location.Create(filePath, textSpan, linePositionSpan)— creates anExternalFileLocation(LocationKind.ExternalFile) which bypasses Roslyn's pragma suppression checks. OnlySourceLocation(LocationKind.SourceFile) instances, created viaLocation.Create(SyntaxTree, TextSpan), are checked against theSyntaxTree's pragma directive table.This is the issue described in dotnet/roslyn#68291.
Solution
Following the technique first applied in eiriktsarpalis/PolyType#401, split each affected generator's
RegisterSourceOutputpipeline into two separate pipelines:Selectto extract just the equatable model (which contains noLocation/SyntaxTreereferences), deduplicates by model equality, only re-fires on structural changes.Diagnosticobjects that preserve the originalSourceLocation(LocationKind.SourceFile) from the syntax tree. This enables Roslyn to check theSyntaxTree's pragma directive table for#pragma warning disabledirectives.Parsers now create
Diagnostic.Create(descriptor, location, ...)directly instead of wrapping inDiagnosticInfo.Create(...)which trimmed the location. SinceImmutableArray<Diagnostic>does not have value equality, the diagnostic pipeline fires more frequently but the work is trivially cheap (just iterating and reporting).