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

Implement type merging in deconstruction with tuple literal #12526

Merged
merged 5 commits into from
Jul 20, 2016
Merged

Implement type merging in deconstruction with tuple literal #12526

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
69bf0a9
Implement type merging in deconstruction with tuple literal
jcouv Jul 14, 2016
1317d0f
Report more than one type merging error
jcouv Jul 14, 2016
6eb1d77
PR feedback (1)
jcouv Jul 17, 2016
6c33e54
PR feedback (2)
jcouv Jul 19, 2016
b5d8bde
PR feedback (3)
jcouv Jul 19, 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
92 changes: 69 additions & 23 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 @@ -56,16 +56,25 @@ private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CShar

if ((object)boundRHS.Type == null)
{
if (boundRHS.Kind == BoundKind.TupleLiteral && !isDeclaration)
if (boundRHS.Kind == BoundKind.TupleLiteral)
{
// tuple literal without type such as `(null, null)`, let's fix it up by peeking at the LHS
TypeSymbol lhsAsTuple = MakeTupleTypeFromDeconstructionLHS(checkedVariables, diagnostics, Compilation);
boundRHS = GenerateConversionForAssignment(lhsAsTuple, boundRHS, diagnostics);
// Let's fix the literal up by figuring out its type
// For declarations, that means merging type information from the LHS and RHS
// For assignments, only the LHS side matters since it is necessarily typed
TypeSymbol lhsAsTuple = MakeMergedTupleType(checkedVariables, (BoundTupleLiteral)boundRHS, node, diagnostics, Compilation);
if (lhsAsTuple != null)
Copy link
Member

Choose a reason for hiding this comment

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

(object)lhsAsTuple != null

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'm still not quite sure when that is useful. Is it for certain types only (such as Symbols)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll queue that and your other suggestion in next PR.

Copy link
Member

Choose a reason for hiding this comment

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

Only for types that define operator==.

{
boundRHS = GenerateConversionForAssignment(lhsAsTuple, boundRHS, diagnostics);
}
}
else
{
// expression without type such as `null`
Error(diagnostics, ErrorCode.ERR_DeconstructRequiresExpression, right);
}

if ((object)boundRHS.Type == null)
{
// we could still not infer a type for the RHS
FailRemainingInferences(checkedVariables, diagnostics);

return new BoundDeconstructionAssignmentOperator(
Expand All @@ -74,7 +83,6 @@ private BoundDeconstructionAssignmentOperator BindDeconstructionAssignment(CShar
voidType, hasErrors: true);
}
}

var deconstructionSteps = ArrayBuilder<BoundDeconstructionDeconstructStep>.GetInstance(1);
var assignmentSteps = ArrayBuilder<BoundDeconstructionAssignmentStep>.GetInstance(1);
bool hasErrors = !DeconstructIntoSteps(new BoundDeconstructValuePlaceholder(boundRHS.Syntax, boundRHS.Type), node, diagnostics, checkedVariables, deconstructionSteps, assignmentSteps);
Expand Down Expand Up @@ -188,7 +196,7 @@ private static void SetInferredTypes(ArrayBuilder<DeconstructionVariable> variab
if (!variable.HasNestedVariables && variable.Single.Kind == BoundKind.DeconstructionLocalPendingInference)
{
BoundLocal local = ((DeconstructionLocalPendingInference)variable.Single).SetInferredType(foundTypes[i], success: true);
variables[i] = new DeconstructionVariable(local);
variables[i] = new DeconstructionVariable(local, local.Syntax);
}
}
}
Expand All @@ -211,7 +219,7 @@ private void FailRemainingInferences(ArrayBuilder<DeconstructionVariable> variab
if (variable.Single.Kind == BoundKind.DeconstructionLocalPendingInference)
{
var local = ((DeconstructionLocalPendingInference)variable.Single).FailInference(this);
variables[i] = new DeconstructionVariable(local);
variables[i] = new DeconstructionVariable(local, local.Syntax);
}
}
}
Expand All @@ -226,11 +234,11 @@ private class DeconstructionVariable
public readonly ArrayBuilder<DeconstructionVariable> NestedVariables;
public readonly CSharpSyntaxNode Syntax;

public DeconstructionVariable(BoundExpression variable)
public DeconstructionVariable(BoundExpression variable, CSharpSyntaxNode syntax)
{
Single = variable;
NestedVariables = null;
Syntax = variable.Syntax;
Syntax = syntax;
}

public DeconstructionVariable(ArrayBuilder<DeconstructionVariable> variables, CSharpSyntaxNode syntax)
Expand Down Expand Up @@ -282,21 +290,59 @@ private bool DeconstructOrAssignOutputs(
}

/// <summary>
/// For cases where the RHS of a deconstruction-assignment has no type (TupleLiteral), we squint and look at the LHS as a tuple type to give the RHS a type.
/// For cases where the RHS of a deconstruction-declaration is a tuple literal, we merge type information from both the LHS and RHS.
/// For cases where the RHS of a deconstruction-assignment is a tuple literal, the type information from the LHS determines the merged type, since all variables have a type.
Copy link
Member

Choose a reason for hiding this comment

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

Where in the method do we check for declaration vs. assignment?

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 never have to check explicitly. The LHS in an assignment is always completely typed (no vars).

/// Returns null if a merged tuple type could not be fabricated.
/// </summary>
private static TypeSymbol MakeTupleTypeFromDeconstructionLHS(ArrayBuilder<DeconstructionVariable> topLevelCheckedVariables, DiagnosticBag diagnostics, CSharpCompilation compilation)
private static TypeSymbol MakeMergedTupleType(ArrayBuilder<DeconstructionVariable> lhsVariables, BoundTupleLiteral rhsLiteral, CSharpSyntaxNode syntax, DiagnosticBag diagnostics, CSharpCompilation compilation)
Copy link
Member

Choose a reason for hiding this comment

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

Minor point: diagnostics is typically the last non-ref/out parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

{
var typesBuilder = ArrayBuilder<TypeSymbol>.GetInstance(topLevelCheckedVariables.Count);
foreach (var variable in topLevelCheckedVariables)
int leftLength = lhsVariables.Count;
int rightLength = rhsLiteral.Arguments.Length;

var typesBuilder = ArrayBuilder<TypeSymbol>.GetInstance(lhsVariables.Count);
Copy link
Member

Choose a reason for hiding this comment

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

Could use leftLength here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in next PR (#12527)

for (int i = 0; i < rightLength; i++)
{
if (variable.HasNestedVariables)
{
typesBuilder.Add(MakeTupleTypeFromDeconstructionLHS(variable.NestedVariables, diagnostics, compilation));
}
else
BoundExpression element = rhsLiteral.Arguments[i];
TypeSymbol mergedType = element.Type;

if (i < leftLength)
Copy link
Member

Choose a reason for hiding this comment

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

What is the scenario where the right length is greater than left length? And are we reporting an error if there are null types in those elements?

Copy link
Member Author

Choose a reason for hiding this comment

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

Below is the test for that scenario. The merged type is (string, string, int), so we have an error reported for cardinality mismatch in conversion.
I also tried with the RHS being (null, "hello", null) and the error is the same. I'll add that test.

        [Fact]
        public void TypeMergingWithTooManyRightElements()
        {
            string source = @"
class C
{
    static void Main()
    {
        (string x1, var x2) = (null, ""hello"", 3);
    }
}
";
            var comp = CreateCompilationWithMscorlib(source, references: new[] { ValueTupleRef, SystemRuntimeFacadeRef });
            comp.VerifyDiagnostics(
                // (6,9): error CS8211: Cannot deconstruct a tuple of '3' elements into '2' variables.
                //         (string x1, var x2) = (null, "hello", 3);
                Diagnostic(ErrorCode.ERR_DeconstructWrongCardinality, @"(string x1, var x2) = (null, ""hello"", 3);").WithArguments("3", "2").WithLocation(6, 9)
                );
        }

Copy link
Member Author

@jcouv jcouv Jul 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I misspoke. Actually, with (null, "hello", null), no error is reported. I'll fix.
Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

{
typesBuilder.Add(variable.Single.Type);
var variable = lhsVariables[i];
if (variable.HasNestedVariables)
{
if (element.Kind == BoundKind.TupleLiteral)
{
// (variables) on the left and (elements) on the right
mergedType = MakeMergedTupleType(variable.NestedVariables, (BoundTupleLiteral)element, syntax, diagnostics, compilation);
}
else if (element.Type == null)
{
// (variables) on the left and null on the right
Error(diagnostics, ErrorCode.ERR_DeconstructRequiresExpression, element.Syntax);
}
}
else
{
if (variable.Single.Type != null)
{
// typed-variable on the left
mergedType = variable.Single.Type;
}
else if (element.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.

mergedType here and in earlier else if? Also, add (object) cast to all == checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I had a rationale earlier, but forgot what it was... Fixed :-)

{
// typeless-variable on the left and typeless-element on the right
Error(diagnostics, ErrorCode.ERR_DeconstructCouldNotInferMergedType, syntax, variable.Syntax, element.Syntax);
}
}
}

typesBuilder.Add(mergedType);
}

if (typesBuilder.Any(t => t == null))
{
typesBuilder.Free();
return null;
}

return TupleTypeSymbol.Create(locationOpt: null, elementTypes: typesBuilder.ToImmutableAndFree(), elementLocations: default(ImmutableArray<Location>), elementNames: default(ImmutableArray<string>), compilation: compilation, diagnostics: diagnostics);
Expand Down Expand Up @@ -327,7 +373,7 @@ private ArrayBuilder<DeconstructionVariable> BindDeconstructionAssignmentVariabl
var boundVariable = BindExpression(argument.Expression, diagnostics, invoked: false, indexed: false);
var checkedVariable = CheckValue(boundVariable, BindValueKind.Assignment, diagnostics);

checkedVariablesBuilder.Add(new DeconstructionVariable(checkedVariable));
checkedVariablesBuilder.Add(new DeconstructionVariable(checkedVariable, argument));
}
}

Expand Down Expand Up @@ -527,11 +573,11 @@ private ArrayBuilder<DeconstructionVariable> BindDeconstructionDeclarationLocals
DeconstructionVariable local;
if (variable.IsDeconstructionDeclaration)
{
local = new DeconstructionVariable(BindDeconstructionDeclarationLocals(variable, typeSyntax, diagnostics), node.Deconstruction);
local = new DeconstructionVariable(BindDeconstructionDeclarationLocals(variable, typeSyntax, diagnostics), variable.Deconstruction);
}
else
{
local = new DeconstructionVariable(BindDeconstructionDeclarationLocal(variable, typeSyntax, diagnostics));
local = new DeconstructionVariable(BindDeconstructionDeclarationLocal(variable, typeSyntax, diagnostics), variable);
}

localsBuilder.Add(local);
Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4917,4 +4917,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ExplicitTupleElementNames" xml:space="preserve">
<value>Cannot reference 'System.Runtime.CompilerServices.TupleElementNamesAttribute' explicitly. Use the tuple syntax to define tuple names.</value>
</data>
<data name="ERR_DeconstructCouldNotInferMergedType" xml:space="preserve">
<value>The type information on the left-hand-side '{0}' and right-hand-side '{1}' of the deconstruction was insufficient to infer a merged type.</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,6 +1365,7 @@ internal enum ErrorCode

ERR_PredefinedTypeMemberNotFoundInAssembly = 8205,
ERR_MissingDeconstruct = 8206,
ERR_DeconstructCouldNotInferMergedType = 8209,
ERR_DeconstructRequiresExpression = 8210,
ERR_DeconstructWrongCardinality = 8211,
ERR_CannotDeconstructDynamic = 8212,
Expand Down
Loading