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

Change IComparer interface argument so that it accepts nulls. #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidroth
Copy link

@davidroth davidroth commented Nov 13, 2024

Currently ISemVersionComparer and its concrete implementations implements IComparer<SemVersion> which forces one to use the bang operator when using it with some apis like the Linq Order APIs.

Current usage example:

SemVersion?[] versions = [null, SemVersion.Parse(1, 0), null, SemVersion.Parse(2, 0)];

var ordered = versions
.OrderByDescending(x => x, SemVersion.PrecedenceComparer); // Shows CS8620: because x is SemVersion? but the comparer contract documents as SemVersion only.

To get rid of the warning, one has to use the !bang operator to make the compiler happy like so:

.OrderByDescending(x => x, SemVersion.PrecedenceComparer!);

However, I think this is unncessary as the implementation rightly supports nullable comparison.

With this change, the nullability is also documented on the exposing interface to make usage in some APIS easier without any warnings.

@WalkerCodeRanger
Copy link
Owner

WalkerCodeRanger commented Nov 27, 2024

I certainly agree that the comparer accepts null and the bang operator should not be necessary. However, the handling of nullability of generics should mean you don't have to specify T? when implementing IComparer<T> (see this stack overflow answer). I'll have to look at what is going on a little more carefully. However, I would like to put out a release addressing this. Perhaps even a patch release since this is really the behavior I expected.

Notice that the IComparer<T>.Compare is declared with the signature public int Compare (T? x, T? y);.

@davidroth
Copy link
Author

davidroth commented Nov 27, 2024

The problem is not with the IComparer<T>.Compare method, which is annotated to allow nulls as you mentioned.
The problem is that the nullability is not exposed on the implementation of the IComparer<T> and therefore it does not communicate this when flowing as arguments to other methods.

See also the dotnet runtime, where for example they also implement the StringComparer with IComparer<string?> to correctly advertise this on the implementation so that it can flow into other signatures without problems.

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

Successfully merging this pull request may close these issues.

2 participants