dotnet / dotnet/fsharp

Code Generation for Record.CompareTo(object) lacks type-safety checks

Open
#5,379 0 comments 0 reactions 0 assignees View on GitHub
Area-Compiler-HashCompare Feature Improvement
Dominant language
F#
Stars
4.3k
Forks
876
Avg merge
4d 11h
Merged PRs (30d)
131

Description

When generating the `CompareTo(object)` for a record type, there is no type-safety for casting. Instead, the `CompareTo(object)` simply does a direct-cast to the record type. This means callers from C#/VB.NET to `CompareTo(object)` 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("Test 2")); // Throws `System.InvalidCastException`

The generated ILASM for the `TestType::CompareTo`:

.method public hidebysig virtual final instance int32
CompareTo(object obj) cil managed
{
.custom instance void [mscorlib]
System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = ( 01 00 00 00 )
// Code size 13 (0xd)
.maxstack 8
IL_0000: ldarg.0
IL_0001: ldarg.1
IL_0002: unbox.any FsLibrary.TestType
IL_0007: callvirt instance int32 FsLibrary.TestType::CompareTo(class FsLibrary.TestType)
IL_000c: 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.

This can be more easily seen on `sharplab.io`:

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

The generated C# for `CompareTo(object)` is:

[CompilerGenerated]
public sealed override int CompareTo(object obj)
{
return CompareTo((TestType)obj);
}

The culprit here can be easily identified as `(TestType)obj`.

#### Expected behavior

The `CompareTo(object)` should be generated with a type-check, then if the type is compatible it should be casted.

#### Actual behavior

The generated `CompareTo(object)` 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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.