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.

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

Combine deconstruction assignment and declaration, and support discards. #15548

Merged
merged 10 commits into from
Dec 12, 2016
155 changes: 93 additions & 62 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,32 +16,21 @@ namespace Microsoft.CodeAnalysis.CSharp
/// </summary>
internal partial class Binder
{
/// <summary>
/// Only handles assignment-only or declaration-only deconstructions at this point.
/// Issue https://github.com/dotnet/roslyn/issues/15050 tracks allowing mixed deconstructions
/// </summary>
private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
{
var left = node.Left;
var right = node.Right;
CSharpSyntaxNode declaration = null;
Copy link
Member

@jcouv jcouv Nov 30, 2016

Choose a reason for hiding this comment

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

Since declaration and expression must be either null or node, could they just be Boolean values (isDeclaration and isExpression)? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

No, they are used as a location for a diagnostic below.


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

CSharpSyntaxNode expression = null;
var result = BindDeconstruction(node, left, right, diagnostics, ref declaration, ref expression);
if (declaration != null && expression != null)
{
// We only allow assignment-only or declaration-only deconstructions at this point.
// Issue https://github.com/dotnet/roslyn/issues/15050 tracks allowing mixed deconstructions.
// For now we give an error when you mix.
Error(diagnostics, ErrorCode.ERR_MixedDeconstructionUnsupported, left);
Copy link
Member

@cston cston Nov 28, 2016

Choose a reason for hiding this comment

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

Please add a test for this case. #Resolved

Copy link
Member Author

@gafter gafter Nov 28, 2016

Choose a reason for hiding this comment

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

There already is one: MixedDeconstruction_03 in DeconstructionTests.cs


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

}

bool isDeclaration = node.IsDeconstructionDeclaration();
Debug.Assert(isDeclaration || !ContainsDeclarations(left));
return BindDeconstruction(node, left, right, diagnostics, isDeclaration);
}

internal BoundDeconstructionAssignmentOperator BindDeconstruction(
CSharpSyntaxNode node,
ExpressionSyntax left,
ExpressionSyntax right,
DiagnosticBag diagnostics,
bool isDeclaration,
BoundDeconstructValuePlaceholder rightPlaceholder = null)
{
DeconstructionVariable locals = BindDeconstructionVariables(left, isDeclaration, diagnostics);
Debug.Assert(locals.HasNestedVariables);
var result = BindDeconstructionAssignment(node, right, locals.NestedVariables, diagnostics, rhsPlaceholder: rightPlaceholder);
FreeDeconstructionVariables(locals.NestedVariables);
return result;
}

Expand Down Expand Up @@ -122,8 +111,8 @@ private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(
constructionStepsOpt);

TypeSymbol returnType = hasErrors ?
CreateErrorType() :
constructionStepsOpt.Last().OutputPlaceholder.Type;
CreateErrorType() :
constructionStepsOpt.Last().OutputPlaceholder.Type;

var deconstructions = deconstructionSteps.ToImmutableAndFree();
var conversions = conversionSteps.ToImmutableAndFree();
Expand Down Expand Up @@ -257,29 +246,40 @@ private void SetInferredTypes(ArrayBuilder<DeconstructionVariable> variables, Im
var variable = variables[i];
if (!variable.HasNestedVariables)
{
switch (variable.Single.Kind)
var pending = variable.Single;
if ((object)pending.Type != null)
{
case BoundKind.DeconstructionVariablePendingInference:
{
var pending = (DeconstructionVariablePendingInference)variable.Single;
BoundExpression local = pending.SetInferredType(foundTypes[i], this, diagnostics);
variables[i] = new DeconstructionVariable(local, local.Syntax);
}
break;
case BoundKind.DiscardedExpression:
{
var pending = (BoundDiscardedExpression)variable.Single;
if ((object)pending.Type == null)
{
variables[i] = new DeconstructionVariable(pending.SetInferredType(foundTypes[i]), pending.Syntax);
}
}
break;
continue;
}

variables[i] = new DeconstructionVariable(SetInferredType(pending, foundTypes[i], diagnostics), pending.Syntax);
}
}
}

private BoundExpression SetInferredType(BoundExpression expression, TypeSymbol type, DiagnosticBag diagnostics)
{
switch (expression.Kind)
{
case BoundKind.DeconstructionVariablePendingInference:
{
var pending = (DeconstructionVariablePendingInference)expression;
return pending.SetInferredType(type, this, diagnostics);
}
case BoundKind.DiscardedExpression:
{
var pending = (BoundDiscardedExpression)expression;
if ((object)pending.Type == null)
{
Copy link
Member Author

@gafter gafter Nov 28, 2016

Choose a reason for hiding this comment

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

Since the caller has already checked this, I will remove the if and add an assertion that the type is null. #Resolved

return pending.SetInferredType(type);
}
}
break;
}

return expression;
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 30, 2016

Choose a reason for hiding this comment

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

return expression; [](start = 12, length = 18)

Is this line of code reachable? Do we have a test covering it? #Closed

}

/// <summary>
/// Find any deconstruction locals that are still pending inference and fail their inference.
/// </summary>
Expand Down Expand Up @@ -633,22 +633,44 @@ private BoundBadExpression MissingDeconstruct(BoundExpression receiver, CSharpSy
return BadExpression(syntax, childNode);
}

internal BoundDeconstructionAssignmentOperator BindDeconstruction(
CSharpSyntaxNode deconstruction,
ExpressionSyntax left,
ExpressionSyntax right,
DiagnosticBag diagnostics,
ref CSharpSyntaxNode declaration,
ref CSharpSyntaxNode expression,
BoundDeconstructValuePlaceholder rightPlaceholder = null)
{
DeconstructionVariable locals = BindDeconstructionVariables(left, diagnostics, ref declaration, ref expression);
Debug.Assert(locals.HasNestedVariables);
var result = BindDeconstructionAssignment(deconstruction, right, locals.NestedVariables, diagnostics, rhsPlaceholder: rightPlaceholder);
FreeDeconstructionVariables(locals.NestedVariables);
return result;
}

/// <summary>
/// Returns bound variables in a tree.
/// For variables that are being declared, it makes locals (or fields in global statement).
/// If the type is unknown, a deconstruction variable pending inference is used instead (which will be replaced with a local or field later).
/// For expressions that don't declare variables, simply binds them and verify they are assignable to.
///
/// Prepares locals (or fields in global statement) and lvalue expressions corresponding to the variables of the declaration.
/// The locals/fields/lvalues are kept in a tree which captures the nesting of variables.
/// Each local or field is either a simple local or field access (when its type is known) or a deconstruction variable pending inference.
/// The caller is responsible for releasing the nested ArrayBuilders.
/// </summary>
private DeconstructionVariable BindDeconstructionVariables(ExpressionSyntax node, bool isDeclaration, DiagnosticBag diagnostics)
private DeconstructionVariable BindDeconstructionVariables(
ExpressionSyntax node,
DiagnosticBag diagnostics,
ref CSharpSyntaxNode declaration,
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 30, 2016

Choose a reason for hiding this comment

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

ref CSharpSyntaxNode declaration, [](start = 12, length = 33)

This and the next parameter deserve a comment or should be renamed to use more descriptive names. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Nov 30, 2016

Choose a reason for hiding this comment

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

CSharpSyntaxNode [](start = 16, length = 16)

It looks like we can use more specific type for this parameter - DeclarationExpressionSyntax. #Closed

Copy link
Member Author

@gafter gafter Nov 30, 2016

Choose a reason for hiding this comment

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

The caller doesn't need that, but I'm OK with using a more specific type here. #Resolved

ref CSharpSyntaxNode expression)
{
switch (node.Kind())
{
case SyntaxKind.DeclarationExpression:
{
var component = (DeclarationExpressionSyntax)node;
if (declaration == null)
{
declaration = node;
}

var component = (DeclarationExpressionSyntax)node;
bool isVar;
bool isConst = false;
AliasSymbol alias;
Expand All @@ -660,35 +682,38 @@ private DeconstructionVariable BindDeconstructionVariables(ExpressionSyntax node
Error(diagnostics, ErrorCode.ERR_DeconstructionVarFormDisallowsSpecificType, component.Designation);
}

return BindDeconstructionVariables(declType, component.Type, component.Designation, diagnostics);
return BindDeconstructionVariables(declType, component.Designation, diagnostics);
}
case SyntaxKind.TupleExpression:
{
var component = (TupleExpressionSyntax)node;
var builder = ArrayBuilder<DeconstructionVariable>.GetInstance(component.Arguments.Count);
foreach (var arg in component.Arguments)
{
builder.Add(BindDeconstructionVariables(arg.Expression, isDeclaration, diagnostics));
if (arg.NameColon != null)
{
Error(diagnostics, ErrorCode.ERR_TupleElementNamesInDeconstruction, arg.NameColon);
}

builder.Add(BindDeconstructionVariables(arg.Expression, diagnostics, ref declaration, ref expression));
}

return new DeconstructionVariable(builder, node);
}
default:
var boundVariable = BindExpression(node, diagnostics, invoked: false, indexed: false);
var checkedVariable = CheckValue(boundVariable, BindValueKind.Assignment, diagnostics);
if (expression == null && checkedVariable.Kind != BoundKind.DiscardedExpression)
{
var boundVariable = BindExpression(node, diagnostics, invoked: false, indexed: false);
if (isDeclaration && boundVariable.Kind != BoundKind.DiscardedExpression )
{
Error(diagnostics, ErrorCode.ERR_MixedDeconstructionDisallowed, node, node);
}
var checkedVariable = CheckValue(boundVariable, BindValueKind.Assignment, diagnostics);
return new DeconstructionVariable(checkedVariable, node);
expression = node;
}

return new DeconstructionVariable(checkedVariable, node);
}
}

private DeconstructionVariable BindDeconstructionVariables(
TypeSymbol declType,
TypeSyntax typeSyntax,
VariableDesignationSyntax node,
DiagnosticBag diagnostics)
{
Expand All @@ -697,20 +722,20 @@ private DeconstructionVariable BindDeconstructionVariables(
case SyntaxKind.SingleVariableDesignation:
{
var single = (SingleVariableDesignationSyntax)node;
return new DeconstructionVariable(BindDeconstructionVariable(declType, typeSyntax, single, diagnostics), node);
return new DeconstructionVariable(BindDeconstructionVariable(declType, single, diagnostics), node);
}
case SyntaxKind.DiscardedDesignation:
{
var discarded = (DiscardedDesignationSyntax)node;
return new DeconstructionVariable(new BoundDiscardedExpression(discarded, declType), node);
return new DeconstructionVariable(BindDiscardedExpression(discarded, declType), node);
}
case SyntaxKind.ParenthesizedVariableDesignation:
{
var tuple = (ParenthesizedVariableDesignationSyntax)node;
var builder = ArrayBuilder<DeconstructionVariable>.GetInstance();
foreach (var n in tuple.Variables)
{
builder.Add(BindDeconstructionVariables(declType, typeSyntax, n, diagnostics));
builder.Add(BindDeconstructionVariables(declType, n, diagnostics));
}
return new DeconstructionVariable(builder, node);
}
Expand All @@ -719,14 +744,20 @@ private DeconstructionVariable BindDeconstructionVariables(
}
}

private BoundDiscardedExpression BindDiscardedExpression(
DiscardedDesignationSyntax designation,
TypeSymbol declType)
Copy link
Member

@cston cston Nov 28, 2016

Choose a reason for hiding this comment

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

static #Closed

{
return new BoundDiscardedExpression(designation, declType);
}

/// <summary>
/// In embedded statements, returns a BoundLocal when the type was explicit.
/// In global statements, returns a BoundFieldAccess when the type was explicit.
/// Otherwise returns a DeconstructionVariablePendingInference when the type is implicit.
/// </summary>
private BoundExpression BindDeconstructionVariable(
TypeSymbol declType,
TypeSyntax typeSyntax,
SingleVariableDesignationSyntax designation,
DiagnosticBag diagnostics)
{
Expand All @@ -742,7 +773,7 @@ private BoundExpression BindDeconstructionVariable(

if ((object)declType != null)
{
CheckRestrictedTypeInAsync(this.ContainingMemberOrLambda, declType, diagnostics, typeSyntax);
CheckRestrictedTypeInAsync(this.ContainingMemberOrLambda, declType, diagnostics, designation);
return new BoundLocal(designation, localSymbol, constantValueOpt: null, type: declType, hasErrors: hasErrors);
}

Expand Down
57 changes: 57 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,9 @@ private BoundExpression BindExpressionInternal(ExpressionSyntax node, Diagnostic
CreateErrorType("ref"));
}

case SyntaxKind.DeclarationExpression:
return BindDeclarationExpression((DeclarationExpressionSyntax)node, diagnostics);

default:
// NOTE: We could probably throw an exception here, but it's conceivable
// that a non-parser syntax tree could reach this point with an unexpected
Expand Down Expand Up @@ -703,6 +706,60 @@ private static bool IsThrowExpressionInProperContext(ThrowExpressionSyntax node)
}
}

// Bind a declaration expression where it isn't permitted.
private BoundExpression BindDeclarationExpression(DeclarationExpressionSyntax node, DiagnosticBag diagnostics)
{
// This is an error, as declaration expressions are handled specially in every context in which
// they are permitted. So we have a context in which they are *not* permitted. Nevertheless, we
// bind it and then give one nice message.

bool isVar;
bool isConst = false;
AliasSymbol alias;
TypeSymbol declType = BindVariableType(node.Designation, diagnostics, node.Type, ref isConst, out isVar, out alias);
Error(diagnostics, ErrorCode.ERR_DeclarationExpressionNotPermitted, node);
return BindDeclarationVariables(declType, node.Designation, diagnostics);
}

// Bind a declaration variable where it isn't permitted.
private BoundExpression BindDeclarationVariables(TypeSymbol declType, VariableDesignationSyntax node, DiagnosticBag diagnostics)
Copy link
Member

@cston cston Nov 28, 2016

Choose a reason for hiding this comment

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

Is this comment correct? From the body of the method, it's not clear that the declaration variable isn't permitted. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

While it is true that the code implements them, this is used only to bind them in contexts in which they are not currently permitted, via the method above which gives an error message. As you can see, the type inference (line 735 below) is short-circuited to produce an error type.


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

Copy link
Contributor

@AlekseyTs AlekseyTs Nov 29, 2016

Choose a reason for hiding this comment

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

diagnostics [](start = 124, length = 11)

I am not sure why do we want to collect any additional errors besides the one already reported by the caller. It feels like that will only create more noise. #Closed

Copy link
Member Author

@gafter gafter Nov 30, 2016

Choose a reason for hiding this comment

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

We could drop the diagnostics from this call on the floor, but I believe they will be useful to the user when (1) the user refactors their code to use local declarations, or (2) we begin allowing this under more constrained circumstances. If we see complaints about cascaded diagnostics, we should consider refactoring. I'm fine with it as it stands. #ByDesign

{
switch (node.Kind())
{
case SyntaxKind.SingleVariableDesignation:
{
var single = (SingleVariableDesignationSyntax)node;
var result = BindDeconstructionVariable(declType, single, diagnostics);

// for error recovery, we force inference of the type of a misplaced declaration expression
if ((object)declType == null)
{
result = SetInferredType(result, CreateErrorType("var"), diagnostics);
}
return result;
}
case SyntaxKind.DiscardedDesignation:
{
var discarded = (DiscardedDesignationSyntax)node;
return BindDiscardedExpression(discarded, declType);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 29, 2016

Choose a reason for hiding this comment

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

declType [](start = 66, length = 8)

Should we pass an error type if declType is null? #Closed

Copy link
Member Author

@gafter gafter Nov 30, 2016

Choose a reason for hiding this comment

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

Yes; I'll move that to entry to this method. #Resolved

}
case SyntaxKind.ParenthesizedVariableDesignation:
{
var tuple = (ParenthesizedVariableDesignationSyntax)node;
var builder = ArrayBuilder<BoundExpression>.GetInstance();
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 29, 2016

Choose a reason for hiding this comment

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

GetInstance(); [](start = 68, length = 14)

Can we predict the number of elements we will need? #Closed

foreach (var n in tuple.Variables)
{
builder.Add(BindDeclarationVariables(declType, n, diagnostics));
}
var subExpressions = builder.ToImmutableAndFree();
var tupleType = (TypeSymbol)this.Compilation.CreateTupleTypeSymbol(subExpressions.Select<BoundExpression, ITypeSymbol>(e => e.Type).ToImmutableArray());
Copy link
Member

@cston cston Nov 28, 2016

Choose a reason for hiding this comment

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

Consider using TupleTypeSymbol directly rather than ITypeSymbol, etc.:

TupleTypeSymbol.Create(
    null,
    subExpressions.SelectAsArray(e => e.Type),
    default(ImmutableArray<Location>),
    default(ImmutableArray<string>,
    Compilation);
``` #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs Nov 29, 2016

Choose a reason for hiding this comment

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

var tupleType = (TypeSymbol)this.Compilation.CreateTupleTypeSymbol(subExpressions.Select<BoundExpression, ITypeSymbol>(e => e.Type).ToImmutableArray()); [](start = 24, length = 152)

Please use TupleTypeSymbol.Create API instead, see how BindTupleExpression does this. #Closed

return new BoundTupleLiteral(node, ImmutableArray<string>.Empty, subExpressions, tupleType);
Copy link
Member

@cston cston Nov 28, 2016

Choose a reason for hiding this comment

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

Should argumentNamesOpt be default(ImmutableArray<string>) rather than Empty? #Resolved

}
default:
throw ExceptionUtilities.UnexpectedValue(node.Kind());
}
}

private BoundExpression BindTupleExpression(TupleExpressionSyntax node, DiagnosticBag diagnostics)
{
SeparatedSyntaxList<ArgumentSyntax> arguments = node.Arguments;
Expand Down
Loading