-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Migrate LoggerMessageGenerator to IIncrementalGenerator #58068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,8 @@ public partial class LoggerMessageGenerator | |||||
| { | ||||||
| internal class Parser | ||||||
| { | ||||||
| private const string LoggerMessageAttribute = "Microsoft.Extensions.Logging.LoggerMessageAttribute"; | ||||||
|
|
||||||
| private readonly CancellationToken _cancellationToken; | ||||||
| private readonly Compilation _compilation; | ||||||
| private readonly Action<Diagnostic> _reportDiagnostic; | ||||||
|
|
@@ -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; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO this falls into a "preference / style" category, and I don't think the suggested change reads well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stephentoub do you have a preference on this sugar?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| internal static ClassDeclarationSyntax? GetSemanticTargetForGeneration(GeneratorSyntaxContext context) | ||||||
| { | ||||||
| var methodDeclarationSyntax = (MethodDeclarationSyntax)context.Node; | ||||||
|
|
||||||
| foreach (AttributeListSyntax attributeListSyntax in methodDeclarationSyntax.AttributeLists) | ||||||
| { | ||||||
| foreach (var attributeSyntax in attributeListSyntax.Attributes) | ||||||
|
eerhardt marked this conversation as resolved.
Outdated
|
||||||
| { | ||||||
| IMethodSymbol attributeSymbol = context.SemanticModel.GetSymbolInfo(attributeSyntax).Symbol as IMethodSymbol; | ||||||
| if (attributeSymbol == null) | ||||||
| { | ||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| INamedTypeSymbol attributeContainingTypeSymbol = attributeSymbol.ContainingType; | ||||||
| string fullName = attributeContainingTypeSymbol.ToDisplayString(); | ||||||
|
|
||||||
| if (fullName == LoggerMessageAttribute) | ||||||
| { | ||||||
| return methodDeclarationSyntax.Parent as ClassDeclarationSyntax; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
eerhardt marked this conversation as resolved.
Outdated
|
||||||
| } | ||||||
|
|
||||||
| return null; | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Gets the set of logging classes containing methods to output. | ||||||
| /// </summary> | ||||||
| public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSyntax> classes) | ||||||
| { | ||||||
| const string LoggerMessageAttribute = "Microsoft.Extensions.Logging.LoggerMessageAttribute"; | ||||||
|
|
||||||
| INamedTypeSymbol loggerMessageAttribute = _compilation.GetTypeByMetadataName(LoggerMessageAttribute); | ||||||
| if (loggerMessageAttribute == null) | ||||||
| { | ||||||
|
|
@@ -442,11 +473,11 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt | |||||
| LoggerClass currentLoggerClass = lc; | ||||||
| var parentLoggerClass = (classDec.Parent as TypeDeclarationSyntax); | ||||||
|
|
||||||
| bool IsAllowedKind(SyntaxKind kind) => | ||||||
| bool IsAllowedKind(SyntaxKind kind) => | ||||||
| kind == SyntaxKind.ClassDeclaration || | ||||||
| kind == SyntaxKind.StructDeclaration || | ||||||
|
eerhardt marked this conversation as resolved.
|
||||||
| kind == SyntaxKind.RecordDeclaration; | ||||||
|
|
||||||
| while (parentLoggerClass != null && IsAllowedKind(parentLoggerClass.Kind())) | ||||||
| { | ||||||
| currentLoggerClass.ParentClass = new LoggerClass | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,10 @@ | |||||
|
|
||||||
| using System; | ||||||
| using System.Collections.Generic; | ||||||
| using System.Collections.Immutable; | ||||||
| using System.Diagnostics.CodeAnalysis; | ||||||
| using System.Diagnostics.Tracing; | ||||||
| using System.Linq; | ||||||
| using System.Runtime.CompilerServices; | ||||||
| using System.Text; | ||||||
| using Microsoft.CodeAnalysis; | ||||||
|
|
@@ -15,50 +18,38 @@ | |||||
| namespace Microsoft.Extensions.Logging.Generators | ||||||
| { | ||||||
| [Generator] | ||||||
| public partial class LoggerMessageGenerator : ISourceGenerator | ||||||
| public partial class LoggerMessageGenerator : IIncrementalGenerator | ||||||
| { | ||||||
| [ExcludeFromCodeCoverage] | ||||||
| public void Initialize(GeneratorInitializationContext context) | ||||||
| public void Initialize(IncrementalGeneratorInitializationContext context) | ||||||
| { | ||||||
| context.RegisterForSyntaxNotifications(SyntaxReceiver.Create); | ||||||
| IncrementalValuesProvider<ClassDeclarationSyntax> classDeclarations = context.SyntaxProvider | ||||||
| .CreateSyntaxProvider((s, _) => Parser.IsSyntaxTargetForGeneration(s), (ctx, _) => Parser.GetSemanticTargetForGeneration(ctx)) | ||||||
| .Where(m => m is not null); | ||||||
|
eerhardt marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| IncrementalValueProvider<(Compilation, ImmutableArray<ClassDeclarationSyntax>)> compilationAndClasses = | ||||||
| context.CompilationProvider.Combine(classDeclarations.Collect()); | ||||||
|
|
||||||
| context.RegisterSourceOutput(compilationAndClasses, (spc, source) => Execute(source.Item1, source.Item2, spc)); | ||||||
|
eerhardt marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chsienki Is this a case where you would recommend non-semantic source output?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Even though the "full build" succeeds. |
||||||
| } | ||||||
|
|
||||||
| [ExcludeFromCodeCoverage] | ||||||
| public void Execute(GeneratorExecutionContext context) | ||||||
| private void Execute(Compilation compilation, ImmutableArray<ClassDeclarationSyntax> classes, SourceProductionContext context) | ||||||
| { | ||||||
| if (context.SyntaxReceiver is not SyntaxReceiver receiver || receiver.ClassDeclarations.Count == 0) | ||||||
| if (classes.IsDefaultOrEmpty) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||||||
| { | ||||||
| // 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(); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😕 Is this necessary?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just curious as I haven't written against this API, why are there duplicates here? Unfortunate we have to new up a HashSet etc.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if I did re-write the Parser, we would still need to map |
||||||
|
|
||||||
| var p = new Parser(compilation, context.ReportDiagnostic, context.CancellationToken); | ||||||
| IReadOnlyList<LoggerClass> logClasses = p.GetLogClasses(distinctClasses); | ||||||
| if (logClasses.Count > 0) | ||||||
| { | ||||||
| var e = new Emitter(); | ||||||
| string result = e.Emit(logClasses, context.CancellationToken); | ||||||
|
|
||||||
| context.AddSource("LoggerMessage.g.cs", SourceText.From(result, Encoding.UTF8)); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| [ExcludeFromCodeCoverage] | ||||||
| private sealed class SyntaxReceiver : ISyntaxReceiver | ||||||
| { | ||||||
| internal static ISyntaxReceiver Create() | ||||||
| { | ||||||
| return new SyntaxReceiver(); | ||||||
| } | ||||||
|
|
||||||
| public List<ClassDeclarationSyntax> ClassDeclarations { get; } = new (); | ||||||
|
|
||||||
| public void OnVisitSyntaxNode(SyntaxNode syntaxNode) | ||||||
| { | ||||||
| if (syntaxNode is ClassDeclarationSyntax classSyntax) | ||||||
| { | ||||||
| ClassDeclarations.Add(classSyntax); | ||||||
| } | ||||||
| context.AddSource("LoggerMessage.g.cs", SourceText.From(result, Encoding.UTF8)); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||

Uh oh!
There was an error while loading. Please reload this page.