Implement type merging in deconstruction with tuple literal #12526
Conversation
| } | ||
| } | ||
| "; | ||
| var comp = CompileAndVerify(source, expectedOutput: " 2 3", additionalRefs: new[] { ValueTupleRef, SystemRuntimeFacadeRef }); |
There was a problem hiding this comment.
Need to add a semantic info test for this scenario. We should see that the type of the (null, 2, 3) is null, converted type is (string, byte, int) and conversion is ImplicitTupleLiteral
There was a problem hiding this comment.
Yes, I'll add that. Thanks
|
LGTM |
Why would type merging ever be needed in a Tuple assignment expression? I understood the new rule to only affect the declaration scenario. |
|
@gafter You are correct. In an assignment, all the expressions on the left necessarily have a type, and so there is not type inference (only target typing). So in the "merging" all the types from from the left, whereas in a declaration, types can merge from both sides. |
|
Sure |
| var nestedLiteral = literal.Arguments[1]; | ||
| Assert.Equal(@"(1, 2)", nestedLiteral.ToString()); | ||
| Assert.Null(model.GetTypeInfo(nestedLiteral).Type); | ||
| Assert.Null(model.GetTypeInfo(nestedLiteral).ConvertedType); |
There was a problem hiding this comment.
@VSadov Could you comment on this? Is this what we should expect (null Type and ConvertedType)?
I couldn't find a test on tuple literal conversions with nested literals.
There was a problem hiding this comment.
@VSadov Ok to leave as-is?
There was a problem hiding this comment.
I think (1,2) must have a type and a converted type. You should be able to extract it into a temp/method and without a type it would be impossible.
I am actually surprised this does not "just work". It is a trivial tuple literal, it should have at least the natural type...
The problem could be somewhere else.
It would be acceptable to merge this as is and enter an issue to follow up.
|
@gafter Could you do the second code review? |
|
@dotnet/roslyn-compiler Please do a second code review. This PR is from last Thursday. |
|
I am in an ECMA meeting today and tomorrow. |
| /// </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) |
There was a problem hiding this comment.
Minor point: diagnostics is typically the last non-ref/out parameter.
|
LGTM |
| { | ||
| // typeless-variable on the left and typeless-element on the right | ||
| Error(diagnostics, ErrorCode.ERR_DeconstructCouldNotInferMergedType, syntax, variable.Syntax, element.Syntax); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| if ((object)element.Type == null) |
|
LGTM |
|
Thanks! I'll go ahead with merge. |
| // 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, Compilation, diagnostics); | ||
| if (lhsAsTuple != null) |
There was a problem hiding this comment.
Thanks. I'm still not quite sure when that is useful. Is it for certain types only (such as Symbols)?
There was a problem hiding this comment.
I'll queue that and your other suggestion in next PR.
There was a problem hiding this comment.
Only for types that define operator==.
This fix reflects the LDM decision from yesterday to support deconstructions such as
(string x1, byte x2, var x3) = (null, 1, 2);by hallucinating a tuple type which merges types from both sides and applying it as a conversion on the literal (which means the 1 is seen as a byte and the deconstruction succeeds).The change is to update the method which figures the target type. It used to only consider the LHS, but now it merges types from both sides. This literal conversion is applied both in declarations and assignments.
There is also a minor tweak to improve the syntax associated with
DeconstructionVariable, which is visible in the new diagnostics.@dotnet/roslyn-compiler for review.
Relates to deconstruction work items: #11299
Relates to issue #12410