Migrate LoggerMessageGenerator to IIncrementalGenerator #58068
Conversation
This reduces the time spent in the background in VS running the source generator, since we only need to respond to methods that have the LoggerMessageAttribute on them. Contributes to dotnet#56702
|
Tagging subscribers to this area: @maryamariyan Issue DetailsThis reduces the time spent in the background in VS running the source generator, since we only need to respond to methods that have the LoggerMessageAttribute on them. Contributes to #56702 Note: The current plan is to enable multi-targeting in a separate PR, once the design for how to multi-target Roslyn Components is solidified. We are considering using the IIncrementalGenerator for RC1 in order to get feedback before doing the multi-targeting work. cc @jaredpar
|
| @@ -28,13 +30,42 @@ public Parser(Compilation compilation, Action<Diagnostic> reportDiagnostic, Canc | |||
| _reportDiagnostic = reportDiagnostic; | |||
| } | |||
|
|
|||
| internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) => | |||
| node is MethodDeclarationSyntax m && m.AttributeLists.Count > 0; | |||
There was a problem hiding this comment.
| node is MethodDeclarationSyntax m && m.AttributeLists.Count > 0; | |
| node is MethodDeclarationSyntax { AttributeLists.Count: > 0 }; |
There was a problem hiding this comment.
IMO this falls into a "preference / style" category, and I don't think the suggested change reads well.
There was a problem hiding this comment.
@stephentoub do you have a preference on this sugar?
There was a problem hiding this comment.
I'm fine with either style here (as long as they produce the same IL, which I believe they do). The pattern matching syntax is newer and takes some getting used to, and in a case like this, it doesn't buy you a whole lot; its benefit becomes more apparent when there are more things being checked, deeper levels of nesting being examined, etc.
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (context.SyntaxReceiver is not SyntaxReceiver receiver || receiver.ClassDeclarations.Count == 0) | ||
| if (classes.IsDefaultOrEmpty) |
There was a problem hiding this comment.
| if (classes.IsDefaultOrEmpty) | |
| if (classes.IsEmpty) |
There was a problem hiding this comment.
Why NullRef here when someone passes in an uninitialized ImmutableArray? Is this guaranteed to never have a null backing array?
| { | ||
| // nothing to do yet | ||
| return; | ||
| } | ||
|
|
||
| var p = new Parser(context.Compilation, context.ReportDiagnostic, context.CancellationToken); | ||
| IReadOnlyList<LoggerClass> logClasses = p.GetLogClasses(receiver.ClassDeclarations); | ||
| IEnumerable<ClassDeclarationSyntax> distinctClasses = classes.Distinct(); |
There was a problem hiding this comment.
Yes. The Parser is currently written such that it expects distinct ClassDeclarations passed into it. I didn't want to re-write it completely since we are planning on multi-targeting with Roslyn 3.9 as well.
| IncrementalValueProvider<(Compilation, ImmutableArray<ClassDeclarationSyntax>)> compilationAndClasses = | ||
| context.CompilationProvider.Combine(classDeclarations.Collect()); | ||
|
|
||
| context.RegisterSourceOutput(compilationAndClasses, (spc, source) => Execute(source.Item1, source.Item2, spc)); |
There was a problem hiding this comment.
@chsienki Is this a case where you would recommend non-semantic source output?
There was a problem hiding this comment.
What does "non-semantic source output" mean?
If it means using RegisterImplementationSourceOutput, that doesn't work for this generator because this generator fills out partial methods. If we use RegisterImplementationSourceOutput, then the IDE doesn't see the implementation being filled out and you get red squiggles like the following:
Even though the "full build" succeeds.
| { | ||
| // nothing to do yet | ||
| return; | ||
| } | ||
|
|
||
| var p = new Parser(context.Compilation, context.ReportDiagnostic, context.CancellationToken); | ||
| IReadOnlyList<LoggerClass> logClasses = p.GetLogClasses(receiver.ClassDeclarations); | ||
| IEnumerable<ClassDeclarationSyntax> distinctClasses = classes.Distinct(); |
There was a problem hiding this comment.
just curious as I haven't written against this API, why are there duplicates here? Unfortunate we have to new up a HashSet etc.
There was a problem hiding this comment.
Because the Parser.IsSyntaxTargetForGeneration is keying off of MethodDeclarations that have LoggerMessageAttributes applied to them. So we are filtering down to the methods we care about. But the Parser is currently written in terms of ClassDeclarations, and I didn't want to completely re-write the Parser since the plan is to multi-target against Roslyn 3.9. If the Parser was re-written, we wouldn't be able to share as much code between the two targets.
There was a problem hiding this comment.
Even if I did re-write the Parser, we would still need to map MethodDeclaration => ClassDeclaration => LoggerClass, which would probably involve a Dictionary. So we'd still need to have a hash of them.
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.cs
Show resolved
Hide resolved
...ons/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
Show resolved
Hide resolved
|
/backport to release/6.0-rc1 |
|
Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1174845336 |
ghost
locked as resolved and limited conversation to collaborators
This reduces the time spent in the background in VS running the source generator, since we only need to respond to methods that have the LoggerMessageAttribute on them.
Contributes to #56702
Note: The current plan is to enable multi-targeting in a separate PR, once the design for how to multi-target Roslyn Components is solidified. We are considering using the IIncrementalGenerator for RC1 in order to get feedback before doing the multi-targeting work.
cc @jaredpar