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

.NET 6.0 Enumerable.MaxBy conflicts with Enumerable.MaxBy #1647

Closed
clement911 opened this issue Nov 19, 2021 · 18 comments
Closed

.NET 6.0 Enumerable.MaxBy conflicts with Enumerable.MaxBy #1647

clement911 opened this issue Nov 19, 2021 · 18 comments

Comments

@clement911
Copy link

.NET 6.0 introduces built-in methods for MaxBy/MinBy which have the same signature as the IX ones.
As a result, after upgrading to .NET 6.0, I get the following error:

The call is ambiguous between the following methods or properties: 'System.Linq.EnumerableEx.MaxBy<TSource, TKey>(System.Collections.Generic.IEnumerable, System.Func<TSource, TKey>)' and 'System.Linq.Enumerable.MaxBy<TSource, TKey>(System.Collections.Generic.IEnumerable, System.Func<TSource, TKey>)'

@clement911
Copy link
Author

Actually the return type is different as it is a List for IX but a single value for .NET 6.

@clairernovotny
Copy link
Member

We'll get this fixed soon. Are there any other conflicts due to new methods/overrides?

@clement911
Copy link
Author

That's the only one I noticed but I'm not sure if there are any others.

@clairernovotny
Copy link
Member

Fixed in 5.1.

@clement911
Copy link
Author

Hi @clairernovotny
Did you remove these methods altogether for NET 6.0?
We still would like to use them.
You see, the ones built-in return a single element where as the IX ones return a list.
Would it be possible to re-add them to NET 6.0 but use a different name to avoid a conflict?
Maybe MaxByAll? Or MaxByWithTies?

What do you think?

@clairernovotny
Copy link
Member

@terrajobst @bartdesmet for naming/suggestions.

@SicJG
Copy link

SicJG commented Dec 15, 2021

If i'm not mistaken the problem with ambiguity is bigger and solution provided for MaxBy/MinBy in 5.1 might be accidentally broken after future minor and major .NET releases

Current pattern for solving ambiguity that i see in sources is wrapping duplicating methods with compiler directive for hiding method when referencing from some target frameworks from reference assembly. For example hiding of MaxBy was implemented by following directive (MinBy solution is simular):

#if !(REFERENCE_ASSEMBLY && (NET6_0))
    public static IList<TSource> MaxBy<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector){...}
    public static IList<TSource> MaxBy<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, IComparer<TKey> comparer){...}
    private static IList<TSource> ExtremaBy<TSource, TKey>(IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TKey, TKey, int> compare){...}
#endif 

Problem that i see in this solution is that we are checking especially that are we targeting .net6.0 listed in reference assembly project targetFrameworks:
<TargetFrameworks>net4.8;netstandard2.1;net6.0</TargetFrameworks>
it works for now, but if we change target after .NET 7 release to <TargetFrameworks>net4.8;netstandard2.1;net7.0</TargetFrameworks>
it's going to break.

To fix it we must somehow set not just NET6_0, but something simular to NET6_0 || NET6_1 || NET7_0 ...
If we use .net 5+ sdk for development process it can be achieved using NET6_0_OR_GREATER constant

Actually problem is already hurting us:
Discribed pattern was introduced in #734 #743 (problem and solution desribed by @clairernovotny here and from then SkipLast/TakeLast methods are wrapped with following compiler directives

#if !(REF_ASSM && NETCOREAPP2_0) <= original in https://github.com/dotnet/reactive/pull/734 c9bffe5ef15a245f7bed0b6ec060ed99091b3a06 
#if !(REFERENCE_ASSEMBLY && (NETCOREAPP2_1 || NETSTANDARD2_1)) <= current version
    public static IEnumerable<TSource> SkipLast<TSource>(this IEnumerable<TSource> source, int count){...}
    public static IEnumerable<TSource> TakeLast<TSource>(this IEnumerable<TSource> source, int count){...}
#endif

ref assembly at that time targeted: <TargetFrameworks>netstandard1.0;netcoreapp2.0;netstandard2.1</TargetFrameworks>
time by time it has changed to current state resulting following demo project IxCompatibilityWithNet6.zip not compile now with error:
Test.cs(7, 21): [CS0121] The call is ambiguous between the following methods or properties: 'System.Linq.EnumerableEx.SkipLast<TSource>(System.Collections.Generic.IEnumerable<TSource>, int)' and 'System.Linq.Enumerable.SkipLast<TSource>(System.Collections.Generic.IEnumerable<TSource>, int)'
Csproj:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFramework>net6.0</TargetFramework>        
        <Nullable>enable</Nullable>
        <LangVersion>default</LangVersion>
    </PropertyGroup>

    <ItemGroup>
      <PackageReference Include="System.Interactive" Version="5.1.0" />
    </ItemGroup>

</Project>

Cs:

using System.Linq;

namespace IxCompatibilityWithNet6;

public sealed class Test
{
    public static void Run()
    {
        new[] { 1 }.SkipLast(1);
    }
}

Problem reproduces only on net6.0 as it is explicitly targeted in reference assembly

@clement911
Copy link
Author

Using NET6_0_OR_GREATER constant sounds good.
However the issue here is that not only MaxBy and MinBy were added to .NET, but also that their return type is different.
We are targeting net6.0 but we still need to compile our code with the IX versions of the methods.

In Enumerable:
public static TSource? MaxBy<TSource,TKey> (this IEnumerable<TSource> source, Func<TSource,TKey> keySelector);

In EnumerableEx:
public static IList<TSource> MaxBy<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector);

Given that the return type is different, I believe the only option to make the IX MaxBy available in .NET 6+ is to rename it to disambiguate. For example, it could be named MaxByWithTies.

@SicJG
Copy link

SicJG commented Dec 17, 2021

@clement911 sure, you right that we need renaming to keep originnally conflicting, but still useful methods available from API

My message is more about problems of existing "hiding" pattern.

I just think that hiding conflicting method for keeping code compilable and adding renamed copies for specific frameworks for keeping functionality available is for sure related, but pretty parallel topics.

So we can both set NET6_0_OR_GREATER for current MaxBy/MinBy(NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER for SkipLast/TakeLast) and add new MaxByWithTies/MinByWithTies methods.

@clement911
Copy link
Author

That would be great

@clairernovotny
Copy link
Member

I've updated the code to use the OR_GREATER defines. There was a bug in the Extras that caused those not to be defined by default.

@terrajobst
Copy link
Member

@clement911 @clairernovotny

Would it be possible to re-add them to NET 6.0 but use a different name to avoid a conflict?
Maybe MaxByAll? Or MaxByWithTies?

What do the IX ones? Assuming they include ties, then MaxByWithTies seems the most apt name to me.

@clement911
Copy link
Author

The IX ones include ties. That's why they return a List. The .NET ones return a single object.

@kmate95
Copy link

kmate95 commented Jan 13, 2022

DebugPanel.razor(140, 54): [CS0121] The call is ambiguous between the following methods or properties: 'System.Linq.EnumerableEx.TakeLast<TSource>(System.Collections.Generic.IEnumerable<TSource>, int)' and 'System.Linq.Enumerable.TakeLast<TSource>(System.Collections.Generic.IEnumerable<TSource>, int)'

@bbarry
Copy link

bbarry commented Jan 28, 2022

@SicJG This is exactly what I am seeing in an attempt to upgrade my application to .net 6 today. Should a new issue be opened here?

@SicJG
Copy link

SicJG commented Jan 28, 2022

@bbarry i don't think so. Described problem was fixed in 4c248d2 and merged to main.
We're just waiting for next release of Ix.Net, could be a patch with this fix only.
@clairernovotny do you know when it may happen?

@clairernovotny
Copy link
Member

Ix / Ix Async 6.0.1 were just published that have these fixes (and now include MinByWithTies/MaxByWithTies for those that need the original behavior).

@clement911
Copy link
Author

Thank you very much @clairernovotny

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

No branches or pull requests

6 participants