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

Deconstruction-assignment should return a tuple type #12635

Closed
jcouv opened this issue Jul 20, 2016 · 8 comments · Fixed by #13115
Closed

Deconstruction-assignment should return a tuple type #12635

jcouv opened this issue Jul 20, 2016 · 8 comments · Fixed by #13115
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers New Language Feature - Tuples Tuples

Comments

@jcouv
Copy link
Member

jcouv commented Jul 20, 2016

Per LDM 7/13, if we have time, let's have the return type be a tuple type, instead of void.
The tuple should only be formed if there is code to consume it.

@HaloFour
Copy link

Out of curiosity what is the reason for the change? I'm not questioning it as much as I'd like to understand it. I gather that having positional deconstruction return a tuple is easier to write particularly given that you can resort to simple expressions in the method. However they would also be a bit slower given the need to copy the resultant tuple back to the caller (and then potentially deconstruct again).

Can the community hope to see more LDM notes? It's been several months since the last drop and I think several members are getting a bit antsy here.

@andrew-skybound
Copy link

Why not have the compiler simply recognize properties with the names "Item1", "Item2" etc? Wouldn't that offer the best possible performance, since accessing those properties would be inlined anyway, and properties going into wildcards (*) would not even need to be accessed?

@HaloFour
Copy link

Why not have the compiler simply recognize properties with the names "Item1", "Item2" etc? Wouldn't that offer the best possible performance, since accessing those properties would be inlined anyway, and properties going into wildcards (*) would not even need to be accessed?

They could only be inlined if they were relatively simple, which the might be for types that are effectively tuples but could be more complicated if the individual members would need to be calculated. Being proper properties would pollute the members of existing types regardless of whether or not they're used as tuples. If properties were considered I'd rather attributes that would decorate existing properties which would specify the positional order. However, the use of properties at all shuts down the ability to define extension members to perform deconstruction, since "extension everything" isn't slated for C# 7.0.

I think using tuples works pretty well here. It aids in the case of the deconstruct method using (eventually) pattern matching expressions to construct the result.

One thing that a tuple return type does mess up is that you'd lose the ability to overload deconstruction by the arity or count of the tuple elements.

@jcouv
Copy link
Member Author

jcouv commented Jul 21, 2016

@andrew-skybound @HaloFour
Sorry, we're indeed behind on language discussions (we'll discuss that more on the other thread).

Regular assignment expressions have a value that you can continue with. For example: x = (y = 3);.
In the case of deconstruction-assignment, we have implemented a void return type which was unusual.
For consistency, we prefer to have a return type, which would be a tuple in this case.
But as you correctly point out, we don't want to have a performance penalty for common cases.
The current thinking is that we should be able to implement an optimization where the tuple won't be materialized or values copied, unless there is an consumer for the return value.

So if you do (x, y) = ToBeDeconstructed(); then there would be no tuple.
Only if you do something like var t = ((x, y) = ToBeDeconstructed()); or var t = ((x, (y, z)) = ToBeDeconstructed()).Item2 would a tuple be created.

@jcouv
Copy link
Member Author

jcouv commented Jul 21, 2016

One thing that a tuple return type does mess up is that you'd lose the ability to overload deconstruction by the arity or count of the tuple elements.

To clarify, the discussion of having the assignment return a tuple does not mean the deconstruction would change. We still use a Deconstruct method, but after the assignment is done we would gather the proper parts into a tuple for further usage.

@HaloFour
Copy link

@jcouv

So this is an implementation detail? What would the Deconstruct method look like? Either of the following?

public void Deconstruct(out int x, out int y) {
    x = 123;
    y = 456;
}

public (int x, int y) Deconstruct() {
    return (123, 456);
}

@jcouv
Copy link
Member Author

jcouv commented Jul 21, 2016

@HaloFour
The Deconstruct signature would remain the former:

public void Deconstruct(out int x, out int y) {
    x = 123;
    y = 456;
}

The benefits of this design is that you can introduce more parts over time (Deconstruct with 3 or 4 out parameters) and you can implement Deconstruct as an extension method.

@andrew-skybound
Copy link

@HaloFour

They could only be inlined if they were relatively simple, which the might be for types that are effectively tuples but could be more complicated if the individual members would need to be calculated.

Properties for the most common case of tuple-like types (Tuple, KeyValuePair, Point, Rectangle, Vector, etc) are all guaranteed to be inlined, regardless of the tuple arity, meaning that deconstructing one of these instances will be as cheap as individual assignments. ADeconstruct method for a type with at least 4 values is probably not going to be inlined.

Being proper properties would pollute the members of existing types regardless of whether or not they're used as tuples.

[EditorBrowsable(EditorBrowsableState.Never)]

If properties were considered I'd rather attributes that would decorate existing properties which would specify the positional order.

Using attributes would make it impossible to extend types which cannot be altered.

However, the use of properties at all shuts down the ability to define extension members to perform deconstruction, since "extension everything" isn't slated for C# 7.0.

The compiler could support GetItemX() methods as an alternative when ItemX properties are not found.

@jcouv

Sorry, I misinterpreted the topic to be referring to the return type of the Deconstruct method. I think the result of a deconstruction assignment expression should be the object which was itself deconstructed, in this case a tuple, just like how the result of an assignment expression is the r-value itself (the l-value is not "read back" or somehow recreated). If this is already what has been agreed on, that's great.

The Deconstruct signature would remain the former

I am worried about using out arguments for the return values because out arguments are not covariant like an actual return value is. I explain this in more detail in #12654 (comment).

@jcouv jcouv added this to the 2.0 (Preview 5) milestone Aug 9, 2016
@jcouv jcouv added the 4 - In Review A fix for the issue is submitted for review. label Aug 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers New Language Feature - Tuples Tuples
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants