Deconstruction-assignment: expression type should be void #11594
Conversation
| @@ -54,6 +54,7 @@ public override BoundNode VisitDeconstructionAssignmentOperator(BoundDeconstruct | |||
| stores.Add(assignment); | |||
| } | |||
|
|
|||
| stores.Add(new BoundVoid(node.Syntax, node.Type)); | |||
There was a problem hiding this comment.
The context is that we need to build an expression with a void return type. As this expression is lowered into a sequence, the sequence automatically picks up its return type to be the type of it's last step. And there are checks all over the place that enforce that.
This is the approach we discussed with Vlad and Neal. We also considered tweaking the existing checks, or creating a VoidSequence, neither of which seems advantageous compared to BoundVoid. I'm happy to discuss options though.
|
LGTM |
|
It appears the LDM is likely to change their mind about the type of a tuple assignment. |
|
@gafter Yes, I noticed the discussion ;-) Even if we do change in the end, void return is a useful stepping stone and better than the implementation I had (using the type of the last assignment), as demonstrated by the unittests that can now be un-skipped. Also, this will allow me to start working on nesting cases without worrying about last assignment type. |
|
|
|
Thanks for the reviews. |
| @@ -402,7 +402,7 @@ public void Deconstruct(out int a, out string b, out string c) | |||
| comp.VerifyDiagnostics(); | |||
| } | |||
|
|
|||
| [Fact(Skip = "PROTOTYPE(tuples)")] | |||
| [Fact] | |||
There was a problem hiding this comment.
I'd add a test mixing deconstruction assignment and a regular ones. Just to make sure it works.
(a, b) = c = d = e;
and mix with postfix operator. - Just to make sure deconstruction composes in the few scenarios where it is possible.
(a, b) = c++; May need to declare extension Deconstruct on Int32 to make this work.
There was a problem hiding this comment.
Thanks for the test ideas. I'll add.
|
LGTM |
LDM settled on a void return type for deconstruction-assignment expressions.
The change below introduces
BoundVoidand uses it as the last bound node in the sequence generated by lowering deconstruction-assignments.I'm also unskipping a number of tests that pass as a result.
Addresses the first issue in the deconstruction backlog: #11299
@dotnet/roslyn-compiler for review.