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

Handling remaining PROTOTYPE markers in features/tuples branch #12411

Merged
merged 5 commits into from
Jul 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
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
8 changes: 4 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Deconstruct.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ private BoundExpression BindDeconstructionAssignment(AssignmentExpressionSyntax
var left = (TupleExpressionSyntax)node.Left;
ArrayBuilder<DeconstructionVariable> checkedVariables = BindDeconstructionAssignmentVariables(left.Arguments, left, diagnostics);

var result = BindDeconstructionAssignment(node, node.Right, checkedVariables, diagnostics);
var result = BindDeconstructionAssignment(node, node.Right, checkedVariables, diagnostics, isDeclaration: false);
FreeDeconstructionVariables(checkedVariables);

return result;
Expand All @@ -47,7 +47,7 @@ private static void FreeDeconstructionVariables(ArrayBuilder<DeconstructionVaria
/// Deconstruct steps for tuples have no invocation to Deconstruct, but steps for non-tuples do.
/// The caller is responsible for releasing all the ArrayBuilders in checkedVariables.
/// </summary>
private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CSharpSyntaxNode node, ExpressionSyntax right, ArrayBuilder<DeconstructionVariable> checkedVariables, DiagnosticBag diagnostics, BoundDeconstructValuePlaceholder rhsPlaceholder = null)
private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CSharpSyntaxNode node, ExpressionSyntax right, ArrayBuilder<DeconstructionVariable> checkedVariables, DiagnosticBag diagnostics, bool isDeclaration, BoundDeconstructValuePlaceholder rhsPlaceholder = null)
{
TypeSymbol voidType = GetSpecialType(SpecialType.System_Void, diagnostics, node);

Expand All @@ -56,7 +56,7 @@ private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CShar

if ((object)boundRHS.Type == null)
Copy link
Member

Choose a reason for hiding this comment

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

How do we distinguish between the case:

  • RHS is (null, null) and hence has no type
  • RHS is in error and hence has no type.

In the latter case I assume we wouldn't want to go down this code path.

Copy link
Member Author

Choose a reason for hiding this comment

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

When RHS is in error, its type is ErrorType (not null) which is already handled below (by DeconstructIntoStep). Added a test explicitly for this.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a good rule for when errors will cause Type to be ErrorType and when it will be null? Having both without a justification for when to choose one or the other make it harder to review code.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. I haven't heard any. So far, the cases I encountered where Type was null is in the expression null and also typeless tuples (() => {}, null).
I filed issue #12428 and will follow-up on that question separately.

{
if (boundRHS.Kind == BoundKind.TupleLiteral)
if (boundRHS.Kind == BoundKind.TupleLiteral && !isDeclaration)
{
// tuple literal without type such as `(null, null)`, let's fix it up by peeking at the LHS
TypeSymbol lhsAsTuple = MakeTupleTypeFromDeconstructionLHS(checkedVariables, diagnostics, Compilation);
Expand Down Expand Up @@ -496,7 +496,7 @@ internal BoundDeconstructionAssignmentOperator BindDeconstructionDeclaration(CSh
{
ArrayBuilder<DeconstructionVariable> locals = BindDeconstructionDeclarationLocals(declaration, declaration.Type, diagnostics);

var result = BindDeconstructionAssignment(node, right, locals, diagnostics, rightPlaceholder);
var result = BindDeconstructionAssignment(node, right, locals, diagnostics, isDeclaration: true, rhsPlaceholder: rightPlaceholder);
FreeDeconstructionVariables(locals);

return result;
Expand Down
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpCodeAnalysis.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,10 @@
<Compile Include="Syntax\DeclarationStatementSyntax.cs" />
<Compile Include="Syntax\DelegateDeclarationSyntax.cs" />
<Compile Include="Syntax\DirectiveTriviaSyntax.cs" />
<Compile Include="Syntax\EventFieldDeclarationSyntax.cs" />
<Compile Include="Syntax\ExpressionStatementSyntax.cs" />
<Compile Include="Syntax\FieldDeclarationSyntax.cs" />
<Compile Include="Syntax\FixedStatementSyntax.cs" />
<Compile Include="Syntax\ForEachStatementSyntax.cs" />
<Compile Include="Syntax\GenericNameSyntax.cs" />
<Compile Include="Syntax\InternalSyntax\BinaryExpressionSyntax.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ private ImmutableArray<BoundExpression> LeftHandSideSideEffects(ImmutableArray<B

foreach (var variable in variables)
{
// PROTOTYPE(tuples) should the dynamic flag always be false?
lhsReceivers.Add(TransformCompoundAssignmentLHS(variable, stores, temps, isDynamicAssignment: false));
}

Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8558,7 +8558,6 @@ private VariableDeclarationSyntax ParseDeconstructionTypeIdPart()
/// <summary>
/// Check ahead for a deconstruction declaration. This requires at least one good-looking variable and the presence of an equals sign.
/// Doesn't move the cursor.
/// PROTOTYPE(tuples) Can this be done without allocations?
/// </summary>
private bool IsPossibleDeconstructionDeclaration()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
public sealed partial class EventFieldDeclarationSyntax : BaseFieldDeclarationSyntax
{
public EventFieldDeclarationSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}
}
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/Syntax/FieldDeclarationSyntax.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
public sealed partial class FieldDeclarationSyntax : BaseFieldDeclarationSyntax
{
public FieldDeclarationSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}
}
11 changes: 11 additions & 0 deletions src/Compilers/CSharp/Portable/Syntax/FixedStatementSyntax.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
public sealed partial class FixedStatementSyntax : StatementSyntax
{
public FixedStatementSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
Expand All @@ -12,6 +10,11 @@ public LocalDeclarationStatementSyntax Update(SyntaxTokenList modifiers, Variabl
{
return Update(modifiers, this.RefKeyword, declaration, semicolonToken);
}

public LocalDeclarationStatementSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}
}

Expand Down
36 changes: 0 additions & 36 deletions src/Compilers/CSharp/Portable/Syntax/SyntaxFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2650,39 +2650,3 @@ internal partial class ContextAwareSyntax
#endif
}
}

// PROTOTYPE(tuples) Move this to a better place
namespace Microsoft.CodeAnalysis.CSharp.Syntax
{
public sealed partial class EventFieldDeclarationSyntax : BaseFieldDeclarationSyntax
{
public EventFieldDeclarationSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}

public sealed partial class FieldDeclarationSyntax : BaseFieldDeclarationSyntax
{
public FieldDeclarationSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}

public sealed partial class FixedStatementSyntax : StatementSyntax
{
public FixedStatementSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}

public sealed partial class LocalDeclarationStatementSyntax : StatementSyntax
{
public LocalDeclarationStatementSyntax AddDeclarationVariables(params VariableDeclaratorSyntax[] items)
{
return this.WithDeclaration(this.Declaration.WithVariables(this.Declaration.Variables.AddRange(items)));
}
}
}
45 changes: 37 additions & 8 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDeconstructTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1284,6 +1284,28 @@ static void Main()
);
}

[Fact]
public void ErrorRight()
{
string source = @"
class C
{
static void Main()
{
int x;
(x, x) = undeclared;
}
}
";

var comp = CreateCompilationWithMscorlib(source, parseOptions: TestOptions.Regular.WithRefsFeature());
comp.VerifyDiagnostics(
// (7,18): error CS0103: The name 'undeclared' does not exist in the current context
// (x, x) = undeclared;
Diagnostic(ErrorCode.ERR_NameNotInContext, "undeclared").WithArguments("undeclared").WithLocation(7, 18)
);
}

[Fact]
public void VoidRight()
{
Expand Down Expand Up @@ -2484,7 +2506,8 @@ public void Deconstruct(out int a, out string b)
comp.VerifyDiagnostics();
}

[Fact(Skip = "PROTOTYPE(tuples)")]
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/12400")]
[WorkItem(12400, "https://github.com/dotnet/roslyn/issues/12400")]
public void AssignWithPostfixOperator()
{
string source = @"
Expand Down Expand Up @@ -2513,7 +2536,8 @@ public void Deconstruct(out int a, out string b)
}
}
";
// PROTOTYPE(tuples) we expect "2 hello" instead, which means the evaluation order is wrong
// https://github.com/dotnet/roslyn/issues/12400
// we expect "2 hello" instead, which means the evaluation order is wrong
var comp = CompileAndVerify(source, expectedOutput: "1 hello");
comp.VerifyDiagnostics();
}
Expand Down Expand Up @@ -2936,7 +2960,7 @@ static void Main()
);
}

[Fact(Skip = "PROTOTYPE(tuples)")]
[Fact]
public void TypelessDeclaration()
{
string source = @"
Expand All @@ -2948,13 +2972,15 @@ static void Main()
}
}
";
// crash
var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
comp.VerifyDiagnostics(
// (6,24): error CS8210: Deconstruct assignment requires an expression with a type on the right-hand-side.
// var (x1, x2) = (1, null);
Diagnostic(ErrorCode.ERR_DeconstructRequiresExpression, "(1, null)").WithLocation(6, 24)
);
}

[Fact(Skip = "PROTOTYPE(tuples)")]
[Fact]
public void InferTypeOfTypelessDeclaration()
{
string source = @"
Expand All @@ -2967,9 +2993,12 @@ static void Main()
}
}
";
// crash
var comp = CompileAndVerify(source, expectedOutput: "1 2 ");
comp.VerifyDiagnostics();
var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
comp.VerifyDiagnostics(
// (6,37): error CS8210: Deconstruct assignment requires an expression with a type on the right-hand-side.
// (var (x1, x2), string x3) = ((1, 2), null);
Diagnostic(ErrorCode.ERR_DeconstructRequiresExpression, "((1, 2), null)").WithLocation(6, 37)
);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.Test.Utilities;
using Xunit;
using Roslyn.Test.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Parsing
{
Expand Down Expand Up @@ -1728,7 +1729,8 @@ public void InvalidStatement()
Assert.True(statement.HasErrors);
}

[Fact(Skip = "PROTOTYPE(tuples)")]
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/12402")]
[WorkItem(12402, "https://github.com/dotnet/roslyn/issues/12402")]
public void ConfusedForWithDeconstruction()
{
var text = "for ((int x, var (y, z)) in foo) { }";
Expand Down