Escape rules for deconstruction and foreach variables #22354

jcouv · 2017-09-26T21:49:47Z

Customer scenario
Use ref-like types (such as Span) in a deconstruction or foreach loop. The assignments implicit in those operations need to be tracked to avoid holding on to references past their scope.

Bugs this fixes:
Fixes #22338

Details of the fix

Variables introduced by deconstructions and foreach (regular or deconstruction form) need to track their safe-to-escape scope, as dictated by the right-hand-side of a deconstruction and the foreach collection, respectively.
Expressions in the left-hand-side of a deconstruction need to abide by safe-to-escape rules for assignment: their safe-to-escape scopes should be narrower than the value they will receive from the right-hand-side.

Workarounds, if any
No workaround. This is a safety rule the compiler should enforce.

Risk
Performance impact
Low. This is mostly adding a check to binding deconstructions and foreach.

Is this a regression from a previous update?
No. This is part of a C# 7.2 feature (part of 15.5 release).

jcouv · 2017-09-26T22:11:54Z

@gafter for review. Thanks

gafter · 2017-09-26T23:46:20Z

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

+            foreach (var element in expr.Arguments)
+            {
+                uint valEscape;
+                if (element.Kind == BoundKind.TupleLiteral)


if [](start = 16, length = 2)

I would expect this test to be part of the switch in GetValEscape. It seems out of place here. #Resolved

Oh, I see, in GetValEscape we would be using Math.Max.

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

gafter · 2017-09-26T23:59:14Z

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

@@ -2237,6 +2263,8 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
                case BoundKind.DefaultExpression:
                case BoundKind.Parameter:
                case BoundKind.ThisReference:
+                case BoundKind.TupleLiteral:
+                case BoundKind.ConvertedTupleLiteral:
                    // always returnable
                    return true;


true [](start = 27, length = 4)

I don't think this is correct for a tuple literal. For example, I could write

(spanA, someString) = (spanB, null);

This is all valid in the type system even if spanA and spanB are ref structs (because the right-hand-side has no type), but the safe-to-escape value computed from the right-hand-side is from the safe-to-escape of the variable tupleB, and the whole thing is only valid if the assignment spanA = spanB would be valid.

I corrected the comment just above.

Thanks for the scenario.
I don't agree with your expectation though. The tuple on the right does receive a type, by inferring a merged type from both sides. In this case, the RHS will be made into a (Span, string) which is illegal.
So (spanA, someString) = (spanB, null); should produce an constraint error (Span cannot be used as a type argument).

In current circumstances a tuple literal cannot not contain ref-like elements (That could be considered a bug, but it is what it is). Considering that, returning val-escape being External seems to be valid.
Once we allow tuples to contain ref-likes, we will need to revisit this place. For now it seems valid assumption,

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

gafter

I think there is a remaining issue for code such as (spanA, someString) = (spanB, null);, which it appears this code would permit even if spanA = spanB would be rejected.

jcouv · 2017-09-27T21:18:29Z

Filed feature request to allow deconstruction without requiring ValueTuple type, even when the right-hand-side is a tuple literal (#22382).
When we get to that, we'll have to handle safe-to-escape rules in more details (doing element-wise checks). I took a note in the issue to remind ourselves.

jcouv · 2017-09-27T21:19:09Z

@VSadov @dotnet/roslyn-compiler for review. Thanks

jcouv · 2017-09-27T21:22:10Z

src/Compilers/CSharp/Test/Symbol/Compilation/SemanticModelGetDeclaredSymbolAPITests.cs

+            var tree = compilation.SyntaxTrees[0];
+            var decl = (ArgumentSyntax)tree.GetCompilationUnitRoot().DescendantNodes().Last(n => n.IsKind(SyntaxKind.Argument));
+            var model = compilation.GetSemanticModel(tree);
+            Assert.Null(model.GetDeclaredSymbol(decl));


📝 There is a slight regression of the semantic model here. But (1) I don't think it is very significant, and (2) I think it is worthwhile because the root cause is a change to avoid many duplicate diagnostics.

VSadov · 2017-09-27T21:35:57Z

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

            TypeSymbol returnType = hasErrors ? CreateErrorType() : lhsTuple.Type;

+            uint leftEscape = GetBroadestValEscape(lhsTuple, this.LocalScopeDepth);
+            boundRHS = ValidateEscape(boundRHS, leftEscape, isByRef: false, diagnostics: diagnostics);


In the future we may want to do this pair-wise each value to each assignment target.
For now, since in all reachable cases the assignment values will have the same escape which is equal to escape of RHS, this is sufficient.

VSadov

jcouv · 2017-09-28T22:11:21Z

@gafter Please review when you have the chance.
As we discussed I added an implementation of val escape for tuples.
Also, I realized after our discussion that I should always compute the merged tuple type for a deconstruction (even when I will discard the diagnostics that produces). The latest commit fixes that.

gafter

jcouv · 2017-09-29T19:06:42Z

test windows_debug_unit32_prtest please

jcouv · 2017-09-29T19:41:42Z

test windows_debug_unit32_prtest please

* dotnet/master: Escape rules for deconstruction and foreach variables (dotnet#22354) Address review feedback Address PR feedback Add IOperation unit tests for try/catch statements and make the APIs public again

Escape rules for deconstruction and foreach variables

5943daf

jcouv added Area-Compilers New Language Feature - Readonly References Readonly References labels Sep 26, 2017

jcouv added this to the 15.5 milestone Sep 26, 2017

jcouv self-assigned this Sep 26, 2017

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

Tweaks

462c0e2

Tweaks

952792a

gafter reviewed Sep 26, 2017

View reviewed changes

gafter reviewed Sep 27, 2017

View reviewed changes

jcouv force-pushed the value-escape branch 2 times, most recently from 22686fb to b55f46e Compare September 27, 2017 20:05

Report constraint errors on deconstruction of typeless tuple

a135d99

jcouv force-pushed the value-escape branch from b55f46e to a135d99 Compare September 27, 2017 21:15

jcouv commented Sep 27, 2017

View reviewed changes

VSadov reviewed Sep 27, 2017

View reviewed changes

VSadov approved these changes Sep 27, 2017

View reviewed changes

jcouv added 2 commits September 27, 2017 16:27

Pre-emptively implement val escape logic for tuples

5ec929b

Always make the merged tuple type for tuple deconstructions

68d0b49

jcouv assigned gafter Sep 29, 2017

gafter approved these changes Sep 29, 2017

View reviewed changes

jcouv merged commit 3d3b524 into dotnet:master Sep 29, 2017

jcouv deleted the value-escape branch September 29, 2017 20:52

jcouv added the Approved to merge label Sep 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape rules for deconstruction and foreach variables #22354

Escape rules for deconstruction and foreach variables #22354

jcouv commented Sep 26, 2017

jcouv commented Sep 26, 2017

gafter Sep 26, 2017 •

edited by jcouv

Loading

gafter Sep 26, 2017

gafter Sep 26, 2017 •

edited

Loading

gafter Sep 27, 2017

jcouv Sep 27, 2017

VSadov Sep 27, 2017

gafter left a comment

jcouv commented Sep 27, 2017 •

edited

Loading

jcouv commented Sep 27, 2017

jcouv Sep 27, 2017

VSadov Sep 27, 2017

VSadov left a comment

jcouv commented Sep 28, 2017

gafter left a comment

jcouv commented Sep 29, 2017

jcouv commented Sep 29, 2017

Escape rules for deconstruction and foreach variables #22354

Escape rules for deconstruction and foreach variables #22354

Conversation

jcouv commented Sep 26, 2017

jcouv commented Sep 26, 2017

gafter Sep 26, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

gafter Sep 26, 2017

Choose a reason for hiding this comment

gafter Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

gafter Sep 27, 2017

Choose a reason for hiding this comment

jcouv Sep 27, 2017

Choose a reason for hiding this comment

VSadov Sep 27, 2017

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

jcouv commented Sep 27, 2017 • edited Loading

jcouv commented Sep 27, 2017

jcouv Sep 27, 2017

Choose a reason for hiding this comment

VSadov Sep 27, 2017

Choose a reason for hiding this comment

VSadov left a comment

Choose a reason for hiding this comment

jcouv commented Sep 28, 2017

gafter left a comment

Choose a reason for hiding this comment

jcouv commented Sep 29, 2017

jcouv commented Sep 29, 2017

gafter Sep 26, 2017 •

edited by jcouv

Loading

gafter Sep 26, 2017 •

edited

Loading

jcouv commented Sep 27, 2017 •

edited

Loading