Add ReturnOnly scope #64090

RikkiGibson · 2022-09-16T17:53:36Z

RikkiGibson · 2022-09-16T22:34:47Z

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

@@ -794,21 +797,17 @@ private uint GetParameterValEscape(ParameterSymbol parameter)

        private uint GetParameterRefEscape(ParameterSymbol parameter)
        {
-            if (UseUpdatedEscapeRules)


I suspect that more places where this check is used can be simplified. If the ParameterSymbol APIs behave appropriately depending on which escape rules are in use, then doing these checks separately could be redundant.

jaredpar · 2022-09-18T21:50:39Z

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

+                Diagnostic(ErrorCode.ERR_RefFieldCannotReferToRefStruct, "ref R1<T>").WithLocation(8, 12),
+                // (15,9): error CS9079: Cannot ref-assign 'r1' to 'F' because 'r1' can only escape the current method through a return statement.
+                //         r2.F = ref r1;
+                Diagnostic(ErrorCode.ERR_RefAssignReturnOnly, "r2.F = ref r1").WithArguments("F", "r1").WithLocation(15, 9));


Had some ideas for tests we need to validate this change. I'm sure we have a lot of these already but was having trouble finding them all. Decided to just write out the core scenarios that I thought were impacted by this change. More than happy if these are already covered.

https://gist.github.com/jaredpar/880869bcb54e38cb1e1475b1ff2543ef

Looked around a bit and didn't feel like existing tests were quite similar enough. Added these tests in e582e59. I think some of the tests assumed that RTRS would have ReturnOnly scope by default (added [UnscopedRef] to compensate). Also, ReturnOnlyScope_03 has some cases where we assign a ref to a non-ref field, which I'm not sure was intentional.

cston · 2022-09-19T18:57:09Z

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

-                return parameter.RefKind == RefKind.None ? Binder.TopLevelScope : Binder.ExternalScope;
-            }
+                { RefKind: RefKind.None } or { EffectiveScope: not DeclarationScope.Unscoped } => Binder.TopLevelScope,
+                { EffectiveScope: DeclarationScope.Unscoped, Type.IsRefLikeType: true } => Binder.ReturnOnlyScope,


EffectiveScope: DeclarationScope.Unscoped

Is this check unnecessary given the or above?

Yes. The state machine won't test the property again in this path, though.

It feels like it might be good to either:

add RefKind: not RefKind.None to make the case fully self-describing, or

remove EffectiveScope to try and reduce redundancy.

I went with removing.

cston · 2022-09-19T19:32:19Z

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

+    static R F4([UnscopedRef] ref R y) { return F0(__arglist(ref y)); } // 5
+}";
+            // __refvalue operators can only ref-escape to current method.
+            // __arglist operators essentially assume that anything can escape into anything else, including a ref into its referenced variable.


including a ref into its referenced variable

It looks the consumer cannot assign into the referenced variable though, at least according to // 1.

filed #64130.

cston · 2022-09-19T19:54:10Z

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

+                // (17,24): error CS8170: Struct members cannot return 'this' or other instance members by reference
+                //     S Prop4 => new S { refField = ref this.field }; // 2
+                Diagnostic(ErrorCode.ERR_RefReturnStructThis, "refField = ref this.field").WithLocation(17, 24));
+        }


}

Consider adding corresponding tests for instance methods that return ref structs with ref fields:

S F4() => new S { refField = ref this.field }; // 2 [UnscopedRef] S F5() => new S { refField = ref this.field }; // okay

And consider adding tests for static methods:

static S F4(S r) => new S { refField = ref r.field }; // 2 static S F5([UnscopedRef] S r) => new S { refField = ref r.field }; // okay

And tests that use an intermediate local:

static S F4(S r) { S s = new S { refField = ref r.field }; return s; } // 2 static S F5([UnscopedRef] S r) { S s = new S { refField = ref r.field }; return s; } // okay

I think you meant to have ref S r, in the static method case since that's needed to be able to return ref r.field.

I'm going to skip the intermediate local case, I feel we've reliably covered that ref locals get their RSTE from their initializer.

cston · 2022-09-19T20:05:19Z

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

+                static bool condition() => false;
+
+                static void M1(ref S p1, ref S p2) {
+                    p2.field = ref p1.field; // 1


ref p1.field; // 1

Is the assignment of a ref to a value intentional in these cases?

Should these cases be p2.refField = ref p1.field; instead?

yes this should be p2.refField = ref p1.field. Think I had that wrong in the gist.

cston · 2022-09-19T20:06:05Z

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

+                    p2.refField = ref p1.refField; // Okay
+                }
+
+                // The [UnscopedRef] moves `out` to default RSTS which is *return only*


RSTS

"RSTE"?

I'm now thinking about how many times I've written RSTS when I meant RSTE ... a lot

we have spent a lot of time talking about RSTE of RTRS, so..

cston · 2022-09-19T20:13:57Z

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

+
+                    static S2 Inner1([UnscopedRef] ref S s) => new S2 { S = s };
+                    static S2 Inner2(scoped ref S s) => new S2 { S = s };
+                }


Consider adding:

static S2 M9([UnscopedRef] ref S p) { if (condition()) return Inner1(ref p); return Inner2(ref p); // 9 static S2 Inner1([UnscopedRef] ref S s) => new S2 { S = s }; static S2 Inner2(ref S s) => new S2 { S = s }; }

Did you expect return Inner2(ref p) to be an error here? Why? It looks like ref S p has ReturnOnly scope, so it's OK to return ref S through Inner2 (although that doesn't actually happen, that's what the signature of Inner2 indicates)

Did you expect return Inner2(ref p) to be an error here?

Inner2 essentially has the signature

static S2 Inner2(scoped ref S s) => ...

Given current state of the code base. The call to return Inner2(ref p) should be safe no matter the RSTE of ref p as it can't be returned

my brain is constantly switching the RSTE of RTRS between ReturnOnly and CurrentMethod. we need to get #64122 done so the pain will stop

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

jaredpar · 2022-09-19T20:45:43Z

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

+                static bool condition() => false;
+
+                static void M1(ref S p1, ref S p2) {
+                    p2.field = ref p1.field; // 1


yes this should be p2.refField = ref p1.field. Think I had that wrong in the gist.

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

jaredpar · 2022-09-19T20:47:01Z

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

+                    p2.refField = ref p1.refField; // Okay
+                }
+
+                // The [UnscopedRef] moves `out` to default RSTS which is *return only*


I'm now thinking about how many times I've written RSTS when I meant RSTE ... a lot

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

dotnet-issue-labeler bot added the Area-Compilers label Sep 16, 2022

RikkiGibson force-pushed the return-only-scope-2 branch from e6b9c01 to 7a23527 Compare September 16, 2022 20:50

Add ReturnOnly scope

ec524e6

RikkiGibson force-pushed the return-only-scope-2 branch from 7a23527 to ec524e6 Compare September 16, 2022 22:32

RikkiGibson commented Sep 16, 2022

View reviewed changes

RikkiGibson marked this pull request as ready for review September 16, 2022 22:34

RikkiGibson requested a review from a team as a code owner September 16, 2022 22:34

RikkiGibson added 2 commits September 16, 2022 15:54

enable haskell mode

29839c6

fix RefLocalsAndReturnsTests.RefReadonlyCall

dca30f5

jaredpar reviewed Sep 18, 2022

View reviewed changes

cston reviewed Sep 19, 2022

View reviewed changes

RikkiGibson added 2 commits September 19, 2022 12:14

Add suggested tests

e582e59

simplify

d8a761e

cston reviewed Sep 19, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated Show resolved Hide resolved

jaredpar reviewed Sep 19, 2022

View reviewed changes

RikkiGibson added 4 commits September 19, 2022 14:06

add a bit of test coverage

d0749e5

address some test feedback

95e8f68

more

efb9aa8

more

33438c0

cston approved these changes Sep 19, 2022

View reviewed changes

jcouv added the Feature - Ref Fields label Sep 19, 2022

jaredpar approved these changes Sep 19, 2022

View reviewed changes

RikkiGibson merged commit fbe8ad2 into dotnet:main Sep 20, 2022

RikkiGibson deleted the return-only-scope-2 branch September 20, 2022 00:41

ghost added this to the Next milestone Sep 20, 2022

allisonchou mentioned this pull request Sep 20, 2022

[Automated] PRs inserted in VS build main-32920.147 #64151

Closed

Cosifne modified the milestones: Next, 17.4 P3 Sep 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReturnOnly scope #64090

Add ReturnOnly scope #64090

RikkiGibson commented Sep 16, 2022 •

edited

Loading

RikkiGibson Sep 16, 2022

jaredpar Sep 18, 2022

RikkiGibson Sep 19, 2022 •

edited

Loading

cston Sep 19, 2022

RikkiGibson Sep 19, 2022

RikkiGibson Sep 19, 2022

cston Sep 19, 2022

RikkiGibson Sep 19, 2022

cston Sep 19, 2022 •

edited

Loading

RikkiGibson Sep 19, 2022

cston Sep 19, 2022 •

edited

Loading

jaredpar Sep 19, 2022

cston Sep 19, 2022

jaredpar Sep 19, 2022

RikkiGibson Sep 19, 2022 •

edited

Loading

cston Sep 19, 2022

RikkiGibson Sep 19, 2022

jaredpar Sep 19, 2022 •

edited

Loading

RikkiGibson Sep 19, 2022

jaredpar Sep 19, 2022

jaredpar Sep 19, 2022

Add ReturnOnly scope #64090

Add ReturnOnly scope #64090

Conversation

RikkiGibson commented Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Sep 16, 2022 •

edited

Loading

RikkiGibson Sep 19, 2022 •

edited

Loading

cston Sep 19, 2022 •

edited

Loading

cston Sep 19, 2022 •

edited

Loading

RikkiGibson Sep 19, 2022 •

edited

Loading

jaredpar Sep 19, 2022 •

edited

Loading