-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Extensions: address follow-ups for new metadata design #79674
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
Conversation
f3755e9 to
9deb8e6
Compare
9deb8e6 to
9b0d740
Compare
| { | ||
| case SymbolKind.NamedType: | ||
| if (!((NamedTypeSymbol)member).IsExtension) // PROTOTYPE: Figure out what to do about extensions, if anything | ||
| if (!((NamedTypeSymbol)member).IsExtension) // Tracked by https://github.com/dotnet/roslyn/issues/78963 : Figure out what to do about extensions, if anything |
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.
📝 added tracking bullet to the issue ("do we need special handling in GetSymbolToLocationMap (scenario related to winobj files)")
AlekseyTs
left a comment
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.
LGTM (commit 1)
It looks like this test and the tests below do not test this fact. They test that things are merged, but do not actually test if the names are in metadata for the grouping type. So, if that was the intent of the test, it was not achieved. In order to achieve it, we could get to the PENamedTypeSymbol representing the grouping type in metadata. Our implementation gets grouping name from that symbol, for example. #Closed Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:25506 in 9b0d740. [](commit_id = 9b0d740, deletion_comment = False) |
In addressing this, I realized the event is malformed. The accessors in In reply to: 3142036745 Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:4490 in 9b0d740. [](commit_id = 9b0d740, deletion_comment = False) |
Adjusted comment In reply to: 3145111607 Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:25506 in 9b0d740. [](commit_id = 9b0d740, deletion_comment = False) |
The response sounds like the attribute has been added? but I do not see any changes like that. In reply to: 3145147928 Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:4490 in 9b0d740. [](commit_id = 9b0d740, deletion_comment = False) |
Actually, I see a new test added with the attributes In reply to: 3146130980 Refers to: src/Compilers/CSharp/Test/Emit3/Semantics/ExtensionTests2.cs:4490 in 9b0d740. [](commit_id = 9b0d740, deletion_comment = False) |
Erase attribute in PE
Disallow attribute in source
Verify round-tripping on extension block
Re-enable skipped tests
Addresses some of the follow-up issues tracked by #78963
Relates to test plan #76130