Update method invocation escape analysis to match spec changes#62886
Update method invocation escape analysis to match spec changes#62886cston merged 16 commits intodotnet:mainfrom
Conversation
b0b968b to
ef1d1f3
Compare
4e775dd to
301445b
Compare
f29a0a2 to
a0cb59c
Compare
6dca921 to
1c554c0
Compare
b9a1e23 to
9d1bdd0
Compare
509a7e6 to
1a57572
Compare
| </data> | ||
| <data name="ERR_UnscopedRefAttributeUnsupportedTarget" xml:space="preserve"> | ||
| <value>UnscopedRefAttribute can only be applied to 'out' parameters, 'ref' parameters that refer to 'ref struct' types, and instance methods and properties on 'struct' types other than constructors and 'init' accessors.</value> | ||
| <value>UnscopedRefAttribute can only be applied to 'out' parameters, 'ref' and 'in' parameters that refer to 'ref struct' types, and instance methods and properties on 'struct' types other than constructors and 'init' accessors.</value> |
There was a problem hiding this comment.
Doesn't need to happen here but it might be good to adjust this language to have distinct messages like
- cannot use UnscopedRefAttribute here because the associated by-reference parameter is unscoped by default.
- cannot use UnscopedRefAttribute here because the associated parameter is by-value.
It feels like it will be easier to spell out the precise enumeration of which parameters are unscoped by default, etc. in documentation rather than in a single diagnostic message.
RikkiGibson
left a comment
There was a problem hiding this comment.
still need time to fully review RefFieldTests but everything else LGTM.
| class B1 : A | ||
| { | ||
| public override void F1<T>(out T t) { t = default!; } | ||
| public override void F2<T>(out T? t) where T : default { t = default; } |
There was a problem hiding this comment.
What is this actually testing wrt ref fields? Ref fields can only be in ref structs, and ref structs cannot be used as generic type parameters.
There was a problem hiding this comment.
What is this actually testing wrt ref fields?
It's not testing ref fields. I believe I added this test when there unexpected errors around overrides, T?, and scoped (since the out parameters are implicitly scoped). I think the test will be more meaningful after addressing the issue link below and adding [UnscopedRef] cases.
333fred
left a comment
There was a problem hiding this comment.
Done review pass (commit 14). A few test questions left.
Updates the implementation of escape analysis rules for method invocation to match the recent spec changes - see #62791.
The updated rules are in effect when compiling with a corlib that includes
System.Runtime.CompilerServices.RuntimeFeature.ByRefFields, or when compiling with-langversion:11or higher.Method invocation:
Method arguments must match:
The PR also includes a change to treat
refparameters toref structtypes asscoped refby default - see #62691.Fixes #62791
Fixes #62691
Fixes #63016