Code Generation for Record.CompareTo(object, IComparer) lacks type-safety checks, and casts before checking nulls

[EBrown8534]

When generating the CompareTo(object, IComparer) for a record type, there is no type-safety for casting. Instead, the CompareTo(object, IComparer) simply does a direct-cast to the record type. This means callers from C#/VB.NET to CompareTo(object, IComparer) will get an InvalidCastException if they send an object that is not the correct type.

Repro steps

F# Code:

type TestType =
    { Name : string }

C# Code:

var testType = new FsLibrary.TestType("Test");
Console.WriteLine(testType.CompareTo("string", null));

The generated ILASM for the TestType::CompareTo:

.method public hidebysig virtual final instance int32
        CompareTo(object obj,
                  class [mscorlib] System.Collections.IComparer comp) cil managed
{
  .custom instance void [mscorlib]
    System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 ) 
  // Code size       59 (0x3b)
  .maxstack  4
  .locals init ([0] class FsLibrary.TestType V_0)
  IL_0000:  ldarg.1
  IL_0001:  unbox.any FsLibrary.TestType
  IL_0006:  stloc.0
  IL_0007:  ldarg.0
  IL_0008:  ldnull
  IL_0009:  cgt.un
  IL_000b:  brfalse.s IL_002c
  IL_000d:  ldarg.1
  IL_000e:  unbox.any FsLibrary.TestType
  IL_0013:  ldnull
  IL_0014:  cgt.un
  IL_0016:  brfalse.s IL_002a
  IL_0018:  ldarg.0
  IL_0019:  ldfld      string FsLibrary.TestType::Name@
  IL_001e:  ldloc.0
  IL_001f:  ldfld      string FsLibrary.TestType::Name@
  IL_0024:  call int32 [mscorlib]System.String::CompareOrdinal(string,
                                                                     string)
  IL_0029:  ret
  IL_002a:  ldc.i4.1
  IL_002b:  ret
  IL_002c:  ldarg.1
  IL_002d:  unbox.any FsLibrary.TestType
  IL_0032:  ldnull
  IL_0033:  cgt.un
  IL_0035:  brfalse.s IL_0039
  IL_0037:  ldc.i4.m1
  IL_0038:  ret
  IL_0039:  ldc.i4.0
  IL_003a:  ret
} // end of method TestType::CompareTo

The obvious problem here is the unconditional unboxing of the argument to the FsLibrary.TestType. There is no attempt made to test the type for compatibility.

Additionally, this unboxing is always done, is done several times, and is not saved until the branch for which it is needed (IL_001e).

This can be more easily seen on sharplab.io:

https://sharplab.io/#v2:DYLgZgzgPgLgngBwKYAIAqSIzY1BeAWACgVSUBvFAOQEMBbVEFLAJwEsA7AcxQF9igA=

The generated C# for CompareTo(object, IComparer) is:

[CompilerGenerated]
public sealed override int CompareTo(object obj, IComparer comp)
{
    TestType testType = (TestType)obj;
    if (this != null)
    {
        if ((TestType)obj != null)
        {
            return string.CompareOrdinal(Name@, testType.Name@);
        }
        return 1;
    }
    if ((TestType)obj != null)
    {
        return -1;
    }
    return 0;
}

The culprit here can be easily identified as TestType testType = (TestType)obj, as well as the location of that cast, the fact that we cast obj to TestType in two other locations (rather than using the previously-casted version).

Expected behavior

The CompareTo(object, IComparer) should be generated with a type-check, then if the type is compatible it should be casted. It should also do the cast only when necessary, rather than as the first step.

Ideally, a more-proper C# version would look like:

[CompilerGenerated]
public sealed override int CompareTo(object obj, IComparer comp)
{
    if (this != null)
    {
        if (this is TestType) // Test for type here, can also use `as`
        {
            TestType testType = (TestType)obj; // Move this into the block where it's absolutely necessary
            if (testType != null)
            {
                return string.CompareOrdinal(Name@, testType.Name@);
            }
            return 1;
        }
    }
    if (obj != null) // No cast needed here, but not actively harmful to keep
    {
        return -1;
    }
    return 0;
}

Actual behavior

The generated CompareTo(object, IComparer) is not properly compatible with other .NET languages.

Known workarounds

None. The C# version must implement it's own comparison.

Related information

• Windows 10 Pro for Workstations (10.0.16299.492)
• Public Visual Studio 2017 Community
• .NET 4.7.2
• Visual Studio 2017 Community 15.7.2
• Low/Medium, for someone like me who does a lot of C# <-> F# Interop it's far more severe

[TIHan]

I think this might be an improvement, it seems reasonable; but I don't know the full intentions of why we do a direct cast rather than check.

[zpodlovics]

I guess these code assume the F# usage only (somewhat reasonable for F# records): "If a type is defined in F#, null is not permitted as a regular value unless the AllowNullLiteral attribute is applied to the type. If a type is defined in some other .NET language, null is a possible value, and when you are interoperating with such types, your F# code might encounter null values." [1]

Also value types (struct record) will not require null checks as null is not a valid value for a value type (unless represented as Nullable<'T> but that's a different story)

In the future could these non-nullable requirement also be encoded in a more interoperable way too eg.: as a modreq?

[1] https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2012/dd233197(v=vs.110)

[EBrown8534]

@TIHan That may be the case, but it's also worth noting that the end result of this code is the cast being performed 3 times in the function, when one will suffice. Granted, casting is usually quick, but there's no reason to do it that often, especially when I assume this function is used a lot by the compiler.

Looking at the docs, it looks like even the Microsoft support docs do a direct cast, not sure why, but it still feels wrong:

https://support.microsoft.com/en-us/help/320727/how-to-use-the-icomparable-and-icomparer-interfaces-in-visual-c

The role of IComparable is to provide a method of comparing two objects of a particular type. This is necessary if you want to provide any ordering capability for your object. Think of IComparable as providing a default sort order for your objects. For example, if you have an array of objects of your type, and you call the Sort method on that array, IComparable provides the comparison of objects during the sort. When you implement the IComparable interface, you must implement the CompareTo method, as follows:

// Implement IComparable CompareTo method - provide default sort order.
int IComparable.CompareTo(object obj)
{
   car c=(car)obj;
   return String.Compare(this.make,c.make);
}

Even their example has no type-safety check. I wonder if that's because IComparable is typically only used by collections.

I still don't like it, but I like it less now that I see that the support docs use it.

[reinux]

Is there a chance this can be addressed? You don't even need to call from another language for this to be a problem:

type Animal =
  abstract member Name: string
  inherit System.IComparable
  
type Dog =
  { numWoofs: int }
  interface Animal with
    member this.Name = "dogbert"
    
type Cat =
  { numMeows: int }
  interface Animal with
    member this.Name = "catsy"
    
let dog = { numWoofs = 3 }
let cat = { numMeows = 5 }

let map = Map<Animal, string> [ dog, "woof"; cat, "bahhh" ] 

Results in:

System.InvalidCastException: Unable to cast object of type 'Dog' to type 'Cat'.

Granted, the key of a map is usually something simple, but I don't think this is necessarily all that uncomon a use case.