-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes from 1 commit
1da1e53
1003470
887d224
ac9c1a3
cc42c17
711b6b0
f90f62b
c4d4991
c5e4534
3e0d9db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test for this case. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There already is one: 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; | ||
} | ||
|
||
|
@@ -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(); | ||
|
@@ -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) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the caller has already checked this, I will remove the |
||
return pending.SetInferredType(type); | ||
} | ||
} | ||
break; | ||
} | ||
|
||
return expression; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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> | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This and the next parameter deserve a comment or should be renamed to use more descriptive names. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It looks like we can use more specific type for this parameter - DeclarationExpressionSyntax. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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) | ||
{ | ||
|
@@ -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); | ||
} | ||
|
@@ -719,14 +744,20 @@ private DeconstructionVariable BindDeconstructionVariables( | |
} | ||
} | ||
|
||
private BoundDiscardedExpression BindDiscardedExpression( | ||
DiscardedDesignationSyntax designation, | ||
TypeSymbol declType) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
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) | ||
{ | ||
|
@@ -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); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Should we pass an error type if There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please use TupleTypeSymbol.Create API instead, see how BindTupleExpression does this. #Closed |
||
return new BoundTupleLiteral(node, ImmutableArray<string>.Empty, subExpressions, tupleType); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should |
||
} | ||
default: | ||
throw ExceptionUtilities.UnexpectedValue(node.Kind()); | ||
} | ||
} | ||
|
||
private BoundExpression BindTupleExpression(TupleExpressionSyntax node, DiagnosticBag diagnostics) | ||
{ | ||
SeparatedSyntaxList<ArgumentSyntax> arguments = node.Arguments; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
declaration
andexpression
must be eithernull
ornode
, could they just be Boolean values (isDeclaration
andisExpression
)? #ClosedThere was a problem hiding this comment.
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)