Proposal: Supporting C# 7 deconstruction of certain types.

[jamesqo]

Background

C# 7 added support for deconstruction of user-defined types via a Deconstruct(out ...) method. (see it in action) It would make sense to be able to use this feature with tuple-like types such as KeyValuePair and DictionaryEntry.

Proposal

public struct KeyValuePair<TKey, TValue>
{
    // BLOCKED (do not add now): [EditorBrowsable(EditorBrowsableState.Never)]
    public void Deconstruct(out TKey key, out TValue value);
}

public struct DictionaryEntry
{
    // BLOCKED (do not add now): [EditorBrowsable(EditorBrowsableState.Never)]
    public void Deconstruct(out object key, out object value);
}

Usage

public void MyDictionaryMethod()
{
    var dict = new Dictionary<int, string>();
    dict.Add(1, "2");
    var (one, two) = dict.First();
}

[justinvp]

KeyValuePair<TKey, TValue>? DictionaryEntry? Any others?

[stephentoub]

@MadsTorgersen, what is the thinking about what types should expose deconstructors?

Personally, I think it makes sense for tuple-like types, e.g. KeyValuePair, DictionaryEntry, Tuple, ValueTuple, etc. I'm less convinced it's a good idea for other types just as a way to extract their fields.

[MadsTorgersen]

I agree completely. No reason to reinvent the dot.

[mellinoe]

The fixed-size types in System.Numerics.Vectors could be a decent candidate for this too: Vector2, Vector3, Vector4, Plane, Quaternion, Matrix3x2 (?), Matrix4x4 (?).

[jamesqo]

@stephentoub @MadsTorgersen I didn't think this would be a controversial proposal; I thought this was basically how deconstruction was intended to be used. However, if you disagree, I'll update the proposal to include just KeyValuePair/DictionaryEntry.

[JonHanna]

Of the types mentioned here, I'm not convinced about ArraySegment (but maybe I just don't use it enough), but with the drawing primitives I think it's common enough to want to get all instance properties in a use that it would make sense.

[jamesqo]

@JonHanna, just updated the proposal; see @stephentoub & @MadsTorgersen's comments.

[JonHanna]

Yep. I'm speaking in favour of that part of the original. I think tough there are going to be a bunch of debates in the near future in a bunch of .NET projects where one side thinks of a type as tuple-y and the other doesn't. (I'll admit to abusing Point to give me a quick integer 2-tuple in the days of .NET 1.1, so maybe that's colouring my thinking).

[jamesqo]

@karelz, maybe this is OK for review? I edited the original proposal and changed it to only include @justinvp's suggestions, namely KeyValuePair and DictionaryEntry. I think those types are pretty much intended to be used with this language feature.

[terrajobst]

@MadsTorgersen, @KrzysztofCwalina asked whether exposing a casting operator from TypeX to Tuple/ValueTuple would work, too. Does it have to be a Descontruct method?

I assume converting from a KeyValuePair<,> to ValueTuple<,> would work with an implicit conversion, while decontructing into multiple locals would not. Is that understanding correct?

[jamesqo]

@MadsTorgersen @terrajobst I just had an idea-- should we do this for Nullable as well? Usage:

(bool hasValue, T valueOrDefault) = nullable;

I think F# already allows you to do this for out-parameters.

[MadsTorgersen]

@jamesqo How would you use this? I fear that any real usage would get awkward, because you want to test the Boolean, but to do that you first have to extract it into a local.

It seems to me that nullables are better consumed with patterns or the ?? operator. Ideally they would have a TryGetValue method, which could be easily consumed with out variables.

[karelz]

@MadsTorgersen @KrzysztofCwalina can you please check @terrajobst's question above (https://github.com/dotnet/corefx/issues/13746#issuecomment-262335120)?

[MadsTorgersen]

@terrajobst @KrzysztofCwalina @karelz A conversion operator to a tuple doesn't provide deconstructability. This may or may not make sense, we chose to go with a conservative approach for now.

[karelz]

API review: Approved.

[AlexRadch]

@karelz Can I make PR for this or you works on it? You are assigned on this issue.

[KrzysztofCwalina]

@MadsTorgersen, the question was: why would exposing implicit cast to ValueTuple (instead of Deconstruct method) not be a way to expose deconstructability? Such cast would feel very natural on many types discussed in this thread. The Deconstruct method feels like some compiler plumbing that should not be public.

[karelz]

@AlexRadch assigning to you ... I forgot to remove myself.

[jamesqo]

@karelz, @KrzysztofCwalina We may end up having to make this an extension method instead... EditorBrowsableAttribute is not available in mscorlib.

edit: @KrzysztofCwalina in response to your comment

The Deconstruct method feels like some compiler plumbing that should not be public.

It will be marked [EditorBrowsable(EditorBrowsableState.Never)] so people should not see it in IntelliSense... there are already a bunch of Deconstruct extension methods for tuples here.

[karelz]

Yeah, moving those which would end up in mscorlib into extension methods sounds reasonable (@bartonjs to double check my claim).
The other option would be to move the attribute into mscorlib - @weshaggard is that an option?

[weshaggard]

The other option would be to move the attribute into mscorlib - @weshaggard is that an option?

We could probably do that in .NET Core but on .NET Framework I would be a little careful. When we type-forwarded ExtensionAttribute down for example there was fallout in all sorts of random places.

The other option would be to not mark them hidden in the places where they cannot be marked hidden. In generally we try to avoid using Extension methods for types we actually own, although we make them extension methods for Tuple. @jcouv do you know why we made them extension methods for Tuple? However if we really feel the need to be hidden then we can make them extensions if we want them to be consistent on all the platforms.

[jcouv]

do you know why we made them extension methods for Tuple?

@weshaggard I did a straight port from corefx. I thought it is better to have consistency across platforms.
I don't oppose making Deconstruct (and ToValueTuple probably) instance methods instead of extension methods.

[karelz]

I don't think we should pollute IntelliSense with methods which are only for compilers. We have to hide them from IntelliSense. If that means extension methods, so be it ...
cc: @terrajobst

[AlexRadch]

So I should NOT create deconstruction methods in these types but I can make deconstruction extensions for these types. Where I can make such extension methods?

[jamesqo]

@karelz In the PR associated with this proposal, @jkotas gave instructions on how to move EditorBrowsableAttribute down to CoreLib. Clearly you two seem to have differing conclusions on how to handle this-- I think you need to reach a consensus on whether 1) to use extension methods or 2) to move the type down.

[jkotas]

We just hit the same problem for Span<T>. Span<T> lives in CoreLib, some of its methods are meant to be marked EditorBrowsable(false), and changing the methods to extension methods is not an option - the methods have to be instance methods for other reasons.

[weshaggard]

Yes we can do it as part of that work, however as I mentioned earlier there is risk to desktop with moving it down. We hit all sorts of issues when we moved ExtensionAttribute down.

[karelz]

If moving the attribute on Desktop is risky/problematic (and I can imagine that), then we can consider: Not porting the APIs, or leaving them visible in IntelliSense, or adding extension methods on Desktop.
Neither solution seems ideal, but I think they are better than the alternative of giving up the innovation also on .NET Core and other platforms.

[karelz]

We should discuss the plan in API review (tomorrow) - I filed dotnet/corefx#14447.
Until then, this issue is blocked.

[weshaggard]

I wouldn't consider this issue blocked I would move forward with this issue and leave off those attributes for now. For this case we can also simply put those attributes only in the reference assembly which is the only place they are truly needed.

[karelz]

Fair, I updated the proposal to reflect the API changes (add them without the attribute).

[AlexRadch]

@karelz Thanks, I updated PR dotnet/coreclr#8560

[karelz]

@AlexRadch we need tests for the new APIs.
I have noticed you have a few half-finished API additions without tests. Can you please prioritize them over starting work on even more new APIs?

[AlexRadch]

@karelz I made PR in coreclr and when I should create tests in corefx repository? The PR can be not merged. Now I will try create test for deconstruction.

[karelz]

Thanks, please add them. I have noticed a few other issues assigned to you which are in similar state -- the coreclr change is done, but the tests are not finished yet.
Thank you!

[karelz]

The PR dotnet/corefx#14621 seems to be abandoned. Unassigning the issue - it is again "up for grabs", for anyone to pick it up and finish.

[geirsagberg]

Another useful feature would be deconstruction of anonymous types,. Given the following common pattern for extracting projections from LINQ:

var userNameAndId = MyContext.Users.Select(u => new { u.Id, u.Name }).First();
var userName = userNameAndId.Name;
var userId = userNameAndId.Is;

It would be nice to do the following:

var (userId, userName) = MyContext.Users.Select(u => new { u.Id, u.Name }).First();

The order of the variables would be the same as the order of the declarations in the anonymous type. Alternatively, one could name them if one wanted another order:

var (userName = Name, userId = Id) = MyContext.Users.Select(u => new { u.Id, u.Name }).First();

Of course, one would have to handle cases like .FirstOrDefault(), which might make every deconstructed variable nullable.

[karelz]

@geirsagberg I don't think that is something CoreFX can handle -- this sounds more like a feature of the compiler. Can you please file it there? (or check that it isn't filed or if it wasn't rejected before from some reason)

[geirsagberg]

Yup, will do!

[geirsagberg]

Created a discussion here: dotnet/csharplang#244

[safern]

I did a further investigation on this issue and looks like the implementation is already done. The PR that was abandoned was to add tests on corefx side since the implementation lives in coreclr.

The implementation was added in this PR: dotnet/coreclr#8560

Abandoned PR to add tests: dotnet/corefx#14621

So the only thing that we are missing to mark this issue as completed is to add tests in corefx.

[safern]

Taking this one.

[karelz]

👍

[ghost]

Guys, please explain which version of .Net Core and .Net Framework support Deconstruct method?
I just tried with net47 and it says it does not have deconstruction methods.

It looks that even netstandard 2.0 does not have Deconstruct methods in KeyValuePair:
https://raw.githubusercontent.com/dotnet/standard/master/docs/versions/netstandard2.0_ref.md

public struct KeyValuePair<TKey, TValue> {
         public KeyValuePair(TKey key, TValue value);
         public TKey Key { get; }
         public TValue Value { get; }
         public override string ToString();
     }

[karelz]

Deconstruct methods are only in .NET Core 2.0. They are not in any .NET Framework at this moment, or in lower versions of .NET Core (1.0 and 1.1).

[ghost]

Thanks Karel

In this case I will have to temporarily workaround it with extensions method:

public static class KeyValuePairExtension
{
	public static void Deconstruct<TKey, TValue>(this KeyValuePair<TKey, TValue> source, out TKey key, out TValue value)
	{
		key = source.Key;
		value = source.Value;
	}
}

[karelz]

Sounds like a good workaround.

[drewnoakes]

IGrouping<TKey, TElement> seems like another good candidate.

foreach (var (key, items) in foo.GroupBy(...))
{
    // ...
}