-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LoggingGenerator). #71651
Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LoggingGenerator). #71651
Changes from 1 commit
dad6fc2
32eb2f8
e63a0f9
c4e36d4
93a6327
a1f5727
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 |
---|---|---|
|
@@ -51,13 +51,40 @@ public void OnVisitSyntaxNode(GeneratorSyntaxContext context) | |
{ | ||
if (Parser.IsSyntaxTargetForGeneration(context.Node)) | ||
{ | ||
ClassDeclarationSyntax classSyntax = Parser.GetSemanticTargetForGeneration(context); | ||
ClassDeclarationSyntax classSyntax = GetSemanticTargetForGeneration(context); | ||
if (classSyntax != null) | ||
{ | ||
ClassDeclarations.Add(classSyntax); | ||
} | ||
} | ||
} | ||
|
||
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 == Parser.LoggerMessageAttribute) | ||
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. note: the only check we do is that hte attribute's fully qualified name is LoggerMessageAttribute (e.g. we don't actually check type-equility or anything like that). as such, we don't have to do any additional semantic work ourselves as this is exactly what roslyn is ensuring. |
||
{ | ||
return methodDeclarationSyntax.Parent as ClassDeclarationSyntax; | ||
} | ||
} | ||
} | ||
|
||
return null; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
using System.Text; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.CSharp.Syntax; | ||
using Microsoft.CodeAnalysis.DotnetRuntime.Extensions; | ||
using Microsoft.CodeAnalysis.Text; | ||
|
||
[assembly: System.Resources.NeutralResourcesLanguage("en-us")] | ||
|
@@ -19,7 +20,11 @@ public partial class LoggerMessageGenerator : IIncrementalGenerator | |
public void Initialize(IncrementalGeneratorInitializationContext context) | ||
{ | ||
IncrementalValuesProvider<ClassDeclarationSyntax> classDeclarations = context.SyntaxProvider | ||
.CreateSyntaxProvider(static (s, _) => Parser.IsSyntaxTargetForGeneration(s), static (ctx, _) => Parser.GetSemanticTargetForGeneration(ctx)) | ||
.ForAttributeWithMetadataName( | ||
context, | ||
Parser.LoggerMessageAttribute, | ||
(node, _) => node is MethodDeclarationSyntax, | ||
(context, _) => context.TargetNode.Parent as ClassDeclarationSyntax) | ||
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. the original code did no actual semantic checks beyond just checking that there was an attribute with a particular fully qualified name on it. this is unlike the regex generator which does additional semantic checks. 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 believe that was intentional. https://github.com/dotnet/runtime/pull/51064/files#r614207808 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. 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. note: this wasn't a complaint. Just pointing out the difference between these two generators. It's not necessarily right/wrong, just something to potentially look into depending on what semantics you want. |
||
.Where(static m => m is not null); | ||
|
||
IncrementalValueProvider<(Compilation, ImmutableArray<ClassDeclarationSyntax>)> compilationAndClasses = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Runtime.CompilerServices; | ||
|
||
namespace System.Collections.Generic | ||
{ | ||
/// <summary> | ||
/// These public methods are required by RegexWriter. | ||
/// </summary> | ||
internal ref partial struct ValueListBuilder<T> | ||
{ | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public T Pop() | ||
{ | ||
_pos--; | ||
return _span[_pos]; | ||
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 any concern that this doesn't clear the value that was popped? i assume it's ok since this is all short lived on the stack anyways? |
||
} | ||
} | ||
} | ||
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. note: i'm copying what the regex generator does. it's unclear to me why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to 3.11 file as it's only used for that scenario now.