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

D-declaration should use declaration expressions #14871

Merged
merged 8 commits into from
Nov 8, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,10 @@ private void CheckOutVarDeclaration(BoundLocal node)
!node.WasCompilerGenerated && node.Syntax.Kind() == SyntaxKind.DeclarationExpression)
{
var declaration = (DeclarationExpressionSyntax)node.Syntax;
if (((SingleVariableDesignationSyntax)declaration.Designation).Identifier == node.LocalSymbol.IdentifierToken &&
if (declaration.Designation.Kind() == SyntaxKind.SingleVariableDesignation &&
((SingleVariableDesignationSyntax)declaration.Designation).Identifier == node.LocalSymbol.IdentifierToken &&
declaration.Parent != null &&
declaration.Parent.Kind() == SyntaxKind.Argument &&
((ArgumentSyntax)declaration.Parent).RefOrOutKeyword.Kind() == SyntaxKind.OutKeyword)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 8, 2016

Choose a reason for hiding this comment

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

declaration.Parent [](start = 37, length = 18)

Probably should ensure that the cast is safe (check the kind) and that the Parent is not null. #Closed

{
_variablesDeclared.Add(node.LocalSymbol);
Expand Down
25 changes: 23 additions & 2 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax
{
using Microsoft.CodeAnalysis.Syntax.InternalSyntax;
using System.Linq;

internal partial class LanguageParser : SyntaxParser
{
Expand Down Expand Up @@ -7542,7 +7541,7 @@ private StatementSyntax ParseEmbeddedStatement(bool complexCheck)
if (expression.Kind == SyntaxKind.SimpleAssignmentExpression)
{
var assignment = (AssignmentExpressionSyntax)expression;
if (assignment.Left.EnumerateNodes().Any(x => x.RawKind == (int)SyntaxKind.DeclarationExpression))
if (IsDeconstructionDeclarationLeft(assignment.Left))
{
statement = this.AddError(statement, ErrorCode.ERR_BadEmbeddedStmt);
}
Expand Down Expand Up @@ -8668,6 +8667,28 @@ private static bool TypeFoundInDeconstructionDeclarationVariables(ExpressionSynt
}
}

/// <summary>
/// Returns true if the expression is composed only of nested tuple and declaration expressions.
/// </summary>
private static bool IsDeconstructionDeclarationLeft(ExpressionSyntax node)
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 8, 2016

Choose a reason for hiding this comment

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

IsDeconstructionDeclarationLeft [](start = 28, length = 31)

Is this a dupe of a method from SyntaxExtensions? Consider reusing it instead. #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.

The logic is the same, but one is on internal nodes, the other on public nodes.

{
switch (node.Kind)
{
case SyntaxKind.TupleExpression:
var arguments = ((TupleExpressionSyntax)node).Arguments;
for (int i = 0; i < arguments.Count; i++)
{
if (!IsDeconstructionDeclarationLeft(arguments[i].Expression)) return false;
}

return true;
case SyntaxKind.DeclarationExpression:
return true;
default:
return false;
}
}

private VariableDesignationSyntax ParseDeconstructionDesignation(bool topLevel = false)
{
// the two forms of designation are
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,6 @@ static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ConstructorDeclaration(Micros
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ConstructorDeclaration(Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax> attributeLists, Microsoft.CodeAnalysis.SyntaxTokenList modifiers, Microsoft.CodeAnalysis.SyntaxToken identifier, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.Syntax.ConstructorInitializerSyntax initializer, Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax body, Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax expressionBody) -> Microsoft.CodeAnalysis.CSharp.Syntax.ConstructorDeclarationSyntax
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ConstructorDeclaration(Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax> attributeLists, Microsoft.CodeAnalysis.SyntaxTokenList modifiers, Microsoft.CodeAnalysis.SyntaxToken identifier, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.Syntax.ConstructorInitializerSyntax initializer, Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax body, Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax expressionBody, Microsoft.CodeAnalysis.SyntaxToken semicolonToken) -> Microsoft.CodeAnalysis.CSharp.Syntax.ConstructorDeclarationSyntax
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.DeclarationExpression(Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax type, Microsoft.CodeAnalysis.CSharp.Syntax.VariableDesignationSyntax designation) -> Microsoft.CodeAnalysis.CSharp.Syntax.DeclarationExpressionSyntax
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.DeclarationExpression(Microsoft.CodeAnalysis.CSharp.Syntax.VariableDesignationSyntax designation) -> Microsoft.CodeAnalysis.CSharp.Syntax.DeclarationExpressionSyntax
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.DeclarationPattern(Microsoft.CodeAnalysis.CSharp.Syntax.TypeSyntax type, Microsoft.CodeAnalysis.SyntaxToken identifier) -> Microsoft.CodeAnalysis.CSharp.Syntax.DeclarationPatternSyntax
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.DestructorDeclaration(Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax> attributeLists, Microsoft.CodeAnalysis.SyntaxTokenList modifiers, Microsoft.CodeAnalysis.SyntaxToken identifier, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax expressionBody) -> Microsoft.CodeAnalysis.CSharp.Syntax.DestructorDeclarationSyntax
static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.DestructorDeclaration(Microsoft.CodeAnalysis.SyntaxList<Microsoft.CodeAnalysis.CSharp.Syntax.AttributeListSyntax> attributeLists, Microsoft.CodeAnalysis.SyntaxTokenList modifiers, Microsoft.CodeAnalysis.SyntaxToken identifier, Microsoft.CodeAnalysis.CSharp.Syntax.ParameterListSyntax parameterList, Microsoft.CodeAnalysis.CSharp.Syntax.BlockSyntax body, Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax expressionBody) -> Microsoft.CodeAnalysis.CSharp.Syntax.DestructorDeclarationSyntax
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Syntax/Syntax.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1131,7 +1131,7 @@
</Node>
<Node Name="DeclarationExpressionSyntax" Base="ExpressionSyntax">
<Kind Name="DeclarationExpression"/>
<Field Name="Type" Type="TypeSyntax" Optional="true"/>
<Field Name="Type" Type="TypeSyntax"/>
<Field Name="Designation" Type="VariableDesignationSyntax">
<PropertyComment>
<summary>Declaration representing the variable declared in an out parameter or deconstruction.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2454,6 +2454,24 @@ void M()
);
}

[Fact]
public void AssignmentExpressionCanBeUsedInEmbeddedStatement()
{
var source = @"
class C1
{
void M()
{
int x, y;
if (true)
(x, y) = (1, 2);
}
}
";
var comp = CreateCompilationWithMscorlib(source, references: s_valueTupleRefs);
comp.VerifyDiagnostics();
}

[Fact]
public void DeconstructObsoleteWarning()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ private void AddVariableExpressions(
{
var t = (TupleExpressionSyntax)component;
foreach (ArgumentSyntax a in t.Arguments) AddVariableExpressions(a.Expression, expressions);
Copy link
Member

Choose a reason for hiding this comment

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

always use braces.

// TODO REVIEW I think there was a bug there. Are we missing tests?
break;
Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed with Neal there was a bug.
Filed bug #15095 to improve test coverage

}
case SyntaxKind.DeclarationExpression:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2161,14 +2161,6 @@ private static bool IsReplaceableByVar(
return true;
}

//if (simpleName.IsParentKind(SyntaxKind.TypedVariableComponent))
//{
// replacementNode = candidateReplacementNode;
// issueSpan = candidateIssueSpan;
// return true;
//}
// TODO REVIEW Are we lacking tests? It seems this code should be needed for DeclarationExpression

return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

Filed follow-up issue: #15094

}

Expand Down