Skip to content

Update method invocation escape analysis to match spec changes#62886

Merged
cston merged 16 commits intodotnet:mainfrom
cston:62791
Aug 12, 2022
Merged

Update method invocation escape analysis to match spec changes#62886
cston merged 16 commits intodotnet:mainfrom
cston:62791

Conversation

@cston
Copy link
Contributor

@cston cston commented Jul 23, 2022

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:11 or higher.

Method invocation:

For a given argument a that is passed to parameter p:

  1. If p is scoped ref then a does not contribute ref-safe-to-escape when considering arguments.
  2. If p is scoped then a does not contribute safe-to-escape when considering arguments.

A value resulting from a method invocation e1.M(e2, ...) is safe-to-escape from the smallest of the following scopes:

  1. The calling method
  2. The safe-to-escape contributed by all argument expressions
  3. When the return is a ref struct then ref-safe-to-escape contributed by all ref arguments

A value resulting from a method invocation ref e1.M(e2, ...) is ref-safe-to-escape the smallest of the following scopes:

  1. The safe-to-escape of the rvalue of e1.M(e2, ...)
  2. The ref-safe-to-escape contributed by all ref arguments

Method arguments must match:

For any method invocation e.M(a1, a2, ... aN)

  1. Calculate the safe-to-escape of the method return (for this rule assume it has a ref struct return type)
  2. All ref or out arguments of ref struct types must be assignable by a value with that safe-to-escape. This applies even when the ref argument matches a scoped ref parameter.

The PR also includes a change to treat ref parameters to ref struct types as scoped ref by default - see #62691.

Fixes #62791
Fixes #62691
Fixes #63016

@ghost ghost added the Area-Compilers label Jul 23, 2022
@cston cston force-pushed the 62791 branch 3 times, most recently from b0b968b to ef1d1f3 Compare July 26, 2022 16:17
@cston cston force-pushed the 62791 branch 5 times, most recently from 4e775dd to 301445b Compare July 30, 2022 17:07
@cston cston changed the base branch from main to release/dev17.4 July 30, 2022 17:08
@cston cston force-pushed the 62791 branch 3 times, most recently from f29a0a2 to a0cb59c Compare July 31, 2022 21:10
@cston cston changed the base branch from release/dev17.4 to main August 1, 2022 18:56
@cston cston force-pushed the 62791 branch 3 times, most recently from 6dca921 to 1c554c0 Compare August 4, 2022 17:39
@cston cston force-pushed the 62791 branch 4 times, most recently from b9a1e23 to 9d1bdd0 Compare August 7, 2022 04:26
@cston cston force-pushed the 62791 branch 3 times, most recently from 509a7e6 to 1a57572 Compare August 10, 2022 16:55
</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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done review pass (commit 14). A few test questions left.

@cston cston merged commit fa25949 into dotnet:main Aug 12, 2022
@cston cston deleted the 62791 branch August 12, 2022 02:18
@ghost ghost added this to the Next milestone Aug 12, 2022
@cston cston modified the milestones: Next, 17.4 Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

4 participants