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 all 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
76 changes: 73 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 @@ -1993,6 +2014,14 @@ internal static uint GetValEscape(BoundExpression expr, uint scopeOfTheContainin
// always returnable
return Binder.ExternalScope;

case BoundKind.TupleLiteral:
var tupleLiteral = (BoundTupleLiteral)expr;
return GetTupleValEscape(tupleLiteral.Arguments, scopeOfTheContainingExpression);

case BoundKind.ConvertedTupleLiteral:
var convertedTupleLiteral = (BoundConvertedTupleLiteral)expr;
return GetTupleValEscape(convertedTupleLiteral.Arguments, scopeOfTheContainingExpression);

case BoundKind.MakeRefOperator:
case BoundKind.RefValueOperator:
// for compat reasons
Expand All @@ -2004,6 +2033,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 @@ -2166,6 +2198,17 @@ internal static uint GetValEscape(BoundExpression expr, uint scopeOfTheContainin
}
}

private static uint GetTupleValEscape(ImmutableArray<BoundExpression> elements, uint scopeOfTheContainingExpression)
{
uint narrowestScope = scopeOfTheContainingExpression;
foreach (var element in elements)
{
narrowestScope = Math.Max(narrowestScope, GetValEscape(element, scopeOfTheContainingExpression));
}

return narrowestScope;
}

private static uint GetValEscapeOfObjectInitializer(BoundObjectInitializerExpression initExpr, uint scopeOfTheContainingExpression)
{
var result = Binder.ExternalScope;
Expand Down Expand Up @@ -2240,6 +2283,14 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
// always returnable
return true;

case BoundKind.TupleLiteral:
var tupleLiteral = (BoundTupleLiteral)expr;
return CheckTupleValEscape(tupleLiteral.Arguments, escapeFrom, escapeTo, diagnostics);

case BoundKind.ConvertedTupleLiteral:
var convertedTupleLiteral = (BoundConvertedTupleLiteral)expr;
return CheckTupleValEscape(convertedTupleLiteral.Arguments, escapeFrom, escapeTo, diagnostics);

case BoundKind.MakeRefOperator:
case BoundKind.RefValueOperator:
// for compat reasons
Expand All @@ -2249,6 +2300,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 +2523,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 +2599,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 Expand Up @@ -2577,6 +2634,19 @@ internal static bool CheckValEscape(SyntaxNode node, BoundExpression expr, uint
}
}

private static bool CheckTupleValEscape(ImmutableArray<BoundExpression> elements, uint escapeFrom, uint escapeTo, DiagnosticBag diagnostics)
{
foreach (var element in elements)
{
if (!CheckValEscape(element.Syntax, element, escapeFrom, escapeTo, checkingReceiver: false, diagnostics: diagnostics))
{
return false;
}
}

return true;
}

private static bool CheckValEscapeOfObjectInitializer(BoundObjectInitializerExpression initExpr, uint escapeFrom, uint escapeTo, DiagnosticBag diagnostics)
{
foreach (var expression in initExpr.Initializers)
Expand Down
59 changes: 39 additions & 20 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 @@ -5,7 +5,6 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
Expand Down Expand Up @@ -95,13 +94,15 @@ internal BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, Dia
DeconstructionVariable locals = BindDeconstructionVariables(left, diagnostics, ref declaration, ref expression);
Debug.Assert(locals.HasNestedVariables);

BoundExpression boundRight = rightPlaceholder ?? BindValue(right, diagnostics, BindValueKind.RValue);
boundRight = FixTupleLiteral(locals.NestedVariables, boundRight, deconstruction, diagnostics);
var deconstructionDiagnostics = new DiagnosticBag();
BoundExpression boundRight = rightPlaceholder ?? BindValue(right, deconstructionDiagnostics, BindValueKind.RValue);
boundRight = FixTupleLiteral(locals.NestedVariables, boundRight, deconstruction, deconstructionDiagnostics);

bool resultIsUsed = resultIsUsedOverride || IsDeconstructionResultUsed(left);
var assignment = BindDeconstructionAssignment(deconstruction, left, boundRight, locals.NestedVariables, resultIsUsed, diagnostics);
var assignment = BindDeconstructionAssignment(deconstruction, left, boundRight, locals.NestedVariables, resultIsUsed, deconstructionDiagnostics);
DeconstructionVariable.FreeDeconstructionVariables(locals.NestedVariables);

diagnostics.AddRange(deconstructionDiagnostics);
return assignment;
}

Expand All @@ -113,10 +114,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 +142,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);
var lhsTuple = DeconstructionVariablesAsTuple(left, checkedVariables, diagnostics, ignoreDiagnosticsFromTuple: diagnostics.HasAnyErrors() || !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 @@ -197,7 +203,10 @@ private BoundExpression FixTupleLiteral(ArrayBuilder<DeconstructionVariable> che
// Let's fix the literal up by figuring out its type
// For declarations, that means merging type information from the LHS and RHS
// For assignments, only the LHS side matters since it is necessarily typed
TypeSymbol mergedTupleType = MakeMergedTupleType(checkedVariables, (BoundTupleLiteral)boundRHS, syntax, Compilation, diagnostics);

// If we already have diagnostics at this point, it is not worth collecting likely duplicate diagnostics from making the merged type
bool hadErrors = diagnostics.HasAnyErrors();
TypeSymbol mergedTupleType = MakeMergedTupleType(checkedVariables, (BoundTupleLiteral)boundRHS, syntax, Compilation, hadErrors ? null : diagnostics);
if ((object)mergedTupleType != null)
{
boundRHS = GenerateConversionForAssignment(mergedTupleType, boundRHS, diagnostics);
Expand Down Expand Up @@ -248,7 +257,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 +354,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 @@ -439,6 +457,7 @@ private static TypeSymbol MakeMergedTupleType(ArrayBuilder<DeconstructionVariabl
int rightLength = rhsLiteral.Arguments.Length;

var typesBuilder = ArrayBuilder<TypeSymbol>.GetInstance(leftLength);
var locationsBuilder = ArrayBuilder<Location>.GetInstance(leftLength);
for (int i = 0; i < rightLength; i++)
{
BoundExpression element = rhsLiteral.Arguments[i];
Expand Down Expand Up @@ -479,29 +498,29 @@ private static TypeSymbol MakeMergedTupleType(ArrayBuilder<DeconstructionVariabl
}

typesBuilder.Add(mergedType);
locationsBuilder.Add(element.Syntax.Location);
}

if (typesBuilder.Any(t => t == null))
{
typesBuilder.Free();
locationsBuilder.Free();
return null;
}

// The tuple created here is not identical to the one created by
// DeconstructionVariablesAsTuple. It represents a smaller
// tree of types used for figuring out natural types in tuple literal.
// Therefore, we do not check constraints here as it would report errors
// that are already reported later. DeconstructionVariablesAsTuple
// constructs the final tuple type and checks constraints.
return TupleTypeSymbol.Create(
locationOpt: null,
elementTypes: typesBuilder.ToImmutableAndFree(),
elementLocations: default(ImmutableArray<Location>),
elementLocations: locationsBuilder.ToImmutableAndFree(),
elementNames: default(ImmutableArray<string>),
compilation: compilation,
diagnostics: diagnostics,
shouldCheckConstraints: false,
errorPositions: default(ImmutableArray<bool>));
shouldCheckConstraints: true,
errorPositions: default(ImmutableArray<bool>),
syntax: syntax);
}

private BoundTupleLiteral DeconstructionVariablesAsTuple(CSharpSyntaxNode syntax, ArrayBuilder<DeconstructionVariable> variables,
Expand Down Expand Up @@ -672,7 +691,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
Loading