-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove remaining ref readonly
prototype comments
#69082
Remove remaining ref readonly
prototype comments
#69082
Conversation
@@ -1562,7 +1562,7 @@ internal SynthesizedAttributeData SynthesizeRequiresLocationAttribute(ParameterS | |||
if ((object)Compilation.SourceModule != symbol.ContainingModule) | |||
{ | |||
// For symbols that are not defined in the same compilation (like NoPia), don't synthesize this attribute. | |||
Debug.Assert(false); // PROTOTYPE: Test this code path. | |||
Debug.Assert(false, $"Unexpected embedding of a parameter symbol '{symbol.ToDisplayString()}'"); |
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.
Don't think this path is reachable - only types are embedded in NoPIA scenarios, not parameters. #Closed
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.
only types are embedded in NoPIA scenarios, not parameters.
That is not accurate. Interface methods are embedded and their parameters as well. We probably have a test that hits this code path in SynthesizeIsReadOnlyAttribute
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.
You're right, thanks, adding a test.
Done with review pass (commit 1) |
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 2), assuming CI is passing
Test plan: #68056