Ref fields escape analysis by cston · Pull Request #61195 · dotnet/roslyn

cston · 2022-05-09T23:32:40Z

Proposal: low-level-struct-improvements.md
Test plan: #59194

Included:

Add rules for ref fields (see spec)
Update rules for ref re-assignment (see spec)
Add rules for scoped variables (see spec)
this parameter of struct is implicitly scoped ref
out parameters are implicitly scoped out
Update rules for method invocation (see spec)
Update rules for arguments must match (see spec)
Apply updated rules with -langversion:preview or when runtime contains ref fields feature flag

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

RikkiGibson · 2022-06-03T19:54:07Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

        }

-        internal static uint GetBroadestValEscape(BoundTupleExpression expr, uint scopeOfTheContainingExpression)
+        internal uint GetBroadestValEscape(BoundTupleExpression expr, uint scopeOfTheContainingExpression)


nit: should we settle on one of "widest" or "broadest" and use the term throughout? #ByDesign

Will leave this as is since it wasn't part of the PR.

RikkiGibson · 2022-06-03T20:00:36Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                uint limit = _scope == DeclarationScope.Unscoped && _refKind != RefKind.None ?
+                    Binder.ExternalScope :
+                    Binder.TopLevelScope;
+                return Math.Max(_refEscapeScope, limit);


Should we determine this value at the time we initialize _refEscapeScope? Similarly with _valEscapeScope. #Resolved

RikkiGibson · 2022-06-03T20:36:40Z

src/Compilers/CSharp/Test/Semantic/Semantics/InterpolationTests.cs

                Diagnostic(ErrorCode.ERR_CallArgMixing, @"M2(ref s1, " + expression + @")").WithArguments("CustomHandler.M2(ref S1, ref CustomHandler)", "handler").WithLocation(17, 9),
+                // (17,16): error CS8156: An expression cannot be used in this context because it may not be passed or returned by reference
+                //         M2(ref s1, $"{s}");
+                Diagnostic(ErrorCode.ERR_RefReturnLvalueExpected, "s1").WithLocation(17, 16),


Maybe these diagnostics are referring to the lowered form of the interpolated string here? #Resolved

Yes, these additional errors are reported from CheckInvocationEscape() for CustomHandler.CustomHandler(System.Int32 literalLength, System.Int32 formattedCount, ref S1 s1), from the interpolated string handler conversion data, when checking the safe-to-escape of arguments in CheckInvocationArgMixing() for CustomHandler.M2(ref S1 s1, ref CustomHandler handler).

RikkiGibson · 2022-06-03T21:16:17Z

src/Compilers/CSharp/Test/Semantic/Semantics/RawInterpolationTests_Handler.cs

            //         M2(ref s1, $"{s}");
            Diagnostic(ErrorCode.ERR_CallArgMixing, @"M2(ref s1, " + expression + @")").WithArguments("CustomHandler.M2(ref S1, ref CustomHandler)", "handler").WithLocation(17, 9),
+            // (17,16): error CS8156: An expression cannot be used in this context because it may not be passed or returned by reference
+            //         M2(ref s1, $"""{s}""" + $"""


It would be nice if the diagnostics could be improved here separately of this feature. I didn't find this message to be clear. It looks like the message is referring to code which is synthesized during binding.

RikkiGibson · 2022-06-03T23:55:16Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs

+            CreateCompilationWithMscorlibAndSpan(text).VerifyDiagnostics(
+                // (18,20): error CS8166: Cannot return a parameter by reference 'x' because it is not a ref or out parameter
+                //         return ref x;
+                Diagnostic(ErrorCode.ERR_RefReturnParameter, "x").WithArguments("x").WithLocation(18, 20)


You already mentioned this offline, but we will need to improve the diagnostic here. I suggest using a message like Cannot return parameter '{0}' by reference because it is not a ref parameter. Then possibly a separate message for scoped ref. #Resolved

Yes, I have several PROTOTYPE comments for ERR_RefReturnParameter when it is reported for an out parameter. I agree, it seems reasonable to report "because it is not a ref parameter" only.

src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs

RikkiGibson · 2022-06-06T18:30:52Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs

                //                 x = MayWrap(ref inner);
                Diagnostic(ErrorCode.ERR_EscapeCall, "MayWrap(ref inner)").WithArguments("Program.MayWrap(ref System.Span<int>)", "arg").WithLocation(19, 21)
            );
+            // x = MayWrap(ref outer); is invalid in C#11.


Could you please explain why this is invalid? #Resolved

This is a breaking change:

An rvalue resulting from a method invocation e1.M(e2, ...) is safe-to-escape from the smallest of the following scopes:
...
3. When the return is a ref struct then ref-safe-to-escape of all ref arguments

I've added a comment.

RikkiGibson · 2022-06-06T18:39:20Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs

                Diagnostic(ErrorCode.ERR_CallArgMixing, "MayAssign1(__arglist(ref inner, ref rOuter))").WithArguments("Program.MayAssign1(__arglist)", "__arglist").WithLocation(23, 9)
            );
+
+            // C#11 reports errors for arg2 = MayWrap(ref arg1); because a method invocation that


I'm not sure I grasp this one either but it seems that the general risk is that references to variables within the method MayAssign1 may escape into the variables referenced in its __arglist. Is that right? #Resolved

it seems that the general risk is that references to variables within the method MayAssign1 may escape into the variables referenced in its __arglist

Yes, that's my understanding.

RikkiGibson · 2022-06-06T18:47:09Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs

@@ -3344,11 +3474,12 @@ public ref struct S
 ";
            // Tracking issue: https://github.com/dotnet/roslyn/issues/22361


It looks like we resolved this issue by making it disallowed for a reference to local1 to leak into s in M(out S s)? That's the only reason I can see why we would allow global = local2 here. Consider deleting the comment here and making a note that we resolved the issue as part of this feature implementation. #Resolved

The change below was incorrect and reverted, so this tracking issue still applies.

RikkiGibson · 2022-06-06T21:51:37Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs

 }
 ";
-            CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.RegularPreview).VerifyDiagnostics(
+            CreateCompilationWithMscorlibAndSpan(text, parseOptions: TestOptions.Regular10).VerifyDiagnostics(


Consider also testing LangVersion.Preview here. #Resolved

RikkiGibson · 2022-06-07T16:38:22Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+        {
+            if (UseUpdatedEscapeRules)
+            {
+                return parameter.RefKind is RefKind.None or RefKind.Out || parameter.Scope != DeclarationScope.Unscoped ? Binder.TopLevelScope : Binder.ExternalScope;


It feels like we shouldn't have to check here if this happens to be an out parameter to decide if this is implicitly scoped. The parameter.Scope property should just give the right answer. #Resolved

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

RikkiGibson · 2022-06-07T16:56:57Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

            uint escapeScope = Binder.ExternalScope;

-            ArrayBuilder<bool> inParametersMatchedWithArgs = null;
+            ArrayBuilder<bool>? inParametersMatchedWithArgs = null;


nit: not necessary to handle in this PR, but this should ~~perhaps be a BitVector~~ perhaps be deleted #Resolved

Yes, I've added a PROTOTYPE comment to TryGetUnmatchedInParameterAndFreeMatchedArgs().

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

RikkiGibson · 2022-06-07T17:03:42Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+            ParameterSymbol? unmatchedInParameter = TryGetUnmatchedInParameterAndFreeMatchedArgs(parameters, ref inParametersMatchedWithArgs);



We generate optional arguments during binding now, so does this method of detecting unmatched in parameters actually do anything? Can we delete the whole mechanism for detecting unmatched in parameters? #Resolved

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

cston · 2022-06-20T14:26:24Z

/azp run

azure-pipelines · 2022-06-20T14:26:48Z

Azure Pipelines successfully started running 4 pipeline(s).

cston · 2022-06-20T22:43:22Z

@RikkiGibson, @AlekseyTs, thanks for the detailed reviews. I've responded to all feedback.

AlekseyTs · 2022-06-21T17:55:07Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

                    if (inOutFlags == ParameterAttributes.Out)
                    {
                        refKind = RefKind.Out;
+                        scope = DeclarationScope.RefScoped;


scope = DeclarationScope.RefScoped;

We allow to override it with an attribute? #Closed

Yes, that's possible, with an explicit attribute from metadata.

AlekseyTs · 2022-06-21T18:03:54Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

+                    Debug.Assert((param as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().Scope switch
+                        {
+                            null => true,
+                            DeclarationScope.Unscoped => true,


true

Can we make this stronger? I.e. param.RefKind != RefKind.Out? #Closed

AlekseyTs · 2022-06-21T18:06:14Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

            {
                var fields = typeDescr.Fields;
-                Debug.Assert(fields.All(f => f.Scope == DeclarationScope.Unscoped));
+                Debug.Assert(fields.All(f => f.Scope == DeclarationScope.Unscoped || (f.Scope == DeclarationScope.RefScoped && f.RefKind == RefKind.Out)));


DeclarationScope.Unscoped

Can we make this stronger and verify that parameter is not out? #Closed

AlekseyTs · 2022-06-21T18:06:55Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

            static bool isValidTypeArgument(AnonymousTypeField field)
            {
-                return field.Scope == DeclarationScope.Unscoped &&
+                return (field.Scope == DeclarationScope.Unscoped || (field.Scope == DeclarationScope.RefScoped && field.RefKind == RefKind.Out)) &&


DeclarationScope.Unscoped

Similar question as above #Closed

AlekseyTs · 2022-06-21T18:26:33Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs

+                {
+                    return _refEscapeScope;
+                }
+                return Binder.TopLevelScope;


return Binder.TopLevelScope;

It looks like the table above talks about two different scopes, but we always return the same value. What am I missing? #Resolved

For all scoped values, the ref-safe-to-escape value is the current method; for unscoped values, we return ref-safe-to-escape from the initializer.

for unscoped values, we return ref-safe-to-escape from the initializer.

Perhaps the table above should reflect that and the spec as well.

…scope

…cape analysis

RikkiGibson · 2022-06-22T16:49:34Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Members.cs

                {
                    // https://github.com/dotnet/roslyn/issues/61647: Use public API.
-                    Debug.Assert((param as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().Scope is null or DeclarationScope.Unscoped);
+                    Debug.Assert((param as Symbols.PublicModel.ParameterSymbol)?.GetSymbol<ParameterSymbol>().Scope switch


nit: we could assert !ParameterHelpers.RequiresLIfetimeAnnotationAttribute() here.

RikkiGibson · 2022-06-22T16:59:25Z

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

                {
                    diagnostics.Add(ErrorCode.ERR_ThisInBadContext, thisKeyword.GetLocation());
                }
+                if (refKind == RefKind.Out && scope == DeclarationScope.Unscoped)


This logic seems repeated in several places. It's simple logic so maybe it doesn't matter for feature merge. But we could have something like GetDefaultLifetime(RefKind) and then RequiresLifetimeAnnotation would be implemented as GetDefaultLifetime(param.RefKind) != param.Scope. It's not urgent or blocking to do this, so feel free to WontFix it or to open this as a design debt issue.

AlekseyTs

LGTM (commit 20)

ghost added the Area-Compilers label May 9, 2022

cston force-pushed the ref-scope branch 12 times, most recently from 9b115a6 to c315004 Compare May 16, 2022 23:37

cston force-pushed the ref-scope branch 3 times, most recently from ac3a4fa to 09e43f1 Compare May 18, 2022 21:26

cston added the Feature - Ref Fields label May 20, 2022

cston force-pushed the ref-scope branch 2 times, most recently from 899e9af to 28f99b6 Compare June 3, 2022 13:25

Ref fields escape analysis

cfd69e7

cston force-pushed the ref-scope branch from 28f99b6 to cfd69e7 Compare June 3, 2022 19:01

cston marked this pull request as ready for review June 3, 2022 19:36

cston requested a review from a team as a code owner June 3, 2022 19:36

RikkiGibson self-assigned this Jun 3, 2022

RikkiGibson reviewed Jun 4, 2022

View reviewed changes

More tests

17db3f6

This comment was marked as resolved.

Sign in to view

RikkiGibson reviewed Jun 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Jun 7, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Show resolved Hide resolved

cston added 4 commits June 10, 2022 19:17

Minor updates and several tests

cbdd385

PR feedback

275ca86

Ensure out parameters are considered scoped

6ca6afa

Fix tests

081474c

cston added 3 commits June 20, 2022 14:11

Add method comment

6218444

Consider out parameters in CheckInvocationArgMixing

92f403b

Update comments

3309ab8

Test __arglist

bc816f2

cston mentioned this pull request Jun 21, 2022

Remove TryGetUnmatchedInParameterAndFreeMatchedArgs() if not needed #62054

Closed

Remove PROTOTYPE comment

bf3e30a

AlekseyTs reviewed Jun 21, 2022

View reviewed changes

cston added 4 commits June 21, 2022 16:59

Merge remote-tracking branch 'upstream/features/ref-fields' into ref-…

01efc44

…scope

PR feedback

5834749

Remove test for *EscapeScope since semantic model does not provide es…

5449159

…cape analysis

Test unscoped out parameter

29fff75

RikkiGibson reviewed Jun 22, 2022

View reviewed changes

RikkiGibson approved these changes Jun 22, 2022

View reviewed changes

cston mentioned this pull request Jun 22, 2022

Address PROTOTYPE comments in features/ref-fields #62088

Merged

AlekseyTs approved these changes Jun 23, 2022

View reviewed changes

cston merged commit a428264 into dotnet:features/ref-fields Jun 23, 2022

cston deleted the ref-scope branch June 23, 2022 01:43

cston mentioned this pull request Jul 21, 2022

Remove tracking of unmatched in parameters in GetInvocationEscapeScope() and CheckInvocationEscape() #62834

Closed

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

		@@ -3344,11 +3474,12 @@ public ref struct S
		";
		// Tracking issue: https://github.com/dotnet/roslyn/issues/22361

		ParameterSymbol? unmatchedInParameter = TryGetUnmatchedInParameterAndFreeMatchedArgs(parameters, ref inParametersMatchedWithArgs);

Conversation

cston commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RikkiGibson Jun 3, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 3, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 3, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 3, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

RikkiGibson Jun 6, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 6, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 6, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 6, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jun 7, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RikkiGibson Jun 7, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RikkiGibson Jun 7, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cston commented Jun 20, 2022

Uh oh!

cston commented May 9, 2022 •

edited

Loading

RikkiGibson Jun 3, 2022 •

edited by cston

Loading

RikkiGibson Jun 3, 2022 •

edited by cston

Loading

RikkiGibson Jun 3, 2022 •

edited by cston

Loading

RikkiGibson Jun 3, 2022 •

edited by cston

Loading

RikkiGibson Jun 6, 2022 •

edited by cston

Loading

RikkiGibson Jun 6, 2022 •

edited by cston

Loading

RikkiGibson Jun 6, 2022 •

edited by cston

Loading

RikkiGibson Jun 6, 2022 •

edited by cston

Loading

RikkiGibson Jun 7, 2022 •

edited by cston

Loading

RikkiGibson Jun 7, 2022 •

edited by cston

Loading

RikkiGibson Jun 7, 2022 •

edited by cston

Loading

cston Jun 16, 2022 •

edited

Loading

AlekseyTs Jun 21, 2022 •

edited

Loading

AlekseyTs Jun 21, 2022 •

edited

Loading

AlekseyTs Jun 21, 2022 •

edited

Loading

AlekseyTs Jun 21, 2022 •

edited

Loading

AlekseyTs Jun 21, 2022 •

edited

Loading

cston Jun 22, 2022 •

edited

Loading