Adding ITuple interface and implementation in ValueTuple #10068
Conversation
|
@stephentoub Let me know how we should proceed. I'm happy to schedule a discussion if you feel it would help. |
| /// <summary> | ||
| /// The number of positions in this data structure. | ||
| /// </summary> | ||
| public int Length => 0; |
There was a problem hiding this comment.
I think these should be explicit implementations. I don't think we want to prevent Length from being a valid tuple "friendly" element name. #Resolved
There was a problem hiding this comment.
Yes, I'll do that. #Resolved
|
|
| { | ||
| get | ||
| { | ||
| return 7 + ((ITuple)Rest).Length; |
There was a problem hiding this comment.
Casting to ITuple is going to box Rest. The box allocation can be avoided by removing the cast (which works without boxing because of the constraint):
return 7 + Rest.Length;
|
Other than Justin's comments, LGTM. @terrajobst, anything we need to do here from an API review perspective? |
|
@terrajobst, are we good to move forward with this PR? |
| @@ -920,7 +920,7 @@ public static class TupleExtensions | |||
| } | |||
| #endregion | |||
|
|
|||
| private static ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLong<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest) where TRest : struct => | |||
| private static ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest> CreateLong<T1, T2, T3, T4, T5, T6, T7, TRest>(T1 item1, T2 item2, T3 item3, T4 item4, T5 item5, T6 item6, T7 item7, TRest rest) where TRest : struct, ITuple => | |||
| new ValueTuple<T1, T2, T3, T4, T5, T6, T7, TRest>(item1, item2, item3, item4, item5, item6, item7, rest); | |||
There was a problem hiding this comment.
Why do we need the constraint to struct?
There was a problem hiding this comment.
The TRest must be a nested ValueTuple.
Unfortunately, we don't have a good way to express that constraint, so I am using a weaker (but correct) constraint. ValueTuple is a struct.
There was a problem hiding this comment.
Why ITuple is not enough of a constraint?
There was a problem hiding this comment.
ITuple is public (other types can implement it). It applies to any type that can be used in dynamic pattern matching (when you want to break an object into its elements at runtime).
Also ITuple says nothing about class vs struct, but ValueTuple must be a struct to be usable by the compiler (this will be implemented shortly, dotnet/roslyn#11689).
There was a problem hiding this comment.
We seem to completed a cycle: I asked why it has to be a struct, and now you said that [because] "ValueTuple must be a struct". So, again, why does it have to be a struct?
There was a problem hiding this comment.
Yeah, I love the fact that now tuples (ValueTyple*) are structs. I just think that constraining TRest to struct is unnecessary. If TRest is generated by the compiler, you will make it a struct, i.e. you don't need the constraint.
There was a problem hiding this comment.
I don't have a strong opinion on the struct constraint, but note that removing it will likely force us to add a bit more code, e.g. to protect against Rest being null here:
https://github.com/dotnet/corefx/blob/master/src/System.ValueTuple/src/System/ValueTuple/ValueTuple.cs#L2042
Since we really want this to be constrained to be a value tuple, all of which are structs, and we don't expect nor want anything else in this position, why remove the constraint? What harm does it cause to have it (i.e. what scenario are you envisioning)?
There was a problem hiding this comment.
First, I was simply asking why we have the constraint, not trying to push to remove it. And so I gather the answer is that we don't want to do null checks in ToString?
But secondly, I wonder if it would not be better for rest to be a class after a certain arity. I think past certain arity, the struct will get so large that it would be cheaper to allocate on the heap than to make the copies.
There was a problem hiding this comment.
That's not the reason; I was simply pointing out a side effect of removing it.
There was a problem hiding this comment.
For me, the reason is that we want to constrain to a ValueTuple, but we can't express that, so we apply the maximum constrain that we can express.
I find the idea of switching to nesting class (most likely System.Tuple) after a certain arity intriguing. But that means the compiler would have to be aware of that type too. Even if we removed the struct constraint to leave room for such an option in the future, we'd face the problem that ValueTuple and System.Tuple don't implement the same internal interface.
|
From corefx review and discussion, Closing this PR. |
|
@gafter This is the PR I had punted. I will use this change as template for the various mscorlibs. |
Re-opening the discussion following C# language design meeting yesterday.
The purpose of this interface is to allow objects to be used in positional pattern matching.
ValueTupleis the first type to implement it.From discussion with LDM yesterday, it seems that existing collection interfaces (IReadOnlyCollection and IReadOnlyList) don't fit the purpose. We feel there should be no iterator involved. Also we need an interface as a marker to identify types that are suitable for such usage.
We are still having some debate on the proper name for this interface (ITuple, IDeconstructable, IIndexable, ...).
I renamed
Sizeto beLengthto avoid introducing a new term.