UnmanagedCallersOnly functions with bool/char parameters should be an error

[rolfbjarne]

Version Used:

• main (9 Aug 2022) from sharplab.io

Steps to Reproduce:

See:

• A sharplab repro.

Code:

using System.Runtime.InteropServices;

class App
{
    public unsafe static void Main ()
    {
        delegate* unmanaged<bool, void> i = &I;
        i(true);
    }

    [UnmanagedCallersOnly]
    static void I (bool fileRegion)
    {
    }
}

Note: char has the exact same problem.

Expected Behavior:

A compiler error:

Program.cs(12,20): error CS8894: Cannot use 'bool' as a parameter type on a method attributed with 'UnmanagedCallersOnly'.

Actual Behavior:

Compilation succeeds, but an exception is thrown at runtime:

Unhandled exception. System.InvalidProgramException: Non-blittable parameter types are invalid for UnmanagedCallersOnly methods.
   at App.Main() in /Users/rolf/test/dotnet/console2/Program.cs:line 8

This bug is very similar to #57025, where the same problem occurred, except with ref parameters instead of bool / char parameters.

[tannergooding]

This is a case where the exact behavior of bool/char is runtime/scenario dependent.

For RyuJIT, this should work with the new DisableRuntimeMarshallingAttribute.

CC. @jkotas, @AaronRobinsonMSFT

[rolfbjarne]

@tannergooding does [DisableRuntimeMarshalling] make ref parameters blittable too (#57025)?

[jkotas]

It is no different from other invalid combinations of types or interop marshalling attributes on e.g. DllImport. They are only caught at runtime. Roslyn does not flag them as errors. It is intentional design.

@tannergooding does [DisableRuntimeMarshalling] make ref parameters blittable too (#57025)?

No. ref is never blittable and requires marshalling.

[jaredpar]

Disallowing bool / char in a method, even [UnmanagedCallersOnly], seems like a pretty significant break for the compiler. Is there a justification big enough to warrant such a break? Not really seeing it in this thread.

[rolfbjarne]

Disallowing bool / char in a method, even [UnmanagedCallersOnly], seems like a pretty significant break for the compiler. Is there a justification big enough to warrant such a break? Not really seeing it in this thread.

This issue is very similar to #57025, where you said the break was acceptable: #57025 (comment)

The only difference I see is that this case is scenario dependent, since the [DisableRuntimeMarshallingAttribute] can change marshalling behavior to make the test case work - but I believe the compiler has enough information to determine if this is the case (does the assembly have the attribute or not?) and not issue an error. Another point here is that [DisableRuntimeMarshallingAttribute] has not been released yet, which means that all existing P/Invokes with such signatures are broken.

My personal take is that the compiler does not allow any other non-blittable types in the signature of a [UnmanagedCallersOnly] method, which leads me to believe that if the compiler doesn't complain about a signature, the signature is OK.

Another point is that not many people know exactly which types are blittable or not, so this is a very easy mistake to make (I didn't know, and I've written quite a few P/Invokes over the years, and neither did you guys when you implemented support for [UnmanagedCallersOnly] in csc, otherwise you'd made csc show an error for both #57025 and this issue from the very beginning).

[jaredpar]

This issue is very similar to #57025, where you said the break was acceptable: #57025 (comment)

These are very different. The number of methods that have ref parameters are far smaller than ones that have bool / char.

I believe the compiler has enough information to determine if this is the case (does the assembly have the attribute or not?

The compiler does not process this attribute and has no behavior associated with it.

Closing this as there does not seem to be concensus from runtime team that this should be an error. Will reconsider if that conseus happens.

[tannergooding]

Just noting I think the behavior is fine "as is" today. Users can make it work and it will trivially fail at runtime if the code gets used.

The runtime could potentially improve the exception message to call out DisableRuntimeMarshallingAttribute as a possible fix if desired (CC. @jkoritzinsky)

[AaronRobinsonMSFT]

The runtime could potentially improve the exception message to call out DisableRuntimeMarshallingAttribute as a possible fix if desired (CC. @jkoritzinsky)

This does make sense.

@rolfbjarne Would updating the error message be a reasonable compromise?