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

Fixing some semantic model and refactoring issues with deconstruction #12381

Merged
merged 6 commits into from
Jul 11, 2016
Merged

Fixing some semantic model and refactoring issues with deconstruction #12381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
432135a
ExtractMethod refactoring should not swallow neighboring deconstruction
jcouv Jul 6, 2016
0860351
Fixing GetTypeSymbol
jcouv Jul 7, 2016
fd3eaa2
Adding test for parens issue
jcouv Jul 7, 2016
8a9fa3a
Simplifying parens in d-declaration
jcouv Jul 7, 2016
6ef0517
Making SkipInVisitor also work for C# bound nodes
jcouv Jul 7, 2016
dbddd6a
PR feedback (1)
jcouv Jul 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
4 changes: 2 additions & 2 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 @@ -69,7 +69,7 @@ private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CShar
FailRemainingInferences(checkedVariables, diagnostics);

return new BoundDeconstructionAssignmentOperator(
node, FlattenDeconstructVariables(checkedVariables), boundRHS,
node, isDeclaration, FlattenDeconstructVariables(checkedVariables), boundRHS,
ImmutableArray<BoundDeconstructionDeconstructStep>.Empty, ImmutableArray<BoundDeconstructionAssignmentStep>.Empty,
voidType, hasErrors: true);
}
Expand All @@ -83,7 +83,7 @@ private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CShar
var assignments = assignmentSteps.ToImmutableAndFree();

FailRemainingInferences(checkedVariables, diagnostics);
return new BoundDeconstructionAssignmentOperator(node, FlattenDeconstructVariables(checkedVariables), boundRHS, deconstructions, assignments, voidType, hasErrors: hasErrors);
return new BoundDeconstructionAssignmentOperator(node, isDeclaration, FlattenDeconstructVariables(checkedVariables), boundRHS, deconstructions, assignments, voidType, hasErrors: hasErrors);
}

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,8 @@
<!-- Non-null type is required for this node kind -->
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>

<Field Name="IsDeclaration" Type="bool"/>
Copy link
Member

Choose a reason for hiding this comment

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

What does this field represent? Don't we have a separate BoundLocalDeconstructionDeclaration node to represent declarations?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do have a separate node for d-declaration which is a statement. But all it contains is a BoundDeconstructionAssignmentOperator.
The problem is that in the case of an assignment that is not a declaration, the expressions on the LHS should make it into the node map. But in the case of a declaration, they should not.

For comparison, if you have x = 1;, you will have 3 nodes in the node map: one for the whole assignment expression, one for the value 1 and one for the local x.
But if you have var x = 1, then you will have a different set of 3 nodes: one for the whole statement, one for the value 1 and one for the type var. But no x.


<!-- Various assignable expressions, in a flat structure (no nesting) -->
<Field Name="LeftVariables" Type="ImmutableArray&lt;BoundExpression&gt;"/>

Expand Down
13 changes: 13 additions & 0 deletions src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.NodeMapBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,19 @@ public override BoundNode VisitBinaryOperator(BoundBinaryOperator node)
throw ExceptionUtilities.Unreachable;
}

public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstructionAssignmentOperator node)
Copy link
Member

Choose a reason for hiding this comment

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

VisitDeconstructionAssignmentOperator [](start = 38, length = 37)

What is the reason for this? I'm not as familiar with the node map so it's unclear why we have this behavior.

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 node map allows mapping from syntax back to bound node.
From discussion with Neal, what we want in the map in the case of a deconstruction declaration is: a node for the whole assignment expression, nodes for the bound types that appear on the LHS, and a node for the RHS value. Note that no nodes are added to the map for the locals being declared.
This is similar to what happens for a local declaration (var x = 1;), which also does not put anything for x in the map.

But the bound nodes for deconstruction hold more information than is typical, such as deconstruction steps, conversion and assignment steps. Those don't map directly to syntax though, and so should not appear in the map for the semantic model to understand the syntax.

{
// For deconstruction declarations, the BoundLocals in the LeftVariables should not be added to the map
if (!node.IsDeclaration)
{
VisitList(node.LeftVariables);
}

Visit(node.Right);
// don't map the deconstruction, conversion or assignment steps
return null;
}
Copy link
Member

Choose a reason for hiding this comment

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

} [](start = 12, length = 1)

It seems like all of our testing for the node map is indirect. Instead of actually querying to see if the value is in the map, we use an IDE feature that happens to call into the node map. Is it possible to test this directly?

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 most direct test is via GetTypeInfo. Without the above changes to avoid the visitor putting too many nodes in the map, one cannot get the type info for (1, 2) in var (x, y) = (1, 2);.


protected override bool ConvertInsufficientExecutionStackExceptionToCancelledByStackGuardException()
{
return false;
Expand Down
34 changes: 33 additions & 1 deletion src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3685,6 +3685,38 @@ static void Main()
);
}

[Fact]
public void GetTypeInfoForTupleLiteral()
{
var source = @"
class C
{
static void Main()
{
var x1 = (1, 2);
var (x2, x3) = (1, 2);
}
}
";
Action<ModuleSymbol> validator = module =>
{
var sourceModule = (SourceModuleSymbol)module;
var compilation = sourceModule.DeclaringCompilation;
var tree = compilation.SyntaxTrees.First();
var model = compilation.GetSemanticModel(tree);
var nodes = tree.GetCompilationUnitRoot().DescendantNodes();

var literal1 = nodes.OfType<TupleExpressionSyntax>().First();
Assert.Equal("(int, int)", model.GetTypeInfo(literal1).Type.ToDisplayString());

var literal2 = nodes.OfType<TupleExpressionSyntax>().Skip(1).First();
Assert.Equal("(int, int)", model.GetTypeInfo(literal2).Type.ToDisplayString());
};

var verifier = CompileAndVerify(source, additionalRefs: new[] { ValueTupleRef, SystemRuntimeFacadeRef }, sourceSymbolValidator: validator);
verifier.VerifyDiagnostics();
}

[Fact]
public void ForWithCircularity1()
{
Expand Down Expand Up @@ -4635,4 +4667,4 @@ static void Main()
comp.VerifyDiagnostics();
}
}
}
}
22 changes: 22 additions & 0 deletions src/EditorFeatures/CSharpTest/CodeActions/ExtractMethod/ExtractMethodTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,28 @@ public async Task TestTupleWithNestedNamedTuple()
@"class Program { static void Main ( string [ ] args ) { (int, int, int, int, int, int, int, string, string) x = {|Rename:NewMethod|}(); System . Console . WriteLine ( x.c ); } private static (int, int, int, int, int, int, int, string, string) NewMethod() { return new System.ValueTuple<int, int, int, int, int, int, int, (string a, string b)>(1, 2, 3, 4, 5, 6, 7, (a: ""hello"", b: ""world"")); } }" + TestResources.NetFX.ValueTuple.tuplelib_cs,
index: 0,
parseOptions: TestOptions.Regular,
withScriptOption: true);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod), Test.Utilities.CompilerTrait(Test.Utilities.CompilerFeature.Tuples)]
public async Task TestDeconstruction()
{
await TestAsync(
@"class Program { static void Main ( string [ ] args ) { var (x, y) = [| (1, 2) |]; System . Console . WriteLine ( x ); } } " + TestResources.NetFX.ValueTuple.tuplelib_cs,
@"class Program { static void Main ( string [ ] args ) { var (x, y) = {|Rename:NewMethod|}(); System . Console . WriteLine ( x ); } private static (int, int) NewMethod() { return (1, 2); } }" + TestResources.NetFX.ValueTuple.tuplelib_cs,
index: 0,
parseOptions: TestOptions.Regular,
withScriptOption: true);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsExtractMethod), Test.Utilities.CompilerTrait(Test.Utilities.CompilerFeature.Tuples)]
public async Task TestDeconstruction2()
{
await TestAsync(
@"class Program { static void Main ( string [ ] args ) { var (x, y) = (1, 2); var z = [| 3; |] System . Console . WriteLine ( z ); } } " + TestResources.NetFX.ValueTuple.tuplelib_cs,
@"class Program { static void Main ( string [ ] args ) { var (x, y) = (1, 2); int z = {|Rename:NewMethod|}(); System . Console . WriteLine ( z ); } private static int NewMethod() { return 3; } }" + TestResources.NetFX.ValueTuple.tuplelib_cs,
index: 0,
parseOptions: TestOptions.Regular,
withScriptOption: true);
}
}
Expand Down
35 changes: 32 additions & 3 deletions src/EditorFeatures/CSharpTest/CodeActions/InlineTemporary/InlineTemporaryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3988,7 +3988,7 @@ public void M()
}
}";

await TestAsync(code, expected, index: 0, compareTokens: false, parseOptions: TestOptions.Regular, withScriptOption: true);
await TestAsync(code, expected, index: 0, compareTokens: false);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)]
Expand All @@ -4015,7 +4015,7 @@ public void M()
}
}";

await TestAsync(code, expected, index: 0, compareTokens: false, parseOptions: TestOptions.Regular, withScriptOption: true);
await TestAsync(code, expected, index: 0, compareTokens: false);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)]
Expand All @@ -4041,7 +4041,36 @@ public void M()
}
}";

await TestAsync(code, expected, index: 0, compareTokens: false, parseOptions: TestOptions.Regular, withScriptOption: true);
await TestAsync(code, expected, index: 0, compareTokens: false);
}

[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInlineTemporary)]
public async Task Deconstruction()
{
var code = @"
using System;
class C
{
public void M()
{
var [||]temp = new C();
var (x1, x2) = temp;
var x3 = temp;
}
}";

var expected = @"
using System;
class C
{
public void M()
{
var (x1, x2) = new C();
var x3 = new C();
}
}";

await TestAsync(code, expected, index: 0);
}
}
}
6 changes: 5 additions & 1 deletion ...able/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.CallSiteContainerRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,13 @@ public override SyntaxNode VisitLocalDeclarationStatement(LocalDeclarationStatem
{
node = (LocalDeclarationStatementSyntax)base.VisitLocalDeclarationStatement(node);

if (node.Declaration.IsDeconstructionDeclaration)
{
return node;
}

var list = new List<VariableDeclaratorSyntax>();
var triviaList = new List<SyntaxTrivia>();

// go through each var decls in decl statement
foreach (var variable in node.Declaration.Variables)
{
Expand Down
4 changes: 2 additions & 2 deletions src/Features/CSharp/Portable/ExtractMethod/CSharpMethodExtractor.CSharpCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,9 @@ private OperationStatus CheckActiveStatements(IEnumerable<StatementSyntax> state
foreach (var statement in statements)
{
var declarationStatement = statement as LocalDeclarationStatementSyntax;
if (declarationStatement == null)
if (declarationStatement == null || declarationStatement.Declaration.IsDeconstructionDeclaration)
{
// if given statement is not decl statement, do nothing.
// if given statement is not decl statement, or if it is a deconstruction-declaration, do nothing.
yield return statement;
continue;
}
Expand Down
13 changes: 11 additions & 2 deletions src/Tools/Source/CompilerGeneratorTools/Source/BoundTreeGenerator/BoundNodeClassWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,8 +1165,10 @@ private void WriteWalker()
{
WriteLine("public override BoundNode Visit{0}({1} node)", StripBound(node.Name), node.Name);
Brace();
foreach (Field field in AllFields(node).Where(f => IsDerivedOrListOfDerived("BoundNode", f.Type)))
foreach (Field field in AllFields(node).Where(f => IsDerivedOrListOfDerived("BoundNode", f.Type) && !SkipInVisitor(f)))
{
WriteLine("this.Visit{1}(node.{0});", field.Name, IsNodeList(field.Type) ? "List" : "");
}
WriteLine("return null;");
Unbrace();
}
Expand Down Expand Up @@ -1344,7 +1346,14 @@ private void WriteRewriter()
foreach (Field field in AllNodeOrNodeListFields(node))
{
hadField = true;
WriteLine("{3} {0} = ({3})this.Visit{2}(node.{1});", ToCamelCase(field.Name), field.Name, IsNodeList(field.Type) ? "List" : "", field.Type);
if (SkipInVisitor(field))
{
WriteLine("{2} {0} = node.{1};", ToCamelCase(field.Name), field.Name, field.Type);
}
else
{
WriteLine("{3} {0} = ({3})this.Visit{2}(node.{1});", ToCamelCase(field.Name), field.Name, IsNodeList(field.Type) ? "List" : "", field.Type);
}
}
foreach (Field field in AllTypeFields(node))
{
Expand Down
5 changes: 5 additions & 0 deletions src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,11 @@ private static bool CanReplace(ISymbol symbol)
return true;
}

if (expression is TupleExpressionSyntax)
{
return true;
}

if (!(expression is ObjectCreationExpressionSyntax) && !(expression is AnonymousObjectCreationExpressionSyntax))
{
var symbolInfo = semanticModel.GetSymbolInfo(expression, cancellationToken);
Expand Down
2 changes: 2 additions & 0 deletions src/Workspaces/CSharp/Portable/Extensions/ParenthesizedExpressionSyntaxExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public static bool CanRemoveParentheses(this ParenthesizedExpressionSyntax node)

// Easy statement-level cases:
// var y = (x); -> var y = x;
// var (y, z) = (x); -> var (y, z) = x;
// if ((x)) -> if (x)
// return (x); -> return x;
// yield return (x); -> yield return x;
Expand All @@ -49,6 +50,7 @@ public static bool CanRemoveParentheses(this ParenthesizedExpressionSyntax node)
// using ((x)) -> using (x)
// catch when ((x)) -> catch when (x)
if ((node.IsParentKind(SyntaxKind.EqualsValueClause) && ((EqualsValueClauseSyntax)node.Parent).Value == node) ||
(node.IsParentKind(SyntaxKind.VariableDeconstructionDeclarator) && ((VariableDeconstructionDeclaratorSyntax)node.Parent).Value == node) ||
(node.IsParentKind(SyntaxKind.IfStatement) && ((IfStatementSyntax)node.Parent).Condition == node) ||
(node.IsParentKind(SyntaxKind.ReturnStatement) && ((ReturnStatementSyntax)node.Parent).Expression == node) ||
(node.IsParentKind(SyntaxKind.YieldReturnStatement) && ((YieldStatementSyntax)node.Parent).Expression == node) ||
Expand Down