Relaxed escape rules for spans #22203

VSadov · 2017-09-19T20:33:17Z

Also fixes #22197 (mixing rules for ternary ref)

NOTE: many tests that were tracking scope-escapes would be meaningless now since. We cannot observe the difference since spans cannot become attached to a particular scope by capturing a local ref.

I tried to salvage tests by using global/stackalloc scopes instead - to make sure we are not losing coverage by accident.
However, since that has only 2 levels of escape (not N) as before, there can be some overlap in tests now. I do not think extra testing that might result from this is a concern though.

VSadov · 2017-09-19T21:35:11Z

@dotnet/roslyn-compiler - please review.

OmarTawfik · 2017-09-19T23:05:31Z

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

+                    var consValid = CheckValEscape(conditional.Consequence.Syntax, conditional.Consequence, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics);
+
+                    if (consValid || conditional.IsByRef)
+                    {


Not sure I get that. Why don't we check the alternative if the consequence is valid? #Resolved

Should be !consValid. And I need test, I guess.

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

OmarTawfik · 2017-09-19T23:07:52Z

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

@@ -662,10 +662,7 @@ static void Main()
            comp.VerifyEmitDiagnostics(
                // (15,27): error CS8168: Cannot return local 'local1' by reference because it is not a ref local


because it is not a ref local [](start = 84, length = 29)

because it is? #WontFix

local1 is not a ref local so we cannot return it by reference. When we need to ref return a ternary, we recursively check operands and produce at least one error.
The quality of cascaded error may be improved, but this is how it work right now.

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

OmarTawfik · 2017-09-19T23:42:40Z

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

@@ -45,23 +47,25 @@ static S1 MayWrap(ref int arg)
        }
    }
 ";
-            CreateStandardCompilation(text).VerifyDiagnostics(
-                // (21,42): error CS8168: Cannot return local 'local' by reference because it is not a ref local
+            CreateCompilationWithMscorlibAndSpan(text).VerifyDiagnostics(


CreateCompilationWithMscorlibAndSpan [](start = 12, length = 36)

That reminds me, how are we tracking the addition of Span to the libraries we reference in testing? so that we can remove this helper function #Resolved

I am not even sure we have to change this. Span implementations are platform-specific and we only care about the APIs that exposed. As long as we call right methods, we are producing right code.
I will enter a bug to add some tests with actual span (I think we can use slow-span, since that should be able to run on any platform), but that is not a priority.

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

#22213

In reply to: 139847810 [](ancestors = 139847810,139844972)

VSadov · 2017-09-20T15:28:47Z

@dotnet/roslyn-compiler - any more comments on this?

jcouv · 2017-09-20T16:36:04Z

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

-            uint scopeOfTheContainingExpression)
+            uint scopeOfTheContainingExpression,
+            bool isRefEscape
+        )
        {
            // SPEC: (also applies to the CheckInvocationEscape counterpart)
            //
            //            An lvalue resulting from a ref-returning method invocation e1.M(e2, ...) is ref-safe - to - escape the smallest of the following scopes:


smallest [](start = 129, length = 8)

Nit: "smallest" is ambiguous. Consider using "narrowest" here and a couple of lines below.

That is the terminology from the spec proposal. I think we should fix it there.
Here it is less important, since it is just a comment. Will fix when the spec is updated. (hopefully there will be no changes to rules too :-)

jcouv · 2017-09-20T16:43:25Z

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

                    if (conditional.IsByRef)
                    {
-                        return GetRefEscape(conditional.Consequence, scopeOfTheContainingExpression);
+                        // ref conditional defers to its operands
+                        return Math.Max(GetRefEscape(conditional.Consequence, scopeOfTheContainingExpression),


Are we sure we want to relax the ref-safe-to-escape for ternary? Might that cause problems once we allow ref re-assignment (ie. the ternary could be used on the left)?

It cannot be on the left of ref assignment.

It would ned to be a ref to a ref.

jcouv · 2017-09-20T16:49:19Z

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

@@ -2105,7 +2123,8 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
                        checkingReceiver,
                        escapeFrom,
                        escapeTo,
-                        diagnostics);
+                        diagnostics,
+                        isRefEscape: false);

                case BoundKind.ObjectCreationExpression:


It just occurred to me, are we covering tuples?

Tuples cannot contain ref-structs since they are themselves not ref structs

jcouv

LGTM

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

VSadov requested a review from a team September 19, 2017 22:08

VSadov force-pushed the relaxedEscape branch from 37f38a6 to 65c4648 Compare September 19, 2017 22:30

OmarTawfik reviewed Sep 19, 2017

View reviewed changes

VSadov added 4 commits September 19, 2017 18:22

relaxed escape rules

daae851

Fixes: dotnet#22197

a48e9f8

fix after merge

96dd144

PR feedback

7b33a48

VSadov force-pushed the relaxedEscape branch from 4c73d49 to 7b33a48 Compare September 20, 2017 01:28

jaredpar approved these changes Sep 20, 2017

View reviewed changes

jcouv reviewed Sep 20, 2017

View reviewed changes

jcouv approved these changes Sep 20, 2017

View reviewed changes

VSadov merged commit 698b9e0 into dotnet:features/readonly-ref Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relaxed escape rules for spans #22203

Relaxed escape rules for spans #22203

VSadov commented Sep 19, 2017 •

edited

Loading

VSadov commented Sep 19, 2017

OmarTawfik Sep 19, 2017 •

edited by VSadov

Loading

VSadov Sep 19, 2017

OmarTawfik Sep 19, 2017 •

edited by VSadov

Loading

VSadov Sep 19, 2017 •

edited

Loading

OmarTawfik Sep 19, 2017 •

edited by VSadov

Loading

VSadov Sep 20, 2017

VSadov Sep 20, 2017

VSadov commented Sep 20, 2017

jcouv Sep 20, 2017

VSadov Sep 20, 2017

jcouv Sep 20, 2017

VSadov Sep 20, 2017

VSadov Sep 20, 2017

jcouv Sep 20, 2017

VSadov Sep 20, 2017

jcouv left a comment

		@@ -662,10 +662,7 @@ static void Main()
		comp.VerifyEmitDiagnostics(
		// (15,27): error CS8168: Cannot return local 'local1' by reference because it is not a ref local

Relaxed escape rules for spans #22203

Relaxed escape rules for spans #22203

Conversation

VSadov commented Sep 19, 2017 • edited Loading

VSadov commented Sep 19, 2017

OmarTawfik Sep 19, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarTawfik Sep 19, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

VSadov Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

OmarTawfik Sep 19, 2017 • edited by VSadov Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VSadov commented Sep 20, 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

jcouv left a comment

Choose a reason for hiding this comment

VSadov commented Sep 19, 2017 •

edited

Loading

OmarTawfik Sep 19, 2017 •

edited by VSadov

Loading

OmarTawfik Sep 19, 2017 •

edited by VSadov

Loading

VSadov Sep 19, 2017 •

edited

Loading

OmarTawfik Sep 19, 2017 •

edited by VSadov

Loading