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

The compiler does not check that keys returned by the key selector passed to OrderBy are comparable #92691

Closed
JesHansen opened this issue Sep 27, 2023 · 5 comments
Labels
area-System.Runtime needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Comments

@JesHansen
Copy link

Description

When you have a collection of things (an IEnumerable<Foo>, say) and attempt to order it by some key:

var orderedFoos = foos.OrderBy(f => f.Key)

then this will compile, even in the case where the Key value does not implement IComparable.
This leads to a runtime exception: "Unhandled exception. System.InvalidOperationException: Failed to compare two elements in the array."

It seems like the compiler ought to have enough information about the keys produced by the key selector to realize that they do not implement IComparable, and report this as a compilation error.

Reproduction Steps

The following code:

public class NotComparable
{
    public int InternalKey { get; }

    public NotComparable(int internalKey)
    {
        InternalKey = internalKey;
    }
}

public class Foo
{
    public NotComparable Key { get; }

    public Foo(NotComparable key)
    {
        Key = key;
    }
}

internal class Program
{
    public static void Main(string[] args)
    {
        var example1 = new List<Foo> {new(new(3)), new(new(-5))};
        foreach (var item in example1.OrderBy(foo => foo.Key))
        {
            Console.WriteLine(item);
        }
    }
}

compiles, but thows an System.InvalidOperationException at runtime.

Expected behavior

The compiler should have reported a compilation error, as it is unable to sort the list of Foos

Actual behavior

The code thows an System.InvalidOperationException at runtime.

Regression?

I do not believe this to be a regression

Known Workarounds

If you have access to the source code of the keys, you can have them implement IComparable. If not, then you are probably forced to change the type of the keys to some wrapper type.

Configuration

.NET7 console application running on Windows 11, x64.

I do not believe the behavior is configuration specific.

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 27, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 27, 2023
@karakasa
Copy link
Contributor

karakasa commented Sep 27, 2023

If not, then you are probably forced to change the type of the keys to some wrapper type.

You may use the overloading version that accepts an IComparer.

I agree a source analyzer can benefit this scenario.

@ploeh
Copy link
Contributor

ploeh commented Sep 27, 2023

I agree a source analyzer can benefit this scenario.

This could actually be fixed at the type level, by adding a generic constraint that TKey : IComparable, although this is unlikely to happen because it would constitute a breaking change.

Although one wonders how much of a breaking change it is when the code would throw a run-time exception anyway... but my imagination isn't worse than I can think of some pathological examples where code currently compiles and no exceptions are thrown, even if the key doesn't implement IComparable... 🙄

@karakasa
Copy link
Contributor

I can think of some pathological examples where code currently compiles and no exceptions are thrown, even if the key doesn't implement IComparable... 🙄

just saying, the comparer can compare a null object to another non-null object, regardless of it implements IComparable or not.

@stephentoub
Copy link
Member

stephentoub commented Sep 28, 2023

The compiler can't always know. Imagine you tweaked your example by adding this type:

public class DerivedNotComparable : NotComparable, IComparable
{
    public DerivedNotComparable(int key) : base(key) { }

    public int CompareTo(object? other) => 
        Comparer<int?>.Default.Compare(InternalKey, (other as NotComparable)?.InternalKey);
}

and then just changed this:

var example1 = new List<Foo> {new(new(3)), new(new(-5))};

to this:

var example1 = new List<Foo> { new(new DerivedNotComparable(3)), new(new DerivedNotComparable(-5)) };

From the compiler's perspective, the type of example1 is still List<Foo>, and the type returned by the delegate passed to OrderBy is still NotComparable, but it will now run successfully, because the actual objects returned from the delegate do in fact implement IComparable even though NotComparable does not.

@ghost
Copy link

ghost commented Sep 28, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

When you have a collection of things (an IEnumerable<Foo>, say) and attempt to order it by some key:

var orderedFoos = foos.OrderBy(f => f.Key)

then this will compile, even in the case where the Key value does not implement IComparable.
This leads to a runtime exception: "Unhandled exception. System.InvalidOperationException: Failed to compare two elements in the array."

It seems like the compiler ought to have enough information about the keys produced by the key selector to realize that they do not implement IComparable, and report this as a compilation error.

Reproduction Steps

The following code:

public class NotComparable
{
    public int InternalKey { get; }

    public NotComparable(int internalKey)
    {
        InternalKey = internalKey;
    }
}

public class Foo
{
    public NotComparable Key { get; }

    public Foo(NotComparable key)
    {
        Key = key;
    }
}

internal class Program
{
    public static void Main(string[] args)
    {
        var example1 = new List<Foo> {new(new(3)), new(new(-5))};
        foreach (var item in example1.OrderBy(foo => foo.Key))
        {
            Console.WriteLine(item);
        }
    }
}

compiles, but thows an System.InvalidOperationException at runtime.

Expected behavior

The compiler should have reported a compilation error, as it is unable to sort the list of Foos

Actual behavior

The code thows an System.InvalidOperationException at runtime.

Regression?

I do not believe this to be a regression

Known Workarounds

If you have access to the source code of the keys, you can have them implement IComparable. If not, then you are probably forced to change the type of the keys to some wrapper type.

Configuration

.NET7 console application running on Windows 11, x64.

I do not believe the behavior is configuration specific.

Other information

No response

Author: JesHansen
Assignees: -
Labels:

area-System.Runtime, untriaged, needs-area-label

Milestone: -

@stephentoub stephentoub closed this as not planned Won't fix, can't repro, duplicate, stale Sep 29, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 29, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

No branches or pull requests

5 participants