Correctly supress/elevate diagnostics produced by source generators #52544
Conversation
| @@ -438,6 +440,20 @@ private static GeneratorState SetGeneratorException(CommonMessageProvider provid | |||
| return new GeneratorState(generatorState.Info, e, diagnostic); | |||
| } | |||
|
|
|||
| private static ImmutableArray<Diagnostic> FilterDiagnostics(Compilation compilation, ImmutableArray<Diagnostic> diagnostics, CancellationToken cancellationToken) | |||
| { | |||
| DiagnosticBag filteredDiagnostics = DiagnosticBag.GetInstance(); | |||
There was a problem hiding this comment.
A DiagnosticBag instance is somewhat heavy weight because it's designed for concurrent writes. In this case we're just building up a collection in a single threaded environment and returning it to the caller. Consider using a normal ImmutableArrayBuilder here.
Given that this is immediately consumed and written into a different bag there isn't really a need for the return to be immutable here if that adds extra cost.
There was a problem hiding this comment.
Thanks. Changed to ArrayBuilder but also passing in the overal driver bag, so we can just add them as we go rather than adding them as a bunch at the end.
src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Semantic/SourceGeneration/GeneratorDriverTests.cs
Show resolved
Hide resolved
- Simplify diag filterning - add vb tests
|
@dotnet/roslyn-compiler for a second review please |
| (var sources, var diagnostics) = context.ToImmutableAndFree(); | ||
| stateBuilder[i] = new GeneratorState(generatorState.Info, generatorState.PostInitTrees, ParseAdditionalSources(generator, sources, cancellationToken), diagnostics); | ||
| diagnosticsBag?.AddRange(diagnostics); | ||
| (var sources, var generatorDiagnostics) = context.ToImmutableAndFree(); |
There was a problem hiding this comment.
it would be nice if we could avoid the allocation (just allocate the final array of filtered diagnostics, not the intermediate one), but I am not extremely concerned about it.
There was a problem hiding this comment.
@RikkiGibson Yeah I actually thought about this, but with the upcoming work we'll be collecting diagnostics from multiple places. I figured it made sense to centralize the filtering upfront now, rather than having it spread across multiple places down the line.
ghost
modified the milestones:
|
@chsienki - I'm still seeing warnings in the Error List using VS What version did this fix make it into? |
We were not filtering the diagnostics produced by source generators, meaning there was no way for a user to suppress them.
Fixes #52527