-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Partial events and constructors: merge and check symbols #76970
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
Merged
jjonescz
merged 15 commits into
dotnet:features/PartialEventsCtors
from
jjonescz:PartialEventsCtors-03-Symbols
Feb 8, 2025
Merged
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
eb89e0c
Partial events and constructors: merge and check symbols
jjonescz 03b668d
Improve code
jjonescz 9d4ffa1
Move partial availability check
jjonescz 7e03123
Improve code
jjonescz f49b957
Fix duplicate definition diagnostic
jjonescz e57099c
Test different ordering
jjonescz 75880c5
Avoid reporting diagnostic for escaped `partial`
jjonescz f09ed88
Clarify naming of AccessorsHaveImplementation property
jjonescz 2f98487
Add event definition accessor symbol
jjonescz 0c831b6
Improve code
jjonescz 730adcf
Test sequence points
jjonescz 6fbfa82
Test extern IL
jjonescz 58738cc
Fix interface container diagnostics
jjonescz e6a90df
Fix sequence points issue in the test utility
jjonescz 7573030
Ensure the new accessor owner is a definition
jjonescz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
This change was needed to avoid crash in the sequence point test (added in the same commit). Similar test would also crash for partial methods/properties. Basically the VerifyMethodBody test helper calls GetResolvedMethod on the implementation Cci symbol - that probably doesn't happen normally during emit, that's why the assert was fine for normal scenarios.
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.
Is there any risk that this is adding a new capability to users of the public API, that they may come to depend on, and we didn't intend?
Uh oh!
There was an error while loading. Please reload this page.
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.
Are Cci adapters public API? Anyway I think this was simply wrong before. And users of public APIs would probably not hit the assert (because they use Release bits), so they would just get the wrong result.
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.
This change doesn't look right. I think the API is not supposed to return a different instance, it should either return an original instance, or null. Basically, for a pair of partial declarations, we should never "create" two different adapters, translation layer should make sure of that. Symbols should never be casted to CCI interfaces, they should be translated.
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.
If I remember correctly, PEModuleBuilder performs the translation. There are APIs called "Translate..."
Uh oh!
There was an error while loading. Please reload this page.
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.
Okay, thanks, I think I can skip the failing test for now and look at this separately. Or better yet, I can simply fix this inside the test utility.