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

D-declaration should use declaration expressions #14871

Merged
merged 8 commits into from
Nov 8, 2016
Merged

D-declaration should use declaration expressions #14871

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dc2fe06
WIP on syntax
jcouv Nov 1, 2016
d0f94c3
Updating binding and semantic model to new syntax
jcouv Nov 2, 2016
ecce74d
Updating editor features to adopt new syntax model
jcouv Nov 4, 2016
31e9421
Fix issue with EE scenario
jcouv Nov 5, 2016
7400a9f
PR feedback (1)
jcouv Nov 7, 2016
d5bede6
PR feedback (2)
jcouv Nov 8, 2016
7e4ea6e
Moving IsDeconstructionDeclarationLeft method
jcouv Nov 8, 2016
d39e75e
IDE feedback (1)
jcouv Nov 8, 2016
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: 44 additions & 32 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 @@ -11,22 +11,51 @@
namespace Microsoft.CodeAnalysis.CSharp
{
/// <summary>
/// This portion of the binder converts deconstruction-assignment syntax (AssignmentExpressionSyntax nodes with the left being a tuple expression)
/// into a BoundDeconstructionAssignmentOperator (or bad node).
/// This portion of the binder converts deconstruction-assignment syntax (AssignmentExpressionSyntax nodes with the left
/// being a tuple expression or declaration expression) into a BoundDeconstructionAssignmentOperator (or bad node).
/// </summary>
internal partial class Binder
{
private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
private BoundExpression BindDeconstruction(AssignmentExpressionSyntax node, DiagnosticBag diagnostics)
{
var left = (TupleExpressionSyntax)node.Left;
ArrayBuilder<DeconstructionVariable> checkedVariables = BindDeconstructionAssignmentVariables(left.Arguments, left, diagnostics);
var left = node.Left;
var right = node.Right;

if (node.IsDeconstructionDeclaration())
{
return BindDeconstructionDeclaration(node, left, right, diagnostics);
}

// We only parse assignment-only or declaration-only deconstructions at this point
AssertDeconstructionIsAssignment(left);
Copy link
Member

@cston cston Nov 7, 2016 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Is the comment correct? Given the assert, it looks like we're only expecting to bind (rather than parse) assignments, not declarations. #Resolved

Copy link
Member

@cston cston Nov 7, 2016 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Perhaps remove the assert altogether since node.IsDeconstructionDeclaration() above performed the same check. #Resolved

Copy link
Member Author

@jcouv jcouv Nov 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The first check verifies that all the variables are declared. The second check verifies that all the variables are just assigned too.
The second check tells use that we are not in a mixed scenario (some assignments and some declarations), such as (x, var y) = e;, which only will be supported later. #Resolved

Copy link
Member Author

@jcouv jcouv Nov 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought, the comment is indeed misleading. I meant "at this point in time", not "at this point in the method". Sorry for the confusion. #Resolved


var tuple = (TupleExpressionSyntax)left;
ArrayBuilder<DeconstructionVariable> checkedVariables = BindDeconstructionAssignmentVariables(tuple.Arguments, tuple, diagnostics);
var result = BindDeconstructionAssignment(node, node.Right, checkedVariables, diagnostics, isDeclaration: false);
FreeDeconstructionVariables(checkedVariables);

return result;
}

[Conditional("DEBUG")]
private void AssertDeconstructionIsAssignment(ExpressionSyntax expression)
{
switch (expression.Kind())
{
case SyntaxKind.DeclarationExpression:
Debug.Assert(false);
break;
case SyntaxKind.TupleExpression:
var tuple = (TupleExpressionSyntax)expression;
foreach (var arg in tuple.Arguments)
{
AssertDeconstructionIsAssignment(arg.Expression);
}
break;
default:
return;
}
}

private static void FreeDeconstructionVariables(ArrayBuilder<DeconstructionVariable> variables)
{
foreach (var v in variables)
Expand Down Expand Up @@ -606,24 +635,7 @@ private static void FlattenDeconstructVariables(ArrayBuilder<DeconstructionVaria
return BadExpression(syntax, childNode);
}

internal BoundLocalDeconstructionDeclaration BindDeconstructionDeclarationStatement(DeconstructionDeclarationStatementSyntax node, DiagnosticBag diagnostics)
{
bool modifierErrors;

// No modifiers are allowed in deconstruction declarations
ModifierUtils.MakeAndCheckNontypeMemberModifiers(
modifiers: node.Modifiers,
defaultAccess: DeclarationModifiers.None,
allowedModifiers: DeclarationModifiers.None,
errorLocation: node.Assignment.VariableComponent.Location,
diagnostics: diagnostics,
modifierErrors: out modifierErrors);

var assignment = BindDeconstructionDeclaration(node, node.Assignment.VariableComponent, node.Assignment.Value, diagnostics);
return new BoundLocalDeconstructionDeclaration(node, assignment, hasErrors: modifierErrors);
}

internal BoundDeconstructionAssignmentOperator BindDeconstructionDeclaration(CSharpSyntaxNode node, VariableComponentSyntax declaration, ExpressionSyntax right,
internal BoundDeconstructionAssignmentOperator BindDeconstructionDeclaration(CSharpSyntaxNode node, ExpressionSyntax declaration, ExpressionSyntax right,
DiagnosticBag diagnostics, BoundDeconstructValuePlaceholder rightPlaceholder = null)
{
DeconstructionVariable locals = BindDeconstructionDeclarationVariables(declaration, diagnostics);
Expand All @@ -640,23 +652,23 @@ internal BoundLocalDeconstructionDeclaration BindDeconstructionDeclarationStatem
/// The caller is responsible for releasing the nested ArrayBuilders.
/// </summary>
private DeconstructionVariable BindDeconstructionDeclarationVariables(
VariableComponentSyntax node,
ExpressionSyntax node,
DiagnosticBag diagnostics)
{
switch (node.Kind())
{
case SyntaxKind.TypedVariableComponent:
case SyntaxKind.DeclarationExpression:
{
var component = (TypedVariableComponentSyntax)node;
var component = (DeclarationExpressionSyntax)node;
return BindDeconstructionDeclarationVariables(component.Type, component.Designation, diagnostics);
}
case SyntaxKind.ParenthesizedVariableComponent:
case SyntaxKind.TupleExpression:
{
var component = (ParenthesizedVariableComponentSyntax)node;
var builder = ArrayBuilder<DeconstructionVariable>.GetInstance(component.Variables.Count);
foreach (var n in component.Variables)
var component = (TupleExpressionSyntax)node;
var builder = ArrayBuilder<DeconstructionVariable>.GetInstance(component.Arguments.Count);
foreach (var arg in component.Arguments)
{
builder.Add(BindDeconstructionDeclarationVariables(n, diagnostics));
builder.Add(BindDeconstructionDeclarationVariables(arg.Expression, diagnostics));
}
return new DeconstructionVariable(builder, node);
}
Expand Down
9 changes: 5 additions & 4 deletions 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 @@ -2095,11 +2095,12 @@ private BoundExpression BindArgumentValue(DiagnosticBag diagnostics, ArgumentSyn

private BoundExpression BindOutVariableArgument(DeclarationExpressionSyntax declarationExpression, DiagnosticBag diagnostics)
{
var typeSyntax = declarationExpression.Type();
TypeSyntax typeSyntax = declarationExpression.Type;
var designation = (SingleVariableDesignationSyntax)declarationExpression.Designation;
bool isVar;

// Is this a local?
SourceLocalSymbol localSymbol = this.LookupLocal(declarationExpression.Identifier());
SourceLocalSymbol localSymbol = this.LookupLocal(designation.Identifier);

if ((object)localSymbol != null)
{
Expand Down Expand Up @@ -2130,15 +2131,15 @@ private BoundExpression BindOutVariableArgument(DeclarationExpressionSyntax decl
}

// Is this a field?
GlobalExpressionVariable expressionVariableField = LookupDeclaredField(declarationExpression.VariableDesignation());
GlobalExpressionVariable expressionVariableField = LookupDeclaredField(designation);

if ((object)expressionVariableField == null)
{
// We should have the right binder in the chain, cannot continue otherwise.
throw ExceptionUtilities.Unreachable;
}

BoundExpression receiver = SynthesizeReceiver(declarationExpression.VariableDesignation(), expressionVariableField, diagnostics);
BoundExpression receiver = SynthesizeReceiver(designation, expressionVariableField, diagnostics);

if (typeSyntax.IsVar)
{
Expand Down
10 changes: 3 additions & 7 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,6 @@ public virtual BoundStatement BindStatement(StatementSyntax node, DiagnosticBag
case SyntaxKind.LocalDeclarationStatement:
result = BindLocalDeclarationStatement((LocalDeclarationStatementSyntax)node, diagnostics);
break;
case SyntaxKind.DeconstructionDeclarationStatement:
result = BindDeconstructionDeclarationStatement((DeconstructionDeclarationStatementSyntax)node, diagnostics);
break;
case SyntaxKind.LocalFunctionStatement:
result = BindLocalFunctionStatement((LocalFunctionStatementSyntax)node, diagnostics);
break;
Expand All @@ -68,7 +65,7 @@ public virtual BoundStatement BindStatement(StatementSyntax node, DiagnosticBag
result = BindFor((ForStatementSyntax)node, diagnostics);
break;
case SyntaxKind.ForEachStatement:
case SyntaxKind.ForEachComponentStatement:
case SyntaxKind.ForEachVariableStatement:
result = BindForEach((CommonForEachStatementSyntax)node, diagnostics);
break;
case SyntaxKind.BreakStatement:
Expand Down Expand Up @@ -283,7 +280,6 @@ internal BoundStatement BindPossibleEmbeddedStatement(StatementSyntax node, Diag
case SyntaxKind.IfStatement:
case SyntaxKind.YieldReturnStatement:
case SyntaxKind.LocalDeclarationStatement:
case SyntaxKind.DeconstructionDeclarationStatement:
case SyntaxKind.ReturnStatement:
case SyntaxKind.ThrowStatement:
binder = this.GetBinder(node);
Expand Down Expand Up @@ -1729,9 +1725,9 @@ private BoundExpression BindAssignment(AssignmentExpressionSyntax node, Diagnost
Debug.Assert(node.Left != null);
Debug.Assert(node.Right != null);

if (node.Left.Kind() == SyntaxKind.TupleExpression)
if (node.Left.Kind() == SyntaxKind.TupleExpression || node.Left.Kind() == SyntaxKind.DeclarationExpression)
{
return BindDeconstructionAssignment(node, diagnostics);
return BindDeconstruction(node, diagnostics);
}

var op1 = BindValue(node.Left, diagnostics, BindValueKind.Assignment); // , BIND_MEMBERSET);
Expand Down
128 changes: 118 additions & 10 deletions src/Compilers/CSharp/Portable/Binder/ExpressionVariableFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,16 +210,16 @@ public override void VisitDeclarationPattern(DeclarationPatternSyntax node)

public override void VisitQueryExpression(QueryExpressionSyntax node)
{
// Variables declared in [in] expressions of top level from clause and
// join clauses are in scope
// Variables declared in [in] expressions of top level from clause and
// join clauses are in scope
VisitNodeToBind(node.FromClause.Expression);
Visit(node.Body);
}

public override void VisitQueryBody(QueryBodySyntax node)
{
// Variables declared in [in] expressions of top level from clause and
// join clauses are in scope
// Variables declared in [in] expressions of top level from clause and
// join clauses are in scope
foreach (var clause in node.Clauses)
{
if (clause.Kind() == SyntaxKind.JoinClause)
Expand Down Expand Up @@ -268,7 +268,83 @@ public override void VisitDeclarationExpression(DeclarationExpressionSyntax node
}
}

public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
Copy link
Member

@gafter gafter Nov 7, 2016 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

public [](start = 8, length = 6)

This appears more complicated than necessary, but I think it is fine for now. #Resolved

{
if (node.IsDeconstructionDeclaration())
{
CollectVariablesFromDeconstruction(node.Left, node);
}
else
{
Visit(node.Left);
}

Visit(node.Right);
}

private void CollectVariablesFromDeconstruction(
ExpressionSyntax declaration,
AssignmentExpressionSyntax deconstruction)
Copy link
Member

@cston cston Nov 7, 2016 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

static #Resolved

Copy link
Member Author

@jcouv jcouv Nov 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

The variables are collected in_localsBuilder. #Resolved

{
switch (declaration.Kind())
{
case SyntaxKind.TupleExpression:
{
var tuple = (TupleExpressionSyntax)declaration;
foreach (var arg in tuple.Arguments)
{
CollectVariablesFromDeconstruction(arg.Expression, deconstruction);
}
break;
}
case SyntaxKind.DeclarationExpression:
{
var declarationExpression = (DeclarationExpressionSyntax)declaration;
CollectVariablesFromDeconstruction(declarationExpression.Designation, declarationExpression.Type, deconstruction);
break;
}
default:
throw ExceptionUtilities.UnexpectedValue(declaration.Kind());
}
}

private void CollectVariablesFromDeconstruction(
VariableDesignationSyntax designation,
TypeSyntax closestTypeSyntax,
AssignmentExpressionSyntax deconstruction)
{
switch (designation.Kind())
{
case SyntaxKind.SingleVariableDesignation:
{
var single = (SingleVariableDesignationSyntax)designation;
var variable = MakeDeconstructionVariable(closestTypeSyntax, single, deconstruction);
if ((object)variable != null)
{
_localsBuilder.Add(variable);
}
break;
}
case SyntaxKind.ParenthesizedVariableDesignation:
{
var tuple = (ParenthesizedVariableDesignationSyntax)designation;
foreach (var d in tuple.Variables)
{
CollectVariablesFromDeconstruction(d, closestTypeSyntax, deconstruction);
}
break;
}
default:
throw ExceptionUtilities.UnexpectedValue(designation.Kind());
}
}

protected abstract TFieldOrLocalSymbol MakeOutVariable(DeclarationExpressionSyntax node, BaseArgumentListSyntax argumentListSyntax, SyntaxNode nodeToBind);

protected abstract TFieldOrLocalSymbol MakeDeconstructionVariable(
TypeSyntax closestTypeSyntax,
SingleVariableDesignationSyntax designation,
AssignmentExpressionSyntax deconstruction);
}

internal class ExpressionVariableFinder : ExpressionVariableFinder<LocalSymbol>
Expand Down Expand Up @@ -344,9 +420,10 @@ protected override LocalSymbol MakePatternVariable(DeclarationPatternSyntax node
protected override LocalSymbol MakeOutVariable(DeclarationExpressionSyntax node, BaseArgumentListSyntax argumentListSyntax, SyntaxNode nodeToBind)
{
NamedTypeSymbol container = _scopeBinder.ContainingType;
var designation = (SingleVariableDesignationSyntax)node.Designation;

if ((object)container != null && container.IsScriptClass &&
(object)_scopeBinder.LookupDeclaredField(node.VariableDesignation()) != null)
if ((object)container != null && container.IsScriptClass &&
(object)_scopeBinder.LookupDeclaredField(designation) != null)
{
// This is a field declaration
return null;
Expand All @@ -356,13 +433,28 @@ protected override LocalSymbol MakeOutVariable(DeclarationExpressionSyntax node,
containingSymbol: _scopeBinder.ContainingMemberOrLambda,
scopeBinder: _scopeBinder,
nodeBinder: _enclosingBinder,
typeSyntax: node.Type(),
identifierToken: node.Identifier(),
typeSyntax: node.Type,
identifierToken: designation.Identifier,
kind: LocalDeclarationKind.RegularVariable,
nodeToBind: nodeToBind,
forbiddenZone: argumentListSyntax);
}

protected override LocalSymbol MakeDeconstructionVariable(
TypeSyntax closestTypeSyntax,
SingleVariableDesignationSyntax designation,
AssignmentExpressionSyntax deconstruction)
{
return SourceLocalSymbol.MakeDeconstructionLocal(
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

return SourceLocalSymbol.MakeDeconstructionLocal( [](start = 12, length = 49)

Are we certain that we don't need to check that we might have already created a field for this declaration? #Closed

Copy link
Member Author

@jcouv jcouv Nov 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'll stop by. I didn't understand the concern. #Resolved

containingSymbol: _scopeBinder.ContainingMemberOrLambda,
scopeBinder: _scopeBinder,
nodeBinder: _enclosingBinder,
closestTypeSyntax: closestTypeSyntax,
identifierToken: designation.Identifier,
kind: LocalDeclarationKind.RegularVariable,
deconstruction: deconstruction);
}

#region pool
private static readonly ObjectPool<ExpressionVariableFinder> s_poolInstance = CreatePool();

Expand Down Expand Up @@ -414,13 +506,29 @@ protected override Symbol MakePatternVariable(DeclarationPatternSyntax node, Syn

protected override Symbol MakeOutVariable(DeclarationExpressionSyntax node, BaseArgumentListSyntax argumentListSyntax, SyntaxNode nodeToBind)
{
var designation = node.VariableDesignation();
var designation = (SingleVariableDesignationSyntax)node.Designation;
return GlobalExpressionVariable.Create(
_containingType, _modifiers, node.Type(),
_containingType, _modifiers, node.Type,
designation.Identifier.ValueText, designation, designation.Identifier.GetLocation(),
_containingFieldOpt, nodeToBind);
}

protected override Symbol MakeDeconstructionVariable(
TypeSyntax closestTypeSyntax,
SingleVariableDesignationSyntax designation,
AssignmentExpressionSyntax deconstruction)
{
return GlobalExpressionVariable.Create(
containingType: _containingType,
modifiers: DeclarationModifiers.Private,
typeSyntax: closestTypeSyntax,
name: designation.Identifier.ValueText,
syntax: designation,
location: designation.Location,
containingFieldOpt: null,
nodeToBind: deconstruction);
}

#region pool
private static readonly ObjectPool<ExpressionFieldFinder> s_poolInstance = CreatePool();

Expand Down
Loading