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

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

Merged
merged 10 commits into from
Dec 12, 2016
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1da1e53
Combine deconstruction assignment and declaration, and support discards.
gafter Nov 10, 2016
1003470
Disallow deconstruction declarations except at the statement level.
gafter Nov 28, 2016
887d224
Revise logic to detect `var` in a declaration expression.
gafter Nov 30, 2016
ac9c1a3
Add a test that GetTypeInfo on a discard expression doesn't crash.
gafter Nov 30, 2016
cc42c17
Small changes per code review.
gafter Nov 30, 2016
711b6b0
Add (skipped) test for var invocation in parens.
gafter Nov 30, 2016
f90f62b
Rename "Discarded" to "Discard"
gafter Dec 1, 2016
c4d4991
Changes recommended via code review.
gafter Dec 6, 2016
c5e4534
Minor changes to the handling of declaration expressions per code rev…
gafter Dec 8, 2016
3e0d9db
Addressing blocking feedback while Neal is OOF
jcouv Dec 9, 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
32 changes: 17 additions & 15 deletions docs/features/wildcards.work.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,28 @@ work items remaining for wildcards
- [ ] Gather together a specification document
- [ ] Language behavior (e.g. [this](https://github.com/dotnet/roslyn/issues/14862) and [this](https://github.com/dotnet/roslyn/issues/14794) and [this](https://github.com/dotnet/roslyn/issues/14832))
- [ ] [SemanticModel behavior](https://gist.github.com/gafter/ab10e413efe3a066209cbf14cb874988) (see also [here](https://gist.github.com/gafter/37305d619bd04511f4f66b86f6f2d3a5))
- [ ] Warnings (for non-wildcard expression variables declared but not used)
- [ ] Debugging
- [ ] Warnings (for expression variables declared but not used)
- [x] Debugging (value discarded is not visible in debugger)

### Compiler
- [x] Syntax model changes
- [ ] Symbol changes
- [ ] Parsing for the short-form wildcard pattern `_`
- [ ] Implement binding of wildcards
- [ ] In a pattern
- [ ] In a deconstruction declaration
- [ ] In a deconstruction assignment expression
- [x] Symbol changes
- [x] Parsing for the short-form wildcard pattern `_`
- [x] Implement binding of wildcards
- [x] In a pattern
- [x] In a deconstruction declaration
- [x] In a deconstruction assignment expression
- [ ] In an out argument (in every argument context)
- [ ] Both the long form `var _` and the short form `_`
- [x] Both the long form `var _` and the short form `_`
- [x] Type inference for wildcards in each context
- [ ] Implement semantic model changes
- [ ] Type inference for wildcards in each context
- [ ] Implement lowering of wildcards
- [ ] In a pattern
- [ ] In a deconstruction declaration
- [ ] In a deconstruction assignment expression
- [ ] In an out argument (in every argument context)
- [ ] `GetTypeInfo` of a wildcard expression `_` should be the type of the discarded value
- [ ] `GetSymbolInfo` of a wildcard expression `_` should be an `IDiscardedSymbol`
- [x] Implement lowering of wildcards
- [x] In a pattern
- [x] In a deconstruction declaration
- [x] In a deconstruction assignment expression
- [ ] In an out argument (in each argument context)

### Testing
- [ ] Symbol tests
Expand Down
209 changes: 130 additions & 79 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
View file
Open in desktop

Large diffs are not rendered by default.

73 changes: 66 additions & 7 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 @@ -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,62 @@ 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);
}

/// <summary>
/// Bind a declaration variable where it isn't permitted. The caller is expected to produce a diagnostic.
/// </summary>
private BoundExpression BindDeclarationVariables(TypeSymbol declType, VariableDesignationSyntax node, DiagnosticBag diagnostics)
Copy link
Member

@cston cston Nov 28, 2016 •
edited by gafter
Loading

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 •
edited
Loading

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 •
edited
Loading

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

{
declType = declType ?? CreateErrorType("var");
switch (node.Kind())
{
case SyntaxKind.SingleVariableDesignation:
{
var single = (SingleVariableDesignationSyntax)node;
var result = BindDeconstructionVariable(declType, single, diagnostics);
return result;
}
case SyntaxKind.DiscardDesignation:
{
var discarded = (DiscardDesignationSyntax)node;
return BindDiscardExpression(discarded, declType);
}
case SyntaxKind.ParenthesizedVariableDesignation:
{
var tuple = (ParenthesizedVariableDesignationSyntax)node;
var builder = ArrayBuilder<BoundExpression>.GetInstance(tuple.Variables.Count);
foreach (var n in tuple.Variables)
{
builder.Add(BindDeclarationVariables(declType, n, diagnostics));
}
var subExpressions = builder.ToImmutableAndFree();
var tupleType = TupleTypeSymbol.Create(
null,
subExpressions.SelectAsArray(e => e.Type),
default(ImmutableArray<Location>),
default(ImmutableArray<string>),
Compilation);
return new BoundTupleLiteral(node, default(ImmutableArray<string>), subExpressions, tupleType);
}
default:
throw ExceptionUtilities.UnexpectedValue(node.Kind());
}
}

private BoundExpression BindTupleExpression(TupleExpressionSyntax node, DiagnosticBag diagnostics)
{
SeparatedSyntaxList<ArgumentSyntax> arguments = node.Arguments;
Expand Down Expand Up @@ -1144,7 +1203,7 @@ private BoundExpression BindIdentifier(
{
if (node.IsKind(SyntaxKind.IdentifierName) && FallBackOnDiscard((IdentifierNameSyntax)node, diagnostics))
{
return new BoundDiscardedExpression(node, type: null);
return new BoundDiscardExpression(node, type: null);
}

// Otherwise, the simple-name is undefined and a compile-time error occurs.
Expand Down Expand Up @@ -2127,15 +2186,15 @@ private BoundExpression BindOutVariableArgument(DeclarationExpressionSyntax decl
VariableDesignationSyntax designation = declarationExpression.Designation;
switch (designation.Kind())
{
case SyntaxKind.DiscardedDesignation:
case SyntaxKind.DiscardDesignation:
{
bool isVar;
bool isConst = false;
AliasSymbol alias;
TypeSymbol declType = BindVariableType(designation, diagnostics, typeSyntax, ref isConst, out isVar, out alias);
Debug.Assert(isVar == ((object)declType == null));

return new BoundDiscardedExpression((DiscardedDesignationSyntax)designation, declType);
return new BoundDiscardExpression((DiscardDesignationSyntax)designation, declType);
}
case SyntaxKind.SingleVariableDesignation:
return BindOutVariableDeclarationArgument(declarationExpression, diagnostics);
Expand Down Expand Up @@ -2402,11 +2461,11 @@ private void CoerceArguments<TMember>(
TypeSymbol parameterType = GetCorrespondingParameterType(ref result, parameters, arg);
arguments[arg] = ((OutDeconstructVarPendingInference)argument).SetInferredType(parameterType, success: true);
}
else if (argument.Kind == BoundKind.DiscardedExpression && !argument.HasExpressionType())
else if (argument.Kind == BoundKind.DiscardExpression && !argument.HasExpressionType())
{
TypeSymbol parameterType = GetCorrespondingParameterType(ref result, parameters, arg);
Debug.Assert((object)parameterType != null);
arguments[arg] = ((BoundDiscardedExpression)argument).SetInferredType(parameterType);
arguments[arg] = ((BoundDiscardExpression)argument).SetInferredType(parameterType);
}
}
}
Expand Down Expand Up @@ -6234,9 +6293,9 @@ private BoundExpression ConvertToArrayIndex(BoundExpression index, SyntaxNode no
{
return ((OutVariablePendingInference)index).FailInference(this, diagnostics);
}
else if (index.Kind == BoundKind.DiscardedExpression && !index.HasExpressionType())
else if (index.Kind == BoundKind.DiscardExpression && !index.HasExpressionType())
{
return ((BoundDiscardedExpression)index).FailInference(this, diagnostics);
return ((BoundDiscardExpression)index).FailInference(this, diagnostics);
}

var result =
Expand Down
16 changes: 8 additions & 8 deletions src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ internal BoundExpression MakeInvocationExpression(
var analyzedArguments = AnalyzedArguments.GetInstance();
Debug.Assert(!args.Any(e => e.Kind == BoundKind.OutVariablePendingInference ||
e.Kind == BoundKind.OutDeconstructVarPendingInference ||
e.Kind == BoundKind.DiscardedExpression && !e.HasExpressionType()));
e.Kind == BoundKind.DiscardExpression && !e.HasExpressionType()));
analyzedArguments.Arguments.AddRange(args);
BoundExpression result = BindInvocationExpression(
node, node, methodName, boundExpression, analyzedArguments, diagnostics, queryClause,
Expand Down Expand Up @@ -339,7 +339,7 @@ private ImmutableArray<BoundExpression> BuildArgumentsForDynamicInvocation(Analy
Debug.Assert(arguments.Arguments[i].Kind != BoundKind.OutDeconstructVarPendingInference);

if (arguments.Arguments[i].Kind == BoundKind.OutVariablePendingInference ||
arguments.Arguments[i].Kind == BoundKind.DiscardedExpression && !arguments.Arguments[i].HasExpressionType())
arguments.Arguments[i].Kind == BoundKind.DiscardExpression && !arguments.Arguments[i].HasExpressionType())
{
var builder = ArrayBuilder<BoundExpression>.GetInstance(arguments.Arguments.Count);
builder.AddRange(arguments.Arguments);
Expand All @@ -352,9 +352,9 @@ private ImmutableArray<BoundExpression> BuildArgumentsForDynamicInvocation(Analy
{
builder[i] = ((OutVariablePendingInference)argument).FailInference(this, diagnostics);
}
else if (argument.Kind == BoundKind.DiscardedExpression && !argument.HasExpressionType())
else if (argument.Kind == BoundKind.DiscardExpression && !argument.HasExpressionType())
{
builder[i] = ((BoundDiscardedExpression)argument).FailInference(this, diagnostics);
builder[i] = ((BoundDiscardExpression)argument).FailInference(this, diagnostics);
}

i++;
Expand Down Expand Up @@ -1192,7 +1192,7 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA
break;
}
case BoundKind.OutVariablePendingInference:
case BoundKind.DiscardedExpression:
case BoundKind.DiscardExpression:
{
if (argument.HasExpressionType())
{
Expand Down Expand Up @@ -1230,15 +1230,15 @@ private ImmutableArray<BoundExpression> BuildArgumentsForErrorRecovery(AnalyzedA
newArguments[i] = ((OutVariablePendingInference)argument).SetInferredType(candidateType, null);
}
}
else if (argument.Kind == BoundKind.DiscardedExpression)
else if (argument.Kind == BoundKind.DiscardExpression)
{
if ((object)candidateType == null)
{
newArguments[i] = ((BoundDiscardedExpression)argument).FailInference(this, null);
newArguments[i] = ((BoundDiscardExpression)argument).FailInference(this, null);
}
else
{
newArguments[i] = ((BoundDiscardedExpression)argument).SetInferredType(candidateType);
newArguments[i] = ((BoundDiscardExpression)argument).SetInferredType(candidateType);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ private BoundPattern BindDeclarationPattern(
{
case SyntaxKind.SingleVariableDesignation:
break;
case SyntaxKind.DiscardedDesignation:
case SyntaxKind.DiscardDesignation:
return new BoundDeclarationPattern(node, null, boundDeclType, isVar, hasErrors);
default:
throw ExceptionUtilities.UnexpectedValue(node.Designation.Kind());
Expand Down
10 changes: 5 additions & 5 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 @@ -591,7 +591,7 @@ private TypeSymbol BindVariableType(CSharpSyntaxNode declarationNode, Diagnostic
declarationNode is VariableDesignationSyntax ||
declarationNode.Kind() == SyntaxKind.VariableDeclaration ||
declarationNode.Kind() == SyntaxKind.DeclarationExpression ||
declarationNode.Kind() == SyntaxKind.DiscardedDesignation);
declarationNode.Kind() == SyntaxKind.DiscardDesignation);

// If the type is "var" then suppress errors when binding it. "var" might be a legal type
// or it might not; if it is not then we do not want to report an error. If it is, then
Expand Down Expand Up @@ -1713,15 +1713,15 @@ private BoundExpression BindAssignment(AssignmentExpressionSyntax node, Diagnost
var op1 = BindValue(node.Left, diagnostics, BindValueKind.Assignment); // , BIND_MEMBERSET);
var op2 = BindValue(node.Right, diagnostics, BindValueKind.RValue); // , BIND_RVALUEREQUIRED);

if (op1.Kind == BoundKind.DiscardedExpression)
if (op1.Kind == BoundKind.DiscardExpression)
{
op1 = InferTypeForDiscard((BoundDiscardedExpression)op1, op2, diagnostics);
op1 = InferTypeForDiscard((BoundDiscardExpression)op1, op2, diagnostics);
}

return BindAssignment(node, op1, op2, diagnostics);
}

private BoundExpression InferTypeForDiscard(BoundDiscardedExpression op1, BoundExpression op2, DiagnosticBag diagnostics)
private BoundExpression InferTypeForDiscard(BoundDiscardExpression op1, BoundExpression op2, DiagnosticBag diagnostics)
{
if (op2.Type == null)
{
Expand Down Expand Up @@ -1865,7 +1865,7 @@ private BoundExpression CheckValue(BoundExpression expr, BindValueKind valueKind
Debug.Assert(valueKind == BindValueKind.RefOrOut);
return expr;

case BoundKind.DiscardedExpression:
case BoundKind.DiscardExpression:
Debug.Assert(valueKind == BindValueKind.Assignment || valueKind == BindValueKind.RefOrOut);
return expr;
}
Expand Down
22 changes: 12 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 @@ -260,7 +260,7 @@ public override void VisitBinaryExpression(BinaryExpressionSyntax node)
public override void VisitDeclarationExpression(DeclarationExpressionSyntax node)
{
var argumentSyntax = (ArgumentSyntax)node?.Parent;
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

var argumentSyntax = (ArgumentSyntax)node?.Parent; [](start = 12, length = 50)

I don't think we ever expect the node to be null here and the code below doesn't expect argumentSyntax to be null, please remove the conditional access. #Closed

var argumentListSyntax = (BaseArgumentListSyntax)argumentSyntax?.Parent;
var argumentListSyntax = argumentSyntax.Parent as BaseArgumentListSyntax;
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

var argumentListSyntax = argumentSyntax.Parent as BaseArgumentListSyntax; [](start = 12, length = 73)

We should have tests that verify type inference scenarios for the local symbols that we create here when argumentListSyntax is null, including cases when the inference is triggered by SemanticModel. Could you please point me to the tests like this, or add some if none exist at the moment. #Closed

Copy link
Member Author

@gafter gafter Dec 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

There are five at the moment. See OutVarTests.OutVarDeconstruction_02, OutVarTests.OutVarDeconstruction_03, OutVarTests.OutVarDeconstruction_04, PatternSwitchTests.TupleInPattern, and LocalFunctionTests.NoOperator is also affected. #Resolved

var variable = MakeOutVariable(node, argumentListSyntax, _nodeToBind);
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

var variable = MakeOutVariable(node, argumentListSyntax, _nodeToBind); [](start = 12, length = 70)

The MakeOutVariable name is now misleading, please rename so that the name wouldn't make people believe that the function is used only for legitimate Out Vars. #Closed

Copy link
Member Author

@gafter gafter Dec 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. I'll rename _localsBuilder too. #Resolved

if ((object)variable != null)
{
Expand All @@ -270,7 +270,7 @@ public override void VisitDeclarationExpression(DeclarationExpressionSyntax node

public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
{
if (node.Left.IsKind(SyntaxKind.TupleExpression) || node.Left.IsKind(SyntaxKind.DeclarationExpression))
if (node.IsDeconstruction())
{
CollectVariablesFromDeconstruction(node.Left, node);
}
Expand All @@ -283,14 +283,14 @@ public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
}

private void CollectVariablesFromDeconstruction(
ExpressionSyntax declaration,
ExpressionSyntax possibleTupleDeclaration,
AssignmentExpressionSyntax deconstruction)
{
switch (declaration.Kind())
switch (possibleTupleDeclaration.Kind())
{
case SyntaxKind.TupleExpression:
{
var tuple = (TupleExpressionSyntax)declaration;
var tuple = (TupleExpressionSyntax)possibleTupleDeclaration;
foreach (var arg in tuple.Arguments)
{
CollectVariablesFromDeconstruction(arg.Expression, deconstruction);
Expand All @@ -299,13 +299,15 @@ private void CollectVariablesFromDeconstruction(
}
case SyntaxKind.DeclarationExpression:
{
var declarationExpression = (DeclarationExpressionSyntax)declaration;
var declarationExpression = (DeclarationExpressionSyntax)possibleTupleDeclaration;
CollectVariablesFromDeconstruction(declarationExpression.Designation, declarationExpression.Type, deconstruction);
break;
}
default:
Visit(declaration);
break;
{
Visit(possibleTupleDeclaration);
break;
}
}
}

Expand Down Expand Up @@ -335,7 +337,7 @@ private void CollectVariablesFromDeconstruction(
}
break;
}
case SyntaxKind.DiscardedDesignation:
case SyntaxKind.DiscardDesignation:
break;
default:
throw ExceptionUtilities.UnexpectedValue(designation.Kind());
Expand Down Expand Up @@ -404,7 +406,7 @@ protected override LocalSymbol MakePatternVariable(DeclarationPatternSyntax node
var designation = node.Designation as SingleVariableDesignationSyntax;
if (designation == null)
{
Debug.Assert(node.Designation.Kind() == SyntaxKind.DiscardedDesignation);
Debug.Assert(node.Designation.Kind() == SyntaxKind.DiscardDesignation);
return null;
}

Expand Down
Loading