Runtime: Specify marshalling for booleans in P/Invokes

[elinor-fung]

By default, a boolean will be marshalled using UnmanagedType.Bool (4 bytes - matches Win32 BOOL). Developers can often be bitten by this when marshalling to a 1-byte representation of a boolean (e.g. the C++ bool). Flagging instances where marshalling is not specified could help avoid such issues.

```C#
[DllImport("MyLibrary")]
private static extern void Foo(bool b, MyStruct s); // Trigger on argument

[DllImport("MyLibrary")]
private static extern bool Bar(); // Trigger on return type

struct MyStruct
{
bool B; // Trigger on field of marshalled struct
}
```

Category: Interoperability

cc @terrajobst @stephentoub @AaronRobinsonMSFT @jkoritzinsky

I think this would basically be reviving CA1414. Anyone know if we explicitly didn't want to port that for some reason?

[stephentoub]

FWIW, we've generally avoided explicitly specifying the UnmanagedType when we're using the default. I don't think we'd want to turn this on in dotnet/runtime.

[tannergooding]

Do we know how many places are using bool in dotnet/runtime? I know at least a couple places have been changed from bool to int so the signature is blittable...

[AaronRobinsonMSFT]

I know at least a couple places have been changed from bool to int so the signature is blittable...

To me, this is the best reason for using int instead of bool when the P/Invoke isn't exposed publicly.

@elinor-fung Perhaps this would be a valuable default analyzer for a "performance" group?

[elinor-fung]

There is a 'Performance` category.

@AaronRobinsonMSFT are you suggesting something like checking how a bool is being marshalled and suggesting switching to the blittable equivalent? e.g. UnmanagedType.I1 -> byte, UnmanagedType.VariantBool -> short, UnmanagedType.Bool / default -> int?

[stephentoub]

To me, this is the best reason for using int instead of bool when the P/Invoke isn't exposed publicly.

So the analyzer would only fire if changing that bool to int would avoid needing a marshalling stub? i.e. nothing else in the signature would require marshalling?

[AaronRobinsonMSFT]

@stephentoub Yes. This would be a cheap optimization. It does require the analyzer to know how to compute blittability for all types.

[AaronRobinsonMSFT]

are you suggesting something like checking how a bool is being marshalled and suggesting switching to the blittable equivalent? e.g. UnmanagedType.I1 -> byte, UnmanagedType.VariantBool -> short, UnmanagedType.Bool / default -> int?

@elinor-fung Yes.

[stephentoub]

Yes. This would be a cheap optimization. It does require the analyzer to know how to compute blittability for all types.

Then isn't the analyzer less about bool and more about helping developers via an analyzer/fixer to rewrite the pinvokes to not need a stub? It's not just bool, e.g. rewriting string marshalling, rewriting ref pinning, rewriting SafeHandle-marshalling, etc? Seems odd to me to single out bool here. Do we have a lot of examples in dotnet/runtime where a bool is all that stands in the way between any implicit marshalling being required and no implicit marshalling being required? If yes, makes sense I guess (and we should fix those :-). If no, I'm not sure why we'd focus on that.

[AaronRobinsonMSFT]

Then isn't the analyzer less about bool and more about helping developers via an analyzer/fixer to rewrite the pinvokes to not need a stub?

Yes. I realized this suggestion is really "Your P/Invoke is so close being blittable. Change X and Y and perf!". The usability/intent for bool that @elinor-fung is attempting to address here is the focus. Got side tracked with mention of inlined pinvokes.

[stephentoub]

I think this would basically be reviving CA1414.

It's already stubbed out here:
https://github.com/dotnet/roslyn-analyzers/blob/f15404b312295e7cee16fb40b4c8d3f91f10f087/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/MarkBooleanPInvokeArgumentsWithMarshalAs.cs

For the most part, my understanding is all of the existing fxcop analyzers are being ported as time and desire allows, to enable folks still using fxcop to eventually drop it. But most of those analyzers are / will be off by default moving forward. @mavasani can probably shed more light.

[mavasani]

Yep, @stephentoub is correct. https://github.com/dotnet/roslyn-analyzers/issues/430 already tracks porting CA1414, we haven't received any customer feedback on importance of this rule, so we haven't invested in porting it.

[elinor-fung]

Closing this issue, since it is essentially porting CA1414, which is already tracked through dotnet/roslyn-analyzers#430.