Polyfill the incremental generator ForAttributeWithMetadataName from roslyn (for EventSourceGeneration). #71662
Conversation
…roslyn (for LibraryImportGenerator).
…roslyn (for EventSourcewGenerator).
|
Tagging subscribers to this area: @dotnet/interop-contrib |
| { | ||
| continue; | ||
| } | ||
| if (attribute.AttributeClass.Name != "EventSourceAttribute" && |
There was a problem hiding this comment.
Is && correct here? It shouldn't be ||?
...ibraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/gen/EventSourceGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
|
This is a follow-up pr to the library import pr. @joperezr tells me that this isn't shipping yet, so it was low pri to fix. |
Ok. Out of curiosity why does this depend on the LibraryImport one? |
| { | ||
| const string EventSourceAutoGenerateAttribute = "System.Diagnostics.Tracing.EventSourceAutoGenerateAttribute"; |
There was a problem hiding this comment.
we don't need to check for EventSourceAutoGenerateAttribute. we wouldn't have gotten here unless we had that attribute on the class we're examining.
| if (attributeFullName.Equals(EventSourceAutoGenerateAttribute, StringComparison.Ordinal)) | ||
| { | ||
| autoGenerate = true; | ||
| continue; |
There was a problem hiding this comment.
we required having EventSourceAutoGenerateAttribute before. otherwise, autogenerate would not be true, and we'd bail out below.
| if (!attributeFullName.Equals(EventSourceAttribute, StringComparison.Ordinal)) | ||
| { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
the code below would only execute if we had EventSourceAttribute, so that's what we maintain below.
| // since this generator doesn't know how to generate a nested type... | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
moved this check up. there was no point doing it in a loop since it would always be the same result. and if it always caused us to continue, we'd never set eventSourceClass. so we'd terminate the loop and would return null. so this is effectively the same.
There was a problem hiding this comment.
note: this logic is wrong in that it can't handle: namespace n1 { namespace n2 { class Whatever { } } }. However, that bug existed before. I didn't change it.
A better check would likely be to do if (classDef.Parent is TypeDeclarationSyntax)
| { | ||
| foreach (AttributeSyntax ca in cal.Attributes) | ||
| if (attribute.AttributeClass?.Name != "EventSourceAttribute" || |
There was a problem hiding this comment.
in general, try to do the basic name check first. it's much faster and allocation-light versus ToDisplayString
|
|
||
| if (!autoGenerate) | ||
| string nspace = ns.Name.ToString(); |
There was a problem hiding this comment.
note. this is not correct. if you have namespace N1/*comment*/. N2, then the ToString will include hte extraneous comments and whitespace. However, this was the logic from before, so i'm keeping it.
| // We don't need to include the later generator factories for collection elements | ||
| // as the later generator factories only apply to parameters. | ||
| generatorFactory = new AttributedMarshallingModelGeneratorFactory( | ||
| generatorFactory, | ||
| elementFactory, | ||
| new AttributedMarshallingModelOptions(runtimeMarshallingDisabled, Scenario.ManagedToUnmanagedIn, Scenario.ManagedToUnmanagedRef, Scenario.ManagedToUnmanagedOut)); | ||
| new AttributedMarshallingModelOptions(runtimeMarshallingDisabled, MarshalMode.ManagedToUnmanagedIn, MarshalMode.ManagedToUnmanagedRef, MarshalMode.ManagedToUnmanagedOut)); |
There was a problem hiding this comment.
I'm still not clear on why these changes are necessary in this PR. Is this PR renaming Scenario to MarshalMode or something?
There was a problem hiding this comment.
These were bad merges as i was undoing the changes. The files are now untouched. Note: this will be squashed when PR is merged, so there won't be random cruft like this in your repo history. However, i don't like squashing as the PR is in progress as that can make it harder to see what's going on. If you want though i can pre-squash.
There was a problem hiding this comment.
If you want though i can pre-squash.
Nope.
ghost
locked as resolved and limited conversation to collaborators
Followup to #70911 and #71652