Fix a bug where ToHashSet() is implemented in .Net 4.7.1+ System.Linq, causing failure to compile

[b9chris]

trafficstars

Fix a bug where: using System.Linq; using MoreLINQ;

Causes calls to .ToHashSet() to fail to compile, because an identical implementation exists in .Net 4.7.1+

[Orace]

https://github.com/morelinq/MoreLINQ/blob/b839848e83597e0c7698290ac416354c27160003/MoreLinq/ToHashSet.cs#L17-L19

This test is at compile time, not at use time. You can compile with 4.7 and still have the issue.

Maybe you can look what as been done in similar case (like Zip).

Anyway to fix it at user level, the README file is pretty clear about it:

https://github.com/morelinq/MoreLINQ#usage

[b9chris]

This test is at compile time, not at use time.

Not sure what you mean by this. This flag is meant to prevent inclusion of the ToHashSet() extension method in 4.7.1+, and that's what it does. I've tested it - it fixes the issue.

You can compile with 4.7 and still have the issue.

I think you mean it was introduced in 4.7 not 4.7.1? Simple enough to change, I'll fix that.

Maybe you can look what as been done in similar case (like Zip).

Anyway to fix it at user level, the README file is pretty clear about it:

https://github.com/morelinq/MoreLINQ#usage

What's documented there is a workaround, not a fix. What I'm proposing is a fix.

[Orace]

This test is at compile time, not at use time.

Not sure what you mean by this. This flag is meant to prevent inclusion of the ToHashSet() extension method in 4.7.1+, and that's what it does. I've tested it - it fixes the issue.

You can compile with 4.7 and still have the issue.

I think you mean it was introduced in 4.7 not 4.7.1? Simple enough to change, I'll fix that.

The '#if' tests are done at compile time. MoreLinq is almost distributed via nuget package that are pre-compiled.

Actually I don't know wish version of the framework is used to build the package.

Let say that the package is built in framework 4.5 (or anything not 4.7.1, like 4.7). At compile time "NET471" will be false and the '#if' will be ignored and ToHashSet will be put in the package:

• users of framework 4.5 will find one ToHashSet method (from MoreLinq).
• user of framework 4.7.1 will find two ToHashSet method (MoreLinq and System.Linq).

Now, let say that the package is built in framework 4.7.1 At compile time "NET471" will be true and the '#if' will trigger, ToHashSet will not be put in the package:

• users of framework 4.5 will find zero ToHashSet method.
• users of framework 4.7.1 will find one ToHashSet method (System.Linq).

Hence, for nuget package distribution, nothing is fixed here.

Since the final user framework version is unknown, and because we don't want to remove any functionality for users with a framework version prior to 4.7.1, and because we don't want a MoreLinq for 3.5, one for 4.5, one for 4.7.1, etc... a workaround is advised (using alias, or static using).

Hope it's clear enough, and sorry to no be clear enough at first try.

[b9chris]

Actually I don't know wish version of the framework is used to build the package.

I see, I think you just need to read up on multi-framework targeting. More than one framework is used in multi-target frameworks. MoreLINQ is multi-target. So, the answer is - multiple frameworks are used, and for the 4.7.1 build in this case, the flag is true. For others, like the 4.5.1 build, the flag is false.

It sounds like you've declared this won't work without actually testing anything, so I'd prefer you try it before issuing further objections.

https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting

[viceroypenguin]

I have wondered why this solution isn't used before. I agree with @b9chris, MoreLINQ should be able to block off certain API based on which version it is compiled with. This will make the provided work-around unnecessary.

This could also be applied to .Append() and .Prepend(), provided in the latest .net core offerings.

[Orace]

I have wondered why this solution isn't used before

The cons I see :

• Hard to maintain.
• Many more target frameworks.
• Need a quick new release for any method integrated in a version of a framework (hopefully method signatures matched for now).

This will make the provided work-around unnecessary.

The workaround is still useful in case of name collision with a third-party library.

[viceroypenguin]

On the other hand, the workaround is extra work in the case where the consumer is targeting multiple frameworks. As it is, I have multiple places where I have to include:

#if NETFX
using static MoreLinq.Extensions.AppendExtension;
#endif

If MoreLINQ handled this directly, it would be a simple using MoreLinq; in those cases.

However, I do get your point about extra work in the case of a new .net core version; they are continually adding new Enumerable features that overlap MoreLINQ.

[viceroypenguin]

I did some research over the last couple weeks, trying to figure out if using framework versioning would be viable. @Orace, I read what you wrote, and from a compile-time perspective, that wouldn't be an issue. The Nuget package manager automatically binds at compile-time to the appropriate .dll, even though multiple copies are provided in the .nupkg.

However, I think I came across a hard block at runtime. Here's a scenario that would be non-functional:

• Library A builds in .NET 4.5.1 against MoreLinq, using the fx 4.5.1 version of the library.
• They use enumerable.Append(value), which the compiler translates to MoreEnumerable.Append(enumerable, value).
• Program B is built against .NET Core 3.0
• Program B imports MoreLinq for .NET Core 3.0
• Program B references Library A.

Runtime Exception when Library A cannot find MoreEnumerable.Append() because it is no longer provided by the MoreLinq library loaded by the program. This is not a versioning issue (since both could be built against the same MoreLinq 3.4.0, just against different FX versions).

Since there is no way to offer a type-forward for just a single function, there's no way to tell the CLR that when looking for MoreEnumerable.Append(), replace it w/ Enumerable.Append().

The MoreLinq team could decide to be aware of this issue, and pursue this path anyway, and provide documentation to potential users about how to address it.

However, since they are already disinclined to pursue this, I would suggest that the current workaround is the most viable solution.

@b9chris - I had high hopes for doing this too!

[viceroypenguin]

@b9chris Please check out the morelinq.temp package as a proof of concept for a solution for this issue, built off of https://github.com/viceroypenguin/MoreLINQ/tree/multi-framework-support branch. If you have any comments, put them in #749.

[atifaziz]

This has been superseded by PR #945.