-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Ensure that modreq(InAttribute) is emitted for synthesized ref readonly returning methods
#80893
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
…donly` returning methods Closes dotnet#76847
|
@dotnet/roslyn-compiler For a second review |
1 similar comment
|
@dotnet/roslyn-compiler For a second review |
| comp.VerifyDiagnostics( | ||
| // (5,20): warning CS0649: Field 'var.o' is never assigned to, and will always have its default value null | ||
| // internal other o; | ||
| Diagnostic(ErrorCode.WRN_UnassignedInternalField, "o").WithArguments("var.o", "null").WithLocation(5, 20), |
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.
It's not obvious how this change in diagnostic output results from the change. Is this change expected? #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.
It's not obvious how this change in diagnostic output results from the change. Is this change expected?
It is cased by changes in Binder_Statements.cs, we no longer treat body of ref var () => throw null as returning a reference to an instance of "@var" type. Therefore, we don't treat it as a possible write to the field.
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.
The behavior is consistent with explicit return statements
| var delegateType = expr.GetFunctionType()?.GetInternalDelegateType(); | ||
| delegateType?.AddUseSiteInfo(ref useSiteInfo); | ||
|
|
||
| if (expr is BoundMethodGroup { Methods: not [{ MethodKind: MethodKind.LocalFunction }] } && |
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 this check for MethodKind just to avoid cascading diagnostics (ie. checking the InAttribute both on the local function declaration and again on the usage)?
nit: If so, consider leaving a comment #Resolved
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 a comment
jcouv
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 Thanks (commit 2)
Closes #76847