Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Escape rules for deconstruction and foreach variables #22354

Merged
merged 6 commits into from
Sep 29, 2017
Merged

Escape rules for deconstruction and foreach variables #22354

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5943daf
Escape rules for deconstruction and foreach variables
Sep 26, 2017
462c0e2
Tweaks
jcouv Sep 26, 2017
952792a
Tweaks
jcouv Sep 26, 2017
a135d99
Report constraint errors on deconstruction of typeless tuple
Sep 27, 2017
5ec929b
Pre-emptively implement val escape logic for tuples
jcouv Sep 27, 2017
68d0b49
Always make the merged tuple type for tuple deconstructions
jcouv Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1958,6 +1958,27 @@ internal static bool CheckRefEscape(SyntaxNode node, BoundExpression expr, uint
return false;
}

internal static uint GetBroadestValEscape(BoundTupleExpression expr, uint scopeOfTheContainingExpression)
{
uint broadest = scopeOfTheContainingExpression;
foreach (var element in expr.Arguments)
{
uint valEscape;
if (element.Kind == BoundKind.TupleLiteral)
Copy link
Member

@gafter gafter Sep 26, 2017 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

{
valEscape = GetBroadestValEscape((BoundTupleExpression)element, scopeOfTheContainingExpression);
}
else
{
valEscape = GetValEscape(element, scopeOfTheContainingExpression);
}

broadest = Math.Min(broadest, valEscape);
}

return broadest;
}

/// <summary>
/// Computes the widest scope depth to which the given expression can escape by value.
///
Expand Down Expand Up @@ -1990,6 +2011,8 @@ internal static uint GetValEscape(BoundExpression expr, uint scopeOfTheContainin
case BoundKind.DefaultExpression:
case BoundKind.Parameter:
case BoundKind.ThisReference:
case BoundKind.TupleLiteral:
case BoundKind.ConvertedTupleLiteral:
// always returnable
return Binder.ExternalScope;

Expand All @@ -2004,6 +2027,9 @@ internal static uint GetValEscape(BoundExpression expr, uint scopeOfTheContainin
// same as uninitialized local
return Binder.ExternalScope;

case BoundKind.DeconstructValuePlaceholder:
return ((BoundDeconstructValuePlaceholder)expr).ValEscape;

case BoundKind.Local:
return ((BoundLocal)expr).LocalSymbol.ValEscapeScope;

Expand Down Expand Up @@ -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;
Copy link
Member

@gafter gafter Sep 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I corrected the comment just above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


Expand All @@ -2249,6 +2277,15 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
// same as uninitialized local
return true;

case BoundKind.DeconstructValuePlaceholder:
var placeholder = (BoundDeconstructValuePlaceholder)expr;
if (placeholder.ValEscape > escapeTo)
{
Error(diagnostics, ErrorCode.ERR_EscapeLocal, node, placeholder.Syntax);
return false;
}
return true;

case BoundKind.Local:
var localSymbol = ((BoundLocal)expr).LocalSymbol;
if (localSymbol.ValEscapeScope > escapeTo)
Expand Down Expand Up @@ -2463,8 +2500,6 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
// case BoundKind.NameOfOperator:
// case BoundKind.InterpolatedString:
// case BoundKind.StringInsert:
// case BoundKind.TupleLiteral:
// case BoundKind.ConvertedTupleLiteral:
// case BoundKind.DynamicIndexerAccess:
// case BoundKind.Lambda:
// case BoundKind.DynamicObjectCreationExpression:
Expand Down Expand Up @@ -2541,7 +2576,6 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
// case BoundKind.DeclarationPattern:
// case BoundKind.ConstantPattern:
// case BoundKind.WildcardPattern:
// case BoundKind.DeconstructValuePlaceholder:

#endregion

Expand Down
30 changes: 22 additions & 8 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,12 @@ internal BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Dia
bool resultIsUsed,
DiagnosticBag diagnostics)
{
uint rightEscape = GetValEscape(boundRHS, this.LocalScopeDepth);

if ((object)boundRHS.Type == null || boundRHS.Type.IsErrorType())
{
// we could still not infer a type for the RHS
FailRemainingInferences(checkedVariables, diagnostics);
FailRemainingInferencesAndSetValEscape(checkedVariables, diagnostics, rightEscape);
var voidType = GetSpecialType(SpecialType.System_Void, diagnostics, node);

var type = boundRHS.Type ?? voidType;
Expand All @@ -139,11 +141,14 @@ internal BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Dia
checkedVariables,
out conversion);

FailRemainingInferences(checkedVariables, diagnostics);
FailRemainingInferencesAndSetValEscape(checkedVariables, diagnostics, rightEscape);

var lhsTuple = DeconstructionVariablesAsTuple(left, checkedVariables, diagnostics, ignoreDiagnosticsFromTuple: hasErrors || !resultIsUsed);
TypeSymbol returnType = hasErrors ? CreateErrorType() : lhsTuple.Type;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


var boundConversion = new BoundConversion(
boundRHS.Syntax,
boundRHS,
Expand Down Expand Up @@ -248,7 +253,7 @@ private BoundExpression FixTupleLiteral(ArrayBuilder<DeconstructionVariable> che
else
{
ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders;
var inputPlaceholder = new BoundDeconstructValuePlaceholder(syntax, type);
var inputPlaceholder = new BoundDeconstructValuePlaceholder(syntax, this.LocalScopeDepth, type);
var deconstructInvocation = MakeDeconstructInvocationExpression(variables.Count,
inputPlaceholder, rightSyntax, diagnostics, out outPlaceholders);

Expand Down Expand Up @@ -345,24 +350,33 @@ private BoundExpression SetInferredType(BoundExpression expression, TypeSymbol t

/// <summary>
/// Find any deconstruction locals that are still pending inference and fail their inference.
/// Set the safe-to-escape scope for all deconstruction locals.
/// </summary>
private void FailRemainingInferences(ArrayBuilder<DeconstructionVariable> variables, DiagnosticBag diagnostics)
private void FailRemainingInferencesAndSetValEscape(ArrayBuilder<DeconstructionVariable> variables, DiagnosticBag diagnostics,
uint rhsValEscape)
{
int count = variables.Count;
for (int i = 0; i < count; i++)
{
var variable = variables[i];
if (variable.HasNestedVariables)
{
FailRemainingInferences(variable.NestedVariables, diagnostics);
FailRemainingInferencesAndSetValEscape(variable.NestedVariables, diagnostics, rhsValEscape);
}
else
{
switch (variable.Single.Kind)
{
case BoundKind.Local:
var local = (BoundLocal)variable.Single;
if (local.IsDeclaration)
{
((SourceLocalSymbol)local.LocalSymbol).SetValEscape(rhsValEscape);
}
break;
case BoundKind.DeconstructionVariablePendingInference:
BoundExpression local = ((DeconstructionVariablePendingInference)variable.Single).FailInference(this, diagnostics);
variables[i] = new DeconstructionVariable(local, local.Syntax);
BoundExpression errorLocal = ((DeconstructionVariablePendingInference)variable.Single).FailInference(this, diagnostics);
variables[i] = new DeconstructionVariable(errorLocal, errorLocal.Syntax);
break;
case BoundKind.DiscardExpression:
var pending = (BoundDiscardExpression)variable.Single;
Expand Down Expand Up @@ -672,7 +686,7 @@ private static string ExtractDeconstructResultElementName(BoundExpression expres
out ImmutableArray<BoundDeconstructValuePlaceholder> outPlaceholders, BoundExpression childNode)
{
Error(diagnostics, ErrorCode.ERR_MissingDeconstruct, rightSyntax, receiver.Type, numParameters);
outPlaceholders = default(ImmutableArray<BoundDeconstructValuePlaceholder>);
outPlaceholders = default;

return BadExpression(rightSyntax, childNode);
}
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2536,7 +2536,7 @@ private BoundExpression BindArgumentExpression(DiagnosticBag diagnostics, Expres
else if (argument.Kind == BoundKind.OutDeconstructVarPendingInference)
{
TypeSymbol parameterType = GetCorrespondingParameterType(ref result, parameters, arg);
arguments[arg] = ((OutDeconstructVarPendingInference)argument).SetInferredType(parameterType, success: true);
arguments[arg] = ((OutDeconstructVarPendingInference)argument).SetInferredType(parameterType, this, success: true);
}
else if (argument.Kind == BoundKind.DiscardExpression && !argument.HasExpressionType())
{
Expand Down
22 changes: 15 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,10 @@ internal override BoundStatement BindForEachDeconstruction(DiagnosticBag diagnos
bool hasErrors = !GetEnumeratorInfoAndInferCollectionElementType(ref builder, ref collectionExpr, diagnostics, out inferredType);

ExpressionSyntax variables = ((ForEachVariableStatementSyntax)_syntax).Variable;
var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, inferredType ?? CreateErrorType("var"));

// Tracking narrowest safe-to-escape scope by default, the proper val escape will be set when doing full binding of the foreach statement
var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, this.LocalScopeDepth, inferredType ?? CreateErrorType("var"));

DeclarationExpressionSyntax declaration = null;
ExpressionSyntax expression = null;
BoundDeconstructionAssignmentOperator deconstruction = BindDeconstruction(
Expand Down Expand Up @@ -202,6 +205,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
BoundTypeExpression boundIterationVariableType;
bool hasNameConflicts = false;
BoundForEachDeconstructStep deconstructStep = null;
uint collectionEscape = GetValEscape(collectionExpr, this.LocalScopeDepth);
switch (_syntax.Kind())
{
case SyntaxKind.ForEachStatement:
Expand Down Expand Up @@ -230,7 +234,11 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
}

boundIterationVariableType = new BoundTypeExpression(typeSyntax, alias, iterationVariableType);
this.IterationVariable.SetType(iterationVariableType);

SourceLocalSymbol local = this.IterationVariable;
local.SetType(iterationVariableType);
local.SetValEscape(collectionEscape);

break;
}
case SyntaxKind.ForEachVariableStatement:
Expand All @@ -241,7 +249,7 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
var variables = node.Variable;
if (variables.IsDeconstructionLeft())
{
var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, iterationVariableType);
var valuePlaceholder = new BoundDeconstructValuePlaceholder(_syntax.Expression, collectionEscape, iterationVariableType);
DeclarationExpressionSyntax declaration = null;
ExpressionSyntax expression = null;
BoundDeconstructionAssignmentOperator deconstruction = BindDeconstruction(
Expand Down Expand Up @@ -283,8 +291,8 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,
// I.E. - they will be considered declared and assigned in each iteration step.
ImmutableArray<LocalSymbol> iterationVariables = this.Locals;

Debug.Assert(hasErrors ||
_syntax.HasErrors ||
Debug.Assert(hasErrors ||
_syntax.HasErrors ||
iterationVariables.All(local => local.DeclarationKind == LocalDeclarationKind.ForEachIterationVariable),
"Should not have iteration variables that are not ForEachIterationVariable in valid code");

Expand Down Expand Up @@ -349,8 +357,8 @@ private BoundForEachStatement BindForEachPartsWorker(DiagnosticBag diagnostics,

var getEnumeratorType = builder.GetEnumeratorMethod.ReturnType;
// we never convert struct enumerators to object - it is done only for null-checks.
builder.EnumeratorConversion = getEnumeratorType.IsValueType?
Conversion.Identity:
builder.EnumeratorConversion = getEnumeratorType.IsValueType ?
Conversion.Identity :
this.Conversions.ClassifyConversionFromType(getEnumeratorType, GetSpecialType(SpecialType.System_Object, diagnostics, _syntax), ref useSiteDiagnostics);

if (getEnumeratorType.IsRestrictedType() && (IsDirectlyInIterator || IsInAsyncMethod()))
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
It is used to perform intermediate binding, and will not survive the local rewriting.
-->
<Node Name="BoundDeconstructValuePlaceholder" Base="BoundValuePlaceholderBase">
<Field Name="ValEscape" Type="uint" Null="NotApplicable"/>
</Node>

<!-- only used by codegen -->
Expand Down
9 changes: 5 additions & 4 deletions src/Compilers/CSharp/Portable/BoundTree/OutDeconstructVarPendingInference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,18 @@ internal partial class OutDeconstructVarPendingInference
{
public BoundDeconstructValuePlaceholder Placeholder;

public BoundDeconstructValuePlaceholder SetInferredType(TypeSymbol type, bool success)
public BoundDeconstructValuePlaceholder SetInferredType(TypeSymbol type, Binder binder, bool success)
{
Debug.Assert((object)Placeholder == null);
Debug.Assert(Placeholder is null);

Placeholder = new BoundDeconstructValuePlaceholder(this.Syntax, type, hasErrors: this.HasErrors || !success);
// The val escape scope for this placeholder won't be used, so defaulting to narrowest scope
Placeholder = new BoundDeconstructValuePlaceholder(this.Syntax, binder.LocalScopeDepth, type, hasErrors: this.HasErrors || !success);
return Placeholder;
}

public BoundDeconstructValuePlaceholder FailInference(Binder binder)
{
return SetInferredType(binder.CreateErrorType(), success: false);
return SetInferredType(binder.CreateErrorType(), binder, success: false);
}
}
}
17 changes: 11 additions & 6 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,33 +412,37 @@ protected BoundValuePlaceholderBase(BoundKind kind, SyntaxNode syntax, TypeSymbo

internal sealed partial class BoundDeconstructValuePlaceholder : BoundValuePlaceholderBase
{
public BoundDeconstructValuePlaceholder(SyntaxNode syntax, TypeSymbol type, bool hasErrors)
public BoundDeconstructValuePlaceholder(SyntaxNode syntax, uint valEscape, TypeSymbol type, bool hasErrors)
: base(BoundKind.DeconstructValuePlaceholder, syntax, type, hasErrors)
{

Debug.Assert(type != null, "Field 'type' cannot be null (use Null=\"allow\" in BoundNodes.xml to remove this check)");

this.ValEscape = valEscape;
}

public BoundDeconstructValuePlaceholder(SyntaxNode syntax, TypeSymbol type)
public BoundDeconstructValuePlaceholder(SyntaxNode syntax, uint valEscape, TypeSymbol type)
: base(BoundKind.DeconstructValuePlaceholder, syntax, type)
{

Debug.Assert(type != null, "Field 'type' cannot be null (use Null=\"allow\" in BoundNodes.xml to remove this check)");

this.ValEscape = valEscape;
}


public uint ValEscape { get; }

public override BoundNode Accept(BoundTreeVisitor visitor)
{
return visitor.VisitDeconstructValuePlaceholder(this);
}

public BoundDeconstructValuePlaceholder Update(TypeSymbol type)
public BoundDeconstructValuePlaceholder Update(uint valEscape, TypeSymbol type)
{
if (type != this.Type)
if (valEscape != this.ValEscape || type != this.Type)
{
var result = new BoundDeconstructValuePlaceholder(this.Syntax, type, this.HasErrors);
var result = new BoundDeconstructValuePlaceholder(this.Syntax, valEscape, type, this.HasErrors);
result.WasCompilerGenerated = this.WasCompilerGenerated;
return result;
}
Expand Down Expand Up @@ -8466,7 +8470,7 @@ public override BoundNode VisitGlobalStatementInitializer(BoundGlobalStatementIn
public override BoundNode VisitDeconstructValuePlaceholder(BoundDeconstructValuePlaceholder node)
{
TypeSymbol type = this.VisitType(node.Type);
return node.Update(type);
return node.Update(node.ValEscape, type);
}
public override BoundNode VisitDup(BoundDup node)
{
Expand Down Expand Up @@ -9377,6 +9381,7 @@ public override TreeDumperNode VisitDeconstructValuePlaceholder(BoundDeconstruct
{
return new TreeDumperNode("deconstructValuePlaceholder", null, new TreeDumperNode[]
{
new TreeDumperNode("valEscape", node.ValEscape, null),
new TreeDumperNode("type", node.Type, null)
}
);
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/Source/SourceLocalSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ internal virtual void SetRefEscape(uint value)

internal virtual void SetValEscape(uint value)
{
throw ExceptionUtilities.Unreachable;
_valEscapeScope = value;
}

public override Symbol ContainingSymbol
Expand Down
Loading