D-assignment should return a tuple instead of void #13115
Conversation
| // receiver for first Deconstruct step | ||
| var boundRHS = rhsPlaceholder ?? BindValue(right, diagnostics, BindValueKind.RValue); | ||
|
|
||
| boundRHS = FixTupleLiteral(checkedVariables, boundRHS, node, right, diagnostics); |
There was a problem hiding this comment.
Note: we now fix tuple literal whether they have a type or not.
There was a problem hiding this comment.
So the right-hand-side is already given the final type?
In reply to: 74643437 [](ancestors = 74643437)
|
@dotnet/roslyn-compiler This is now ready for review. Thanks |
| @@ -1765,6 +1767,21 @@ private void EmitObjectCreationExpression(BoundObjectCreationExpression expressi | |||
| } | |||
| } | |||
|
|
|||
| private bool ConstructorNotSideEffecting(BoundObjectCreationExpression expression) | |||
There was a problem hiding this comment.
Consider passing constructor symbol rather than BoundObjectCreationExpression.
|
LGTM |
| @@ -1743,11 +1743,13 @@ private void EmitObjectCreationExpression(BoundObjectCreationExpression expressi | |||
| } | |||
| else | |||
| { | |||
| if (!used && | |||
| expression.Constructor.OriginalDefinition == _module.Compilation.GetSpecialTypeMember(SpecialMember.System_Nullable_T__ctor)) | |||
| if (!used && ConstructorNotSideEffecting(expression.Constructor)) | |||
There was a problem hiding this comment.
ConstructorNotSideEffecting(constructor)
|
@dotnet/roslyn-compiler A second review would be appreciated. Thanks! |
|
Reviewing now. When you commit can you please squash commits whose log message aren't descriptive? |
| var compilation = _module.Compilation; | ||
|
|
||
| return originalDef == compilation.GetSpecialTypeMember(SpecialMember.System_Nullable_T__ctor) || | ||
| originalDef == compilation.GetWellKnownTypeMember(WellKnownMember.System_ValueTuple_T1__ctor) || |
There was a problem hiding this comment.
I'd put the check for System_ValueTuple_T1_ as the last one.
There was a problem hiding this comment.
There are many checks here that would typically fail. Is it possible to make a cheap easy-out check, like for the name of the type, before doing all the tuple checks?
|
@gafter (mispelled earlier) Let me know if you have any other feedback. |
|
LGTM |
|
🎉 Thanks for the review! |
With this change,
var t = (x, y) = (1, 2);is now allowed andtis a tuple.This works with long tuples and nested deconstruction as well.
In the process, I had to include the fix for the evaluation order. The new order is:
ValueTupleis not emitted unless it is consumed, as we require the construction ofValueTupleto be side-effect-free)As a result of this change, you need to reference the ValueTuple library whenever using deconstructions.
Also, you can see that more locals are created to store the outputs of conversions at step (4). I think we should be able to optimize in some more cases.
Fixes #12635 (return type) and #12400 (evaluation order).
FYI for @MadsTorgersen regarding evaluation order.