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

Tuple and extension methods #16159

Closed
alrz opened this issue Dec 30, 2016 · 36 comments
Closed

Tuple and extension methods #16159

alrz opened this issue Dec 30, 2016 · 36 comments

Comments

@alrz
Copy link
Contributor

@alrz alrz commented Dec 30, 2016

using System;
using System.Linq;
using System.Collections.Generic;
static class C
{
    static IEnumerable<(T, U)> AsEnumerable<T, U>(
        this (IEnumerable<T> xs, IEnumerable<U> ys) source)
	=> source.xs.Zip(source.ys, (x, y) => (x, y));

    static void Main()
    {
        foreach(var (x, y) in AsEnumerable((new int[1], new byte[2]))) // OK
        {          
        }      

        foreach(var (x, y) in (new int[1], new byte[2]).AsEnumerable()) // ERROR; needs cast
        {    
        }  
    }   
}

In fact, an extension GetEnumerator should be sufficient without the explicit call.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Dec 30, 2016

Aside: AsEnumerable means something else; I'd definitely call that extension method Zip.

@alrz
Copy link
Contributor Author

@alrz alrz commented Dec 30, 2016

@jnm2 Thas is just an example, As I said this should be possible with a GetEnumerator extension method,

foreach(var (x, y) in (new int[1], new byte[2])) { }
@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Dec 30, 2016

@alrz I see what you're doing there, but I'd call it GetEnumerator if it was going to return IEnumerator<(T, U)> and Zip if it was going to return IEnumerable<(T, U)>.

@gafter
Copy link
Member

@gafter gafter commented Dec 30, 2016

This does not work because an extension method is only applicable when "An implicit identity, reference or boxing conversion exists from expr to the type of the first parameter of [the method]." In this case none of those conversions are applicable; it requires an implicit tuple conversion. This is therefore a decision that required an explicit change from the LDM.

@dotnet/ldm Do you want to make a change here to the interaction of tuples with extension methods? If so, it is now or never.

@HaloFour
Copy link

@HaloFour HaloFour commented Dec 30, 2016

What are the scenarios under which this doesn't currently work? I was toying with a joining extension method for ValueTuple<Task<T1>, Task<T2>> and it seemed to compile just fine (using Dec 14th build on tryroslyn)

public static class TaskJoinExtensions {
    public static TaskAwaiter<(T1, T2)> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks) {
        // join implementation here
    }
}
// later

var (result1, result2) = await (task1, task2);
@alrz
Copy link
Contributor Author

@alrz alrz commented Dec 30, 2016

@HaloFour Because what GetAwaiter accepts and (task1, task2) type both are (Task<T1>, Task<T2>). In the case above we are upcasting T[] to IEnumerable<T>. that's where it breaks. i.e. if you cast arrays to IEnumerable<T> or change the extension method to accept arrays, it compiles just as expected.

@HaloFour
Copy link

@HaloFour HaloFour commented Dec 30, 2016

@alrz

Oh, I see. You're relying on variance/coercion of the generic type parameters of the tuple. Messy, but I agree that it would be useful.

@alrz alrz changed the title Type inference on tuple extension methods Tuple conversion and extension methods Dec 30, 2016
@alrz
Copy link
Contributor Author

@alrz alrz commented Dec 31, 2016

While changing the types would satisfy the compiler,

public static IEnumerable<(T, U)> AsEnumerable<T, U>(
  this (T[] xs, U[] ys) source)
  => source.xs.Zip(source.ys, (x, y) => (x, y));

foreach(var (x, y) in (new int[1], new byte[2]).AsEnumerable()) // OK

It still doesn't find GetEnumerator method,

public static IEnumerator<(T, U)> GetEnumerator<T, U>(
  this (T[] xs, U[] ys) source)
  => source.xs.Zip(source.ys, (x, y) => (x, y)).GetEnumerator();

foreach(var (x, y) in (new int[1], new byte[2])) // ERROR

Am I missing something?

@gafter
Copy link
Member

@gafter gafter commented Jan 3, 2017

@alrz That should work. But it is a separate issue from the OP (which is a language design feature request). I'll file a bug report.

[EDIT 2017-01-03] Checking the C# language spec, I see that extension methods are not specified to work for foreach loops.

@alrz
Copy link
Contributor Author

@alrz alrz commented Jan 3, 2017

As another use case, this would allow dispose by convention for tuples (#11420),

static class E 
{
    public static void Dispose(this (IDisposable, IDisposable) @this)
    {
    	(default(Stream), default(Stream)).Dispose(); // ERROR
    }
}

As a workaround you should cast each element to IDisposable,

        ((IDisposable)default(Stream), (IDisposable)default(Stream)).Dispose(); // OK

or use constrained generics,

    public static void Dispose<T, U>(this (T, U) @this)
        where T : IDisposable
        where U : IDisposable
    {
    	(default(Stream), default(Stream)).Dispose(); // OK
    }

You can write AsEnumerable above this way but you'd have 4 generic types that the compiler cannot infer.

@terrajobst
Copy link
Member

@terrajobst terrajobst commented Jan 3, 2017

@gafter, so is the problem related to extension methods or is the problem more general in the sense that the compiler doesn't allow an implicit conversion between tuple type T1 and T2 if there is an implicit conversion for all tuple element types?

@gafter
Copy link
Member

@gafter gafter commented Jan 3, 2017

@terrajobst The problem in this issue is that the user is expecting an extension method to be applicable when the receiver type is convertible to the extension method's first parameter by a tuple conversion, but not by any of the conversions for which the language specifies that extension methods are to be applicable. This is therefore a feature request. As stated earlier:

This does not work because an extension method is only applicable when "An implicit identity, reference or boxing conversion exists from expr to the type of the first parameter of [the method]." In this case none of those conversions are applicable; it requires an implicit tuple conversion. This is therefore a decision that required an explicit change from the LDM.

@dotnet/ldm Do you want to make a change here to the interaction of tuples with extension methods? If so, it is now or never.

@gafter
Copy link
Member

@gafter gafter commented Jan 3, 2017

In other words, the compiler allows a conversion between the tuple types, but won't use the existence of that conversion to make an extension method to be applicable.

@terrajobst
Copy link
Member

@terrajobst terrajobst commented Jan 3, 2017

I see, so there is a tuple conversion between (T1[], T2[]) and (IEnumerable<T1>, IEnumerable<T2>) but extension methods don't consider tuple conversions.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Jan 3, 2017

I see value in this. Besides getting rid of generic parameter per value, tuples are supposed to be conceptually like an argument list rather than conceptually a structured type. From my perspective, the more that operations on tuples are distributive as operations on each individual value, the better.

@terrajobst
Copy link
Member

@terrajobst terrajobst commented Jan 4, 2017

Extending the conversions rules for extension methods seems desirable, I agree.

@MattGertz MattGertz modified the milestones: 2.1, 2.0 (RC.3) Jan 14, 2017
@alrz
Copy link
Contributor Author

@alrz alrz commented Jan 15, 2017

If so, it is now or never.

Wouldn't this be a breaking change if (possibly) done in 2.1 according to the comment above?

@VSadov
Copy link
Member

@VSadov VSadov commented Jan 17, 2017

I think the reason for the current rule is to basically say "extension method is applicable if the receiver type is identical to the one declared or declared is one of its bases/interfaces". We cannot just add implicit tuple conversions to the mix. Those are too broad.

We could, however, in a case of implicit tuple conversion, require that all underlying conversions comply with the current rule. - I.E. recursively distribute the current rule for tuples.

@VSadov
Copy link
Member

@VSadov VSadov commented Jan 17, 2017

Not sure if this can be pushed to 2.1
Changes in overloading resolution are often breaking via picking a different overload or allowing conflicting candidates.

@jcouv jcouv assigned jcouv and unassigned MadsTorgersen Jan 17, 2017
@jcouv
Copy link
Member

@jcouv jcouv commented Jan 17, 2017

LDM approved this for 2.0 RTM. We'll still have to run this by shiproom, but this appears to satisfy the "now or never" bug bar.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

This black magic makes me incredibly happy:

public static TaskAwaiter<(T1, T2)> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks)
{
    return Task.WhenAll(tasks.Item1, tasks.Item2).ContinueWith(_ => (tasks.Item1.Result, tasks.Item2.Result)).GetAwaiter();
}

var (x, y) = await (thingX.OperationAsync(), thingY.OperationAsync());
@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

The slightly more conscientious me:

public static TaskAwaiter<(T1, T2)> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks)
{
    return Task.WhenAll(tasks.Item1, tasks.Item2)
        .ContinueWith((whenAllTask, state) =>
        {
            whenAllTask.GetAwaiter().GetResult(); // Throw any exceptions
            var stateTasks = ((Task<T1>, Task<T2>))state;
            return (stateTasks.Item1.Result, stateTasks.Item2.Result);
        }, tasks)
        .GetAwaiter();
}

Or perhaps the "proper" implementation:

public static TupleTaskAwaiter<T1, T2> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks)
{
    return new TupleTaskAwaiter<T1, T2>(tasks);
}

public struct TupleTaskAwaiter<T1, T2> : INotifyCompletion
{
    private readonly (Task<T1>, Task<T2>) tasks;
    private readonly TaskAwaiter whenAllAwaiter;

    public TupleTaskAwaiter((Task<T1>, Task<T2>) tasks)
    {
        this.tasks = tasks;
        whenAllAwaiter = Task.WhenAll(tasks.Item1, tasks.Item2).GetAwaiter();
    }

    public bool IsCompleted => whenAllAwaiter.IsCompleted;
    public void OnCompleted(Action continuation) => whenAllAwaiter.OnCompleted(continuation);

    public (T1, T2) GetResult()
    {
        whenAllAwaiter.GetResult();
        return (tasks.Item1.Result, tasks.Item2.Result);
    }
}
@jaredpar
Copy link
Member

@jaredpar jaredpar commented Feb 9, 2017

Using await on a tuple of task is ... beautiful.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

I've done the first 10 so people don't have to. https://gist.github.com/jnm2/3660db29457d391a34151f764bfe6ef7

I could see this finding a nice place in the BCL.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

Hmm, I should probably have added GetAwaiter(this (Task, Task, ... overloads returning Task.WhenAll(...).GetAwaiter().

@jcouv jcouv mentioned this issue Feb 9, 2017
42 of 50 tasks complete
@jcouv
Copy link
Member

@jcouv jcouv commented Feb 9, 2017

@jnm2 Sweet. You could probably turn this into a corefx PR to start discussion with BCL team. Either way, I took a note to discuss with them.

@HaloFour
Copy link

@HaloFour HaloFour commented Feb 9, 2017

@jnm2

Nice work. I only got so far as the toying-around phase, but I was absolutely salivating at the prospect of using tuples, extension methods and deconstruction together to create join patterns. It'd be awesome if this were in the BCL.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

@jcouv Should I start an issue or just PR?

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

Another question I have is whether there is any way to propagate tuple element names. I don't think there is a way, but please tell me if I'm wrong. It may not be desirable in any case.

@jcouv
Copy link
Member

@jcouv jcouv commented Feb 9, 2017

@jnm2 Filed issue to start discussion with CoreFX team: https://github.com/dotnet/corefx/issues/16010

I didn't quite follow your second question (regarding tuple element names). Could you clarify your scenario or problem?

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Feb 9, 2017

@jcouv Is it possible for the task names in the awaited tuple to flow to the tuple names in the result? I don't think so, and I'm not sure it would be a good thing either, but I'm curious and this is what it would look like:

var tuple = await (x: thingX.OperationAsync(), y: thingY.OperationAsync());
Console.WriteLine(tuple.x); // Rather than tuple.Item1

Just in time lol http://mustoverride.com/tuples_names/

@jcouv
Copy link
Member

@jcouv jcouv commented Feb 9, 2017

@HaloFour Could you share some examples (API and usage) of the join patterns you have in mind? Ideally, you could document a proposal in a corefx issue too.

I've just created one for Zip and Unzip: https://github.com/dotnet/corefx/issues/16011

@jcouv
Copy link
Member

@jcouv jcouv commented Feb 9, 2017

@jnm2 Right, tuple names don't pass through generic methods.
But you could deconstruct: var (x, y) = await (thingX.OperationAsync(), thingY.OperationAsync()); then use x.

@Richiban
Copy link

@Richiban Richiban commented Mar 29, 2017

@gafter

[EDIT 2017-01-03] Checking the C# language spec, I see that extension methods are not specified to work for foreach loops.

Is this a deliberate decision? Since discovering that extension methods are allowed elsewhere, such as await, it's a frustrating limitation that we can't use them in foreach. If there's no reason you know of that this is deliberately the case I might open a separate issue.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Aug 25, 2017

Fyi, added ConfigureAwait support to TaskTupleExtensions.

@jnm2
Copy link
Contributor

@jnm2 jnm2 commented Jul 31, 2018

I updated the gist. tuple.ConfigureAwait was causing only the first two tasks to be awaited due to a copy/paste error. 🤦‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.