Method group invocation syntax resolves Enumerable rather than Queryable

[roji]

Not sure to what extent this is by-design: it seems like when a LINQ operator is used with method group syntax, the enumerable version is resolved instead of the queryable one.

class Program
{
    public static void Main()
    {
        IQueryable<int> ints = new[] { 1, 2, 3}.AsQueryable();

        var x = ints.Select(i => Foo(i)); // Queryable.Select is resolved, and x is correctly typed as IQueryable<int>
        var y = ints.Select(Foo); // Enumerable.Select is resolved, and y is typed as IEnumerable<int>
    }

    static int Foo(int x) => x;
}

Seen on dotnet SDK 6.0.100-preview.7.21377

[svick]

I believe this is the expected behavior. The section Anonymous function conversions of the C# spec says:

A lambda expression F is compatible with an expression tree type Expression<D> if F is compatible with the delegate type D.

But the Method group conversions section says no such thing.

So since there is no conversion from a method group to an Expression<T>, only Exnumerable.Select is applicable in the second example, which is why it is chosen. Maybe this code should produce a warning?

[roji]

This seems like quite a pit of failure, with IDE suggestions to replace lambdas with method groups where possible... A warning definitely seems appropriate.

[svick]

@roji Yeah, I think it would be a good idea to disable that suggestion for Expression<T>.

[CyrusNajmabadi]

Yeah, that suggestion should not be active there

[slavanap]

@roji
Such var type deduction logic is by design because Foo is a delegate in original code block. Alternative solution would be

class Program
{
    public static void Main()
    {
        IQueryable<int> ints = new[] { 1, 2, 3 }.AsQueryable();

        var fooCompiled = Foo.Compile();
        var x = ints.Select(i => fooCompiled(i));
        var y = ints.Select(Foo);
    }

    static readonly Expression<Func<int, int>> Foo = x => x;
}

.Compile() here is for demo purpose only, not for production code. If one would need to chain expressions I have written a NuGet package for such purpose. (slavanap.Expressions).

E.g.

/* all variables are deduced as Expression<Func<int, int>> */
var foo = Expr.New ( (int x) => x + 1 );
var bar = Expr.New ( (int y) => y * 2 );
var foobar = Expr.Unfold( (int z) => foo.Use(bar.Use(z)) );

Console.WriteLine(foobar); // prints z => z*2 + 1

Demo

I'd agree with Area-IDE.

[roji]

Aside from not suggesting at the IDE level, I'd also consider @svick's suggestion above of issuing a warning for this.

[slavanap]

Not sure about the warning, but an analyser can catch that.
On the other side IQueryable<T> inherits IEnumerable<T>, which makes IEnumerable extensions applicable to it, but if there's a need to enforce Queryable extensions then there's always a way to call them explicitly or to always supply Expression type into a function. I.e. these 2 methods won't compile.

public static void Main() {
    // Method #1
    {
        IQueryable<int> ints = new[] { 1, 2, 3 }.AsQueryable();
        var x = Queryable.Select(ints, i => Foo(i));
        var y = Queryable.Select(ints, Foo);
    }
			
    // Method #2 (Nuget package)
    {
        IQueryable<int> ints = new[] { 1, 2, 3}.AsQueryable();
        var x = ints.Select(Expr.New(i => Foo(i)));
        var y = ints.Select(Expr.New(Foo));
    }
}

static int Foo(int x) => x;

UPD:

// Method #3

using static System.Linq.Queryable;

class Program
{
    public static void Main()
    {
        var ints = new[] { 1, 2, 3 }.AsQueryable();

        var x = ints.Select(i => Foo(i));
        var y = ints.Select(Foo);
    }

    static int Foo(int x) => x;
}

[roji]

there's a need to enforce Queryable extensions then there's always a way to call them explicitly or to always supply Expression type into a function

The problem isn't with having a workaround - lambda syntax can always be used etc. It's more about the pit of failure of users accidentally invoking Enumerable.Select when Queryable.Select was intended (see dotnet/efcore#25377)

[slavanap]

@roji, sometimes there's a need to enumerate stream of SQL response synchronously. What you're suggesting is to show unnecessary warning in such scenarios and violate initial design of IQueryable<T> being inherited from IEnumerable<T>. Rather the warning can be shown in EF Core on an attempt of GetEnumerator() call on an entity set which is represented by IQueryable<T> typed object. Previously there were similar issues of calling untranslatable-to-SQL delegates in expressions which resulted in full records being pulled from the database which you actually suggest in
dotnet/efcore#25377 (comment)

I.e. you're suggesting to pull full row (blog.rating is selected) instead of selecting just chosen fields from the DB. In my opinion we need to teach users to leverage Expressions rather than adding warnings which violate interfaces design.

Answer on how to effectively leverage Expression generic is in the first part of my comment #55325 (comment)

Expression<Func<int, int>> Foo = x => x;
var y = ints.Select(Foo);

[roji]

sometimes there's a need to enumerate stream of SQL response synchronously.

Of course - and System.Linq already provides AsEnumerable for explicitly expressing enumeration of queryables rather than adding a node to the translated expression tree; that would be the way to avoid the warning/diagnostic. Similar diagnostics already exist in scenarios which have a high risk of being problematic: CA2007 proposes adding explicit ConfigureAwait when awaiting Tasks, while CA1310/CA1307 proposes adding explicit StringComparison values. In both these cases, the code producing the diagnostic isn't necessarily wrong or incorrect, but there's a high likelihood of trouble justifying the diagnostic (not necessarily at warning level by default).

At the end of the day, looking at it purely from the perspective of a dev writing C# code, Select(i => Foo(i)) is equivalent to Select(Foo) - the latter is a terser alternative to the former (to the point where the IDE suggests that simplification). This Queryable/Enumerable resolution behavior is one instance where this breaks down, and can be quite a pit of failure... Ideally the C# itself would allow method groups to be resolved to Expression, but changing that isn't feasible.

[CyrusNajmabadi]

@jinujoseph This would be a good issue to use for someone to look at to investigate and work on a fix for. Thanks!

[CyrusNajmabadi]

This seems like quite a pit of failure, with IDE suggestions to replace lambdas with method groups where possible... A warning definitely seems appropriate.

@roji can you clarify this? I don't believe there is any roslyn feature that is suggesting that you simplify this lambda. Can you provide a screenshot for this, along with what error code/message you're getting here? I'm going to preemptively close this out as i think it might be another tool you're using. If it turns out to be roslyn, we can reopen this.

[CyrusNajmabadi]

This Queryable/Enumerable resolution behavior is one instance where this breaks down, and can be quite a pit of failure...

We would not be able to change this as it would be a massive breaking change. You could also write an analzyer here to look for this and warn you if it's a pattern you want to ban from your own codebases.

[roji]

@CyrusNajmabadi you're right, upon further examination the IDE suggestion to convert to method group came from Resharper/Rider, and it turns out that they also turn off that suggestion for queryables. They also issue a diagnostic for this ("IQueryable is possibly unintentionally used as IEnumerable").

The only actionable thing I do see here is adding that diagnostic to Roslyn, as Resharper/Rider do and as @svick suggested above.

[CyrusNajmabadi]

I don't see it being possible ot add a diagnostic here because of hte massive breaking change. HOwever, tagging @jaredpar as a potential warning wave possibility. I would say this wouldnt' be in roslyn either. It would be up tot he BCL to decide if they wanted to chekc for this and have a suggestion on how to fix this up.