Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for LoggingGenerator). #71651
Conversation
…roslyn (for LoggingGenerator).
| @@ -34,33 +34,6 @@ public Parser(Compilation compilation, Action<Diagnostic> reportDiagnostic, Canc | |||
| internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) => | |||
| node is MethodDeclarationSyntax m && m.AttributeLists.Count > 0; | |||
|
|
|||
| internal static ClassDeclarationSyntax? GetSemanticTargetForGeneration(GeneratorSyntaxContext context) | |||
There was a problem hiding this comment.
moved to 3.11 file as it's only used for that scenario now.
|
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsFollowup to #70911
|
| INamedTypeSymbol attributeContainingTypeSymbol = attributeSymbol.ContainingType; | ||
| string fullName = attributeContainingTypeSymbol.ToDisplayString(); | ||
|
|
||
| if (fullName == Parser.LoggerMessageAttribute) |
There was a problem hiding this comment.
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.
| context, | ||
| Parser.LoggerMessageAttribute, | ||
| (node, _) => node is MethodDeclarationSyntax, | ||
| (context, _) => context.TargetNode.Parent as ClassDeclarationSyntax) |
There was a problem hiding this comment.
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.
I believe that was intentional. https://github.com/dotnet/runtime/pull/51064/files#r614207808
There was a problem hiding this comment.
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.
| return _span[_pos]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
note: i'm copying what the regex generator does. it's unclear to me why Pop is not just in the linked ValueListBuilder file that is referenced.
| public T Pop() | ||
| { | ||
| _pos--; | ||
| return _span[_pos]; |
There was a problem hiding this comment.
@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?
|
@joperezr Who should look at this PR? Thanks! |
|
@maryamariyan - can you take a look? |
|
@joperezr @ericstj , @maryamariyan is OOF. Is there someone else who can look at this? Thanks! |
|
I added a couple more folks. The owners for this area are @eerhardt @tarekgh and @maryamariyan. |
| <Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.cs" Link="Production\ValueListBuilder.cs" /> | ||
| <Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ValueListBuilder.Pop.cs" Link="Production\ValueListBuilder.Pop.cs" /> |
There was a problem hiding this comment.
(nit) what does the Production folder mean here?
There was a problem hiding this comment.
No idea. this was copied from teh regex generator which uses those folder names :) @stephentoub ?
There was a problem hiding this comment.
A random-ish name that shows up in the IDE indicating that the source is also used by the production libraries and not just part of the source generator.
ghost
locked as resolved and limited conversation to collaborators
Followup to #70911