Support for ref-readonly locals #22269

agocke · 2017-09-21T21:22:50Z

Customer scenario

New feature: ref-readonly local variables. In addition to ref-readonly parameters, in C# 7.2 users will be able to declare local variables as ref readonly.

Bugs this fixes:

Mentioned in #19216

Workarounds, if any

New feature!

Risk

Ref readonly locals follow in the footsteps of ref-readonly parameters. Almost everything dangerous with ref readonly locals can be done with ref readonly parameters, and the same checks are applied to ref readonly locals as to ref readonly parameters. The codegen for many of these cases has been manually verified to be safe.

Performance impact

Low. A few small checks on existing codepaths.

Is this a regression from a previous update?

No.

Root cause analysis:

New feature!

How was the bug found?

During test plan review it was decided that not supporting ref readonly locals was too painful to ship to users.

Test documentation updated?

The following scenarios have been manually tested for the IDE:

Category	Feature/Description	C# Signoff
Enable/Disable	Feature Flags To completely enables/disable new compiler features in the compiler & IDE	✅
Typing	General Typing- Type and paste new constructs- Nothing interferes with verbatim typing	✅
	Completion- Typing new keyword/construct names- Dotting off of new constructs- Matching part of the identifier is highlighted (including word prefix matches) [Visual Studio 2015 Update 1]- Target type preselection [Visual Studio 2017]IntelliSense filtering [Visual Studio 2017]	✅
	Formatting - Spacing in and around new constructs- Spacing options- Format Document commandTools > Options settings should be respected	:free:
	Automatic Brace Completion (C# only) - Auto-insert close brace- Shift+Enter commit of IntelliSense and any pending brace completion sessions (Known issue: #18065)	🆓
	Indentation - Typing Enter in an unfinished statement indents the next line	:white_check_mark:
Navigating	Go To Definition - F12 from callsites to definition- Ctrl+click [Visual Studio 2017 version 15.4]	✅
	Go To Implementation - Ctrl+F12 to jump from virtual members to their implementations- Jump from inheritable types to their implementations	:free:
	Find All References - Lists references to a symbol in "Find Symbol Results" window- Shows results in hierarchy grouped by definition [Visual Studio 2015]- Results should be groupable/filterable/classified appropriately [Visual Studio 2017] - Find All References on literals [Visual Studio 2017 version 15.3]	:white_check_mark:
Viewing	Colorization - Keywords, literals, and identifiers colored appropriately in code- Colors should theme appropriately- The "Blue Theme (Additional Contrast)" should have sufficiently dark colors	:white_check_mark:
	Error Squiggles - Squiggles appear as expected on reasonable spans	✅

Ref-readonly locals are mostly identical to ref-readonly parameters. The most important difference is that when possibly mutating methods are called on ref-readonly locals of struct type, a proper temporary is created before calling the method.

jaredpar · 2017-09-22T19:52:21Z

appoved pending reviews

agocke · 2017-09-22T20:52:33Z

ping @VSadov @OmarTawfik for reviews

jaredpar · 2017-09-22T20:59:59Z

src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs

+        /// </summary>
+        private LocalDefinition EmitDupAddress(BoundDup dup, AddressKind addressKind)
+        {
+            if (!HasHome(dup, addressKind != AddressKind.ReadOnly))


Consider using a named argument for the second parm here. Makes it more readable.

jaredpar · 2017-09-22T21:02:38Z

src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs

                    var local = ((BoundLocal)expression).LocalSymbol;
-                    return !IsStackLocal(local) || local.RefKind != RefKind.None;
+                    return !((IsStackLocal(local) && local.RefKind == RefKind.None) || 
+                        (needWriteable && local.RefKind == RefKind.RefReadOnly));


Is needWritable protecting the local or the value?

The local. It's specifying that you must be able to write to the local pointed to by the address.

Then why is the last condition only checking ref readonly

In reply to: 140599487 [](ancestors = 140599487)

i am wrong

In reply to: 140600043 [](ancestors = 140600043,140599487)

jaredpar · 2017-09-22T21:05:05Z

src/Compilers/CSharp/Portable/Symbols/LocalSymbol.cs

@@ -268,7 +268,7 @@ internal virtual bool IsWritable
                    case LocalDeclarationKind.UsingVariable:
                        return false;
                    default:
-                        return true;
+                        return RefKind != RefKind.RefReadOnly;


The local is still assignable. Is that not what is being tracked here?

So what do you expect to happen with the following code?

int x = 0; ref readonly int xr = ref x; xr++;

Same thing that happens if xr is a readonly static

In reply to: 140599254 [](ancestors = 140599254)

jaredpar · 2017-09-22T21:08:11Z

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

+                // (11,25): error CS1510: A ref or out value must be an assignable variable
+                //         ref int z = ref x;
+                Diagnostic(ErrorCode.ERR_RefLvalueExpected, "x").WithLocation(11, 25)
+            );


Need a test that essentially does the following

ref readonly int x = ref 1;

This is legal when done as an argument to a ref readonly parameter. Need to be clear if this is okay / not okay when done in this context.

This is prohibited for now. I'll add test. It does seem a little annoying that this is prohibited for locals but allowed for calls. We may want to relax that restriction and auto-spill like we do for arguments, but I'm not convinced that needs to be blocking for this PR.

Also:
ref readonly object x = ref someStringLocal; // we will not allow conversions, at least for now

In reply to: 140600354 [](ancestors = 140600354)

VSadov

LGTM
with some test suggestions

VSadov · 2017-09-22T23:49:15Z

Do we have a test to make sure this is not enabled on 7.1 ?

In reply to: 331557740 [](ancestors = 331557740)

OmarTawfik · 2017-09-23T00:02:59Z

src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs

@@ -234,6 +240,21 @@ private void EmitLocalAddress(BoundLocal localAccess)
            {
                _builder.EmitLocalAddress(GetLocal(localAccess));
            }
+            return null;


nit: new line

OmarTawfik · 2017-09-23T00:06:46Z

src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs

@@ -971,7 +971,23 @@ private static bool IsIndirectAssignment(BoundAssignmentOperator node)
        {
            var lhs = node.Left;

-            Debug.Assert(node.RefKind == RefKind.None || (lhs as BoundLocal)?.LocalSymbol.RefKind == RefKind.Ref,
+            bool IsAssignable(RefKind lhsKind, RefKind rhsKind)


IsAssignable [](start = 17, length = 12)

if this is only for asserting, I'd suggest inlining into the Assert call and simply returning false otherwise. Throwing here will not trigger the Debug window while debugging, and will route it to the VS fault handler.

OmarTawfik · 2017-09-23T00:07:56Z

src/Compilers/CSharp/Portable/Symbols/LocalSymbol.cs

@@ -268,7 +268,7 @@ internal virtual bool IsWritable
                    case LocalDeclarationKind.UsingVariable:
                        return false;
                    default:
-                        return true;
+                        return RefKind != RefKind.RefReadOnly;


can we explicitly state the true cases, and throw otherwise?

I don't understand why you would want to ever throw here. Ref variables are meaningful, ref-readonly variables are meaningful, and "non-ref" variables are meaningful.

OmarTawfik · 2017-09-23T00:24:39Z

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

+            );
+        }
+
+        [Fact]
        public void RefLocalMissingInitializer()


I'dd suggest adding a test for:

// In a ref readonly method ref readonly int x = ref SomeField; return ref x; ```

OmarTawfik · 2017-09-23T00:25:38Z

src/Compilers/CSharp/Test/Syntax/Parsing/RefReadonlyReturnsTests.cs

@@ -138,9 +138,6 @@ static void Use<T>(T dummy)
 ";
            var comp = CreateCompilationWithMscorlib45(text, new[] { ValueTupleRef, SystemRuntimeFacadeRef });
            comp.VerifyDiagnostics(


in that case, we should remove the first line of the method. it is covered by others, and it is not proving the "unexpected bind time" here.

In general, it's not a good idea to modify regression tests, even if they're obsolete. Modifying the baseline is fine, but it's good to keep the tests the same if only to prove that there are no backwards-incompatible changes.

OmarTawfik · 2017-09-23T00:26:55Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs

+        Console.WriteLine(rs.X);
+        rs.AddOne();
+        rs.AddOne();
+        rs.AddOne();


rs.AddOne(); [](start = 8, length = 12)

not sure I understand that test. why calling it repeatedly afterwards?

To make sure that we're reusing the same slot, instead of allocating multiple slots for the temp. This is exercising the optimizer.

OmarTawfik · 2017-09-23T00:27:40Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs

+  IL_0052:  call       ""void S.AddOne()""
+  IL_0057:  ret
+}");
+            // This should generate similar IL to the previous


// This should generate similar IL to the previous [](start = 12, length = 50)

why is this grouped into one test? consider separating them.

This is a relatively unique IL pattern so Vlad and I thought it would be a good idea to prove you can reach this IL pattern without going through ref-readonly. The idea is to compare the IL generated for the optimizer spilling the temp to the IL generated for C# code manually spilling.

OmarTawfik · 2017-09-23T00:28:20Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenRefReadonlyReturnTests.cs

+        Console.WriteLine(temp.X);
+        temp = sr;
+        temp.AddOne();
+        Console.WriteLine(temp.X);


Console.WriteLine(temp.X); [](start = 8, length = 26)

should we print s's value afterwards?

I think the IL is sufficient to prove that s remains unmodified. As noted in the comment, this test is mostly important for verifying IL consistency.

OmarTawfik · 2017-09-23T00:33:00Z

Should we add readonly to the suggestion list in the IDE?

OmarTawfik

Done. left a few comments.

agocke · 2017-09-23T02:20:07Z

@OmarTawfik Regarding putting readonly in the completion list -- it was offered for me when I manually tested typing, so I don't think we need to make any changes there.

OmarTawfik · 2017-09-23T02:27:31Z

@agocke I suggest then a test in ReadOnlyKeywordRecommenderTests.cs

agocke · 2017-09-23T18:11:10Z

@OmarTawfik Sounds good, I'll follow up with a bunch of IDE regression tests in a later PR.

* dotnet/master: (351 commits) Avoid scheduling an unnecessary dispose task for EmptyAsyncToken Support for ref-readonly locals (dotnet#22269) ported tests from dotnet#21263 (dotnet#22296) NormalizeWhitespace should not add a space in nullable type (dotnet#22234) Fix the work item number associated with the unit test Add unit test Terminate a sentence Add instructions for upgrading PowerShell Unsafe local should require /unsafe flag on compilation (dotnet#22268) CR feedback Not poisoning restricted types known to be stack only. Typo Error messsage should mention C# 7.0 not C# 7 (dotnet#22255) Package descriptions for CSharp and VB should include summary blurb (dotnet#22166) Fix a trivial issue in how IsLifted is calculated for C# compound assignment operator Do not zero out "With" statement target expression locals referenced within a lambda. (dotnet#22223) Fix unit test failure from merge Mark "private protected", "ref readonly" and other C# 7.2 features as merged (dotnet#22209) Add unit tests for IAwaitExpression and make the API public again more test fixes due to merge conflicts ...

* dotnet/master: (309 commits) Avoid scheduling an unnecessary dispose task for EmptyAsyncToken Support for ref-readonly locals (dotnet#22269) ported tests from dotnet#21263 (dotnet#22296) NormalizeWhitespace should not add a space in nullable type (dotnet#22234) Fix the work item number associated with the unit test Add unit test Terminate a sentence Add instructions for upgrading PowerShell Unsafe local should require /unsafe flag on compilation (dotnet#22268) CR feedback Not poisoning restricted types known to be stack only. Typo Error messsage should mention C# 7.0 not C# 7 (dotnet#22255) Package descriptions for CSharp and VB should include summary blurb (dotnet#22166) Fix a trivial issue in how IsLifted is calculated for C# compound assignment operator Do not zero out "With" statement target expression locals referenced within a lambda. (dotnet#22223) Fix unit test failure from merge Mark "private protected", "ref readonly" and other C# 7.2 features as merged (dotnet#22209) Add unit tests for IAwaitExpression and make the API public again more test fixes due to merge conflicts ...

benaadams · 2017-10-02T23:09:37Z

Yey! Was just about to raise issue for this - wasn't sure how you were meant to use a ref readonly return - other than directly and once; or by using the function call output as the parameter to another function that takes an in

agocke added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 21, 2017

dnfclas added the cla-already-signed label Sep 21, 2017

agocke changed the title ~~WIP~~ Support for ref-readonly locals Sep 22, 2017

agocke added Area-Compilers and removed PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Sep 22, 2017

agocke requested review from VSadov, OmarTawfik and a team September 22, 2017 19:46

Add support for ref-readonly locals

cb6f875

Ref-readonly locals are mostly identical to ref-readonly parameters. The most important difference is that when possibly mutating methods are called on ref-readonly locals of struct type, a proper temporary is created before calling the method.

agocke force-pushed the readonly-ref-locals branch from 83b829e to cb6f875 Compare September 22, 2017 19:50

jaredpar reviewed Sep 22, 2017

View reviewed changes

Respond to PR comments

a8a1fa4

VSadov approved these changes Sep 22, 2017

View reviewed changes

Add some tests around conversions and language version

7fd5722

OmarTawfik reviewed Sep 23, 2017

View reviewed changes

OmarTawfik approved these changes Sep 23, 2017

View reviewed changes

Respond to PR comments

eac83bd

agocke merged commit fef4159 into dotnet:master Sep 23, 2017

agocke deleted the readonly-ref-locals branch September 23, 2017 18:11

jcouv added the Approved to merge label Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for ref-readonly locals #22269

Support for ref-readonly locals #22269

agocke commented Sep 21, 2017 •

edited

Loading

jaredpar commented Sep 22, 2017

agocke commented Sep 22, 2017

jaredpar Sep 22, 2017

jaredpar Sep 22, 2017

agocke Sep 22, 2017

jaredpar Sep 22, 2017

jaredpar Sep 22, 2017

jaredpar Sep 22, 2017

agocke Sep 22, 2017

jaredpar Sep 22, 2017

jaredpar Sep 22, 2017

agocke Sep 22, 2017

VSadov Sep 22, 2017

VSadov left a comment

VSadov commented Sep 22, 2017

OmarTawfik Sep 23, 2017

OmarTawfik Sep 23, 2017

OmarTawfik Sep 23, 2017

agocke Sep 23, 2017

OmarTawfik Sep 23, 2017

OmarTawfik Sep 23, 2017

agocke Sep 23, 2017

OmarTawfik Sep 23, 2017

agocke Sep 23, 2017

OmarTawfik Sep 23, 2017

agocke Sep 23, 2017

OmarTawfik Sep 23, 2017

agocke Sep 23, 2017

OmarTawfik commented Sep 23, 2017

OmarTawfik left a comment

agocke commented Sep 23, 2017

OmarTawfik commented Sep 23, 2017

agocke commented Sep 23, 2017

benaadams commented Oct 2, 2017

Support for ref-readonly locals #22269

Support for ref-readonly locals #22269

Conversation

agocke commented Sep 21, 2017 • edited Loading

jaredpar commented Sep 22, 2017

agocke commented Sep 22, 2017

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

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

VSadov left a comment

Choose a reason for hiding this comment

VSadov commented Sep 22, 2017

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

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

Choose a reason for hiding this comment

OmarTawfik commented Sep 23, 2017

OmarTawfik left a comment

Choose a reason for hiding this comment

agocke commented Sep 23, 2017

OmarTawfik commented Sep 23, 2017

agocke commented Sep 23, 2017

benaadams commented Oct 2, 2017

agocke commented Sep 21, 2017 •

edited

Loading