Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate LoggerMessageGenerator to IIncrementalGenerator #58068

Merged
merged 3 commits into from
Aug 27, 2021
Merged

Migrate LoggerMessageGenerator to IIncrementalGenerator #58068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8344ddf
Migrate LoggerMessageGenerator to IIncrementalGenerator
eerhardt Aug 24, 2021
a1916c7
PR feedback
eerhardt Aug 24, 2021
9413dab
PR feedback
eerhardt Aug 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,9 @@
<ProjectServicingConfiguration Include="Microsoft.NETCore.App.Ref" PatchVersion="0" />
</ItemGroup>
<PropertyGroup>
<!-- For source generator support we need to target a pinned version in order to be able to run on older versions of Roslyn -->
<MicrosoftCodeAnalysisCSharpWorkspacesVersion>3.9.0</MicrosoftCodeAnalysisCSharpWorkspacesVersion>
<MicrosoftCodeAnalysisVersion>3.9.0</MicrosoftCodeAnalysisVersion>
<!-- For source generator support we are targeting the latest version of Roslyn for now, until we can support multi-targeting -->
<MicrosoftCodeAnalysisCSharpWorkspacesVersion>4.0.0-3.final</MicrosoftCodeAnalysisCSharpWorkspacesVersion>
<MicrosoftCodeAnalysisVersion>4.0.0-3.final</MicrosoftCodeAnalysisVersion>
</PropertyGroup>
<PropertyGroup>
<!-- Code analysis dependencies -->
Expand Down
7 changes: 4 additions & 3 deletions src/libraries/Common/tests/SourceGenerators/RoslynTestUtils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,9 @@ public static TextSpan MakeSpan(string text, int spanNum)
/// Runs a Roslyn generator over a set of source files.
/// </summary>
public static async Task<(ImmutableArray<Diagnostic>, ImmutableArray<GeneratedSourceResult>)> RunGenerator(
ISourceGenerator generator,
IIncrementalGenerator generator,
IEnumerable<Assembly>? references,
IEnumerable<string> sources,
AnalyzerConfigOptionsProvider? optionsProvider = null,
bool includeBaseReferences = true,
CancellationToken cancellationToken = default)
{
Expand All @@ -156,7 +155,9 @@ public static TextSpan MakeSpan(string text, int spanNum)

Compilation? comp = await proj!.GetCompilationAsync(CancellationToken.None).ConfigureAwait(false);

CSharpGeneratorDriver cgd = CSharpGeneratorDriver.Create(new[] { generator }, optionsProvider: optionsProvider);
// workaround https://github.com/dotnet/roslyn/pull/55866. We can remove "LangVersion=Preview" when we get a Roslyn build with that change.
CSharpParseOptions options = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Preview);
CSharpGeneratorDriver cgd = CSharpGeneratorDriver.Create(new[] { generator.AsSourceGenerator() }, parseOptions: options);
GeneratorDriver gd = cgd.RunGenerators(comp!, cancellationToken);

GeneratorDriverRunResult r = gd.GetRunResult();
Expand Down
38 changes: 34 additions & 4 deletions src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -28,13 +30,41 @@ public Parser(Compilation compilation, Action<Diagnostic> reportDiagnostic, Canc
_reportDiagnostic = reportDiagnostic;
}

internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) =>
node is MethodDeclarationSyntax m && m.AttributeLists.Count > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
node is MethodDeclarationSyntax m && m.AttributeLists.Count > 0;
node is MethodDeclarationSyntax { AttributeLists.Count: > 0 };

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub do you have a preference on this sugar?

Copy link
Member

Choose a reason for hiding this comment

The 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 (AttributeSyntax attributeSyntax in attributeListSyntax.Attributes)
{
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;
}
}
}

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)
{
Expand Down Expand Up @@ -442,11 +472,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.
Show resolved Hide resolved
kind == SyntaxKind.RecordDeclaration;

while (parentLoggerClass != null && IsAllowedKind(parentLoggerClass.Kind()))
{
currentLoggerClass.ParentClass = new LoggerClass
Expand Down
49 changes: 20 additions & 29 deletions src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(static (s, _) => Parser.IsSyntaxTargetForGeneration(s), static (ctx, _) => Parser.GetSemanticTargetForGeneration(ctx))
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
.Where(static m => m is not null);

IncrementalValueProvider<(Compilation, ImmutableArray<ClassDeclarationSyntax>)> compilationAndClasses =
context.CompilationProvider.Combine(classDeclarations.Collect());

context.RegisterSourceOutput(compilationAndClasses, static (spc, source) => Execute(source.Item1, source.Item2, spc));
}

[ExcludeFromCodeCoverage]
public void Execute(GeneratorExecutionContext context)
private static void Execute(Compilation compilation, ImmutableArray<ClassDeclarationSyntax> classes, SourceProductionContext context)
{
if (context.SyntaxReceiver is not SyntaxReceiver receiver || receiver.ClassDeclarations.Count == 0)
if (classes.IsDefaultOrEmpty)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (classes.IsDefaultOrEmpty)
if (classes.IsEmpty)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 Is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 MethodDeclaration => ClassDeclaration => LoggerClass, which would probably involve a Dictionary. So we'd still need to have a hash of them.


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));
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion .../tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ public class Object {}
public class Void {}
public class String {}
public struct DateTime {}
public abstract class Attribute {}
eerhardt marked this conversation as resolved.
Show resolved Hide resolved
}
namespace System.Collections
{
Expand All @@ -392,10 +393,12 @@ public interface ILogger {}
}
namespace Microsoft.Extensions.Logging
{
public class LoggerMessageAttribute {}
public class LoggerMessageAttribute : System.Attribute {}
}
partial class C
{
[Microsoft.Extensions.Logging.LoggerMessage]
public static partial void Log(ILogger logger);
}
", false, includeBaseReferences: false, includeLoggingReferences: false);

Expand Down