dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.04k stars 4.03k forks source link

Overloading by `nint` and `IntPtr` in `op_Implicit` #70953

Open hamarb123 opened 11 months ago

hamarb123 commented 11 months ago

Version Used: .NET SDK 8.0.100 (and older also, as I wrote this on accident ages ago), targetting net5.0 or net6.0 (doesn't occur on netcoreapp3.1 nor net7.0)

Steps to Reproduce:

public struct EGLsizeiANDROID
{
    public EGLsizeiANDROID(nint a) { }
    public static implicit operator EGLsizeiANDROID(IntPtr a) => new EGLsizeiANDROID(a);
    public static implicit operator EGLsizeiANDROID(nint a) => new EGLsizeiANDROID(a);
}

Expected Behavior:

Compilation fails, since IntPtr and nint are the same type, therefore you cannot overload by them.

Actual Behavior:

Compilation succeeds and produces this invalid IL (since you cannot overload by signature in IL, unless one is compilercontrolled):

image

Note specifically the 2 op_Implicit methods, which have the same signature.

Note that a compile-time error is present on both netcoreapp3.1 and net7.0, so this is potentially related to https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/native-integers, which was introduced in .NET 5.0, and effectively replaced by https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-11.0/numeric-intptr in .NET 7.0 with the introduction of RuntimeFeature.NumericIntPtr.

The same thing also occurs with nuint and UIntPtr from my testing. There are also be other positions where overloading is erroneously allowed, such as by return of op_Implicit, and for op_Explicit also (I've not checked all positions, these are just some I've found).

Note in the specification for this feature, it actually says this code should be illegal: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/native-integers#overriding-hiding-and-implementing

333fred commented 11 months ago

@jcouv can you take a look?

jcouv commented 10 months ago

I wasn't able to repro (see tests below). @hamarb123 Could you check what language version was specified? (adding #error version in a source file and looking at the diagnostic is one way to check)

        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70953")]
        public void DuplicateConversions()
        {
            var source = """
using System;
public struct S
{
    public S(nint a) { }
    public static implicit operator S(IntPtr a) => new S(a);
    public static implicit operator S(nint a) => new S(a);
}
""";

            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net80, parseOptions: TestOptions.Regular11);
            Assert.True(comp.Assembly.RuntimeSupportsNumericIntPtr);
            Assert.True(comp.SupportsRuntimeCapability(RuntimeCapability.NumericIntPtr));
            comp.VerifyDiagnostics(
                // (6,37): error CS0557: Duplicate user-defined conversion in type 'S'
                //     public static implicit operator S(nint a) => new S(a);
                Diagnostic(ErrorCode.ERR_DuplicateConversionInClass, "S").WithArguments("S").WithLocation(6, 37)
                );
        }

        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70953")]
        public void DuplicateMethodOverloads()
        {
            var source = """
using System;
public struct S
{
    public S(nint a) { }
    public static S M(IntPtr a) => new S(a);
    public static S M(nint a) => new S(a);
}
""";

            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net80, parseOptions: TestOptions.Regular11);
            Assert.True(comp.Assembly.RuntimeSupportsNumericIntPtr);
            Assert.True(comp.SupportsRuntimeCapability(RuntimeCapability.NumericIntPtr));
            comp.VerifyDiagnostics(
                // (6,21): error CS0111: Type 'S' already defines a member called 'M' with the same parameter types
                //     public static S M(nint a) => new S(a);
                Diagnostic(ErrorCode.ERR_MemberAlreadyExists, "M").WithArguments("M", "S").WithLocation(6, 21)
                );
        }
hamarb123 commented 10 months ago

On phone: only an issue with net5.0 and net6.0 (& platform specific ones presumably) - see original issue description, but it looks like you have it set to net8.0.

jcouv commented 10 months ago

Thanks for the clarification. I'm able to repro that behavior when targeting .NET 5 (see test below). When the native integer types were first introduced (in C# 9), nint and System.IntPtr were considered different types by the language, but as you pointed out in the spec overloads would not be allowed to just differ between nint and System.IntPtr. So that's a bug in C# 9. Since C# 11 however, we treat those types as identical and the new overload behavior is correct. I would consider this issue as fixed in newer SDK and language version, although the buggy behavior is still accessible with new compiler by targeting older TFM and using older language version.

        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70953")]
        public void DuplicateConversions()
        {
            var source = """
using System;
public struct S
{
    public S(nint a) { }
    public static implicit operator S(IntPtr a) => new S(a);
    public static implicit operator S(nint a) => new S(a);
}
""";

            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net50, parseOptions: TestOptions.Regular9);
            Assert.False(comp.Assembly.RuntimeSupportsNumericIntPtr);
            Assert.False(comp.SupportsRuntimeCapability(RuntimeCapability.NumericIntPtr));
            comp.VerifyDiagnostics(); // incorrect: we expect an error instead
        }
hamarb123 commented 10 months ago

And net6.0 (with C# 10 presumably), which is still officially supported for another ~11 months.