BOOL/BOOLEAN templates are incorrect

[AaronRobinsonMSFT]

The BOOL and BOOLEAN template constructors taking a C# bool are invalid.

CsWin32/src/Microsoft.Windows.CsWin32/templates/BOOL.cs

Line 3 in ab63c9a

internal unsafe BOOL(bool value) => this.Value = *(sbyte*)&value;

CsWin32/src/Microsoft.Windows.CsWin32/templates/BOOLEAN.cs

Line 3 in ab63c9a

internal unsafe BOOLEAN(bool value) => this.Value = *(byte*)&value;

The BOOL template should be, the BOOLEAN is similar.

internal unsafe BOOL(bool value) => this.Value = value ? 1 : 0;

The notion of simply blitting the lowest bits of a bool instance is undefined. It also makes this usage troublesome if a user defines SkipLocalsInitAttribute, which with the current implementation leave the upper bits of Value in an undefined state thus making a .NET false possibly be non-zero.

[JeremyKuhne]

making a .NET false possibly be non-zero.

Wow, that's a good caveat to know. Is that just with the default value, or even if you then assign false?

[jnm2]

Also, comparisons in different contexts don't operate the same on bool values that aren't identical to true or false: dotnet/roslyn#24865 (comment)

[AaronRobinsonMSFT]

making a .NET false possibly be non-zero.

Wow, that's a good caveat to know. Is that just with the default value, or even if you then assign false?

With the current implementation, even if a .NET false value was set, it would only set the least significant byte in the Value field. This means that if the higher 3 bytes are non-zero due to not being explicitly initialized to zero then only the lowest byte would be set to the assumed value of 0.

[elachlan]

This documentation comment is in the winforms code base.
https://github.com/dotnet/winforms/blob/d46ad2e8dc76248739d9ae22b28b399a6a6b299e/src/System.Windows.Forms.Primitives/src/Interop/Interop.BOOL.cs#L9-L17

Does that make this implicit operator incorrect as well?

CsWin32/src/Microsoft.Windows.CsWin32/templates/BOOL.cs

Lines 4 to 8 in ab63c9a

	public static unsafe implicit operator bool(BOOL value)
	{
	sbyte v = checked((sbyte)value.Value);
	return *(bool*)&v;
	}

[AaronRobinsonMSFT]

Does that make this implicit operator incorrect as well?

@elachlan Good lord, I didn't even pay attention to that. Yes, that is very suspect. The Windows BOOL is typically defined to be 0 is false and true is non-zero. There are some Win32 APIs that do questionable things with checking flags - blech.

[elachlan]

@AaronRobinsonMSFT , do we just go with this?

public static unsafe implicit operator bool(BOOL value)
{
return this.value != 0 ? true : false;
}

[AaronRobinsonMSFT]

@AaronRobinsonMSFT , do we just go with this?

Sure or, even simpler - return this.value != 0;

[elachlan]

@AaronRobinsonMSFT the BOOLEAN class has a byte value backing field.

CsWin32/src/Microsoft.Windows.CsWin32/templates/BOOLEAN.cs

Lines 3 to 8 in ab63c9a

	internal unsafe BOOLEAN(bool value) => this.Value = *(byte*)&value;
	public static unsafe implicit operator bool(BOOLEAN value)
	{
	byte v = checked((byte)value.Value);
	return *(bool*)&v;
	}

The tests seem strange:
https://github.com/microsoft/CsWin32/blob/ab63c9ad139a77a43ad331f7334d38799d6125e2/test/GenerationSandbox.Tests/BooleanTests.cs

It looks like BOOLEAN is supposed to not equal when different values are supplied:

CsWin32/test/GenerationSandbox.Tests/BooleanTests.cs

Lines 41 to 48 in ab63c9a

	[Fact]
	public void BOOLEANEqualsComparesExactValue()
	{
	BOOLEAN b1 = new BOOLEAN(1);
	BOOLEAN b2 = new BOOLEAN(2);
	Assert.Equal(b1, b1);
	Assert.NotEqual(b1, b2);
	}

But then, when comparing a native bool value. It is supposed to equal? I think this allows the byte value to be passed to a native bool and then get the value back again.

CsWin32/test/GenerationSandbox.Tests/BooleanTests.cs

Lines 19 to 39 in ab63c9a

	[Theory]
	[InlineData(3)]
	[InlineData(0xff)]
	public void NotLossyConversionBetweenBoolAndBOOLEAN(byte ordinal)
	{
	BOOLEAN nativeBool = new BOOLEAN(ordinal);
	bool managedBool = nativeBool;
	BOOLEAN roundtrippedNativeBool = managedBool;
	Assert.Equal(nativeBool, roundtrippedNativeBool);
	}
	[Theory]
	[InlineData(3)]
	[InlineData(0xff)]
	public void NotLossyConversionBetweenBoolAndBOOLEAN_Ctors(byte ordinal)
	{
	BOOLEAN nativeBool = new BOOLEAN(ordinal);
	bool managedBool = nativeBool;
	BOOLEAN roundtrippedNativeBool = new BOOLEAN(managedBool);
	Assert.Equal(nativeBool, roundtrippedNativeBool);
	}

So the changes to BOOLEAN should not be made according to the tests.

[elachlan]

I think the same tests exist for BOOL as well.

[elachlan]

The documentation on the datatypes doesn't say anything about preserving the underlying values.
https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types

@AArnott have you got any guidance?

[elachlan]

CC: @RussKie

[AaronRobinsonMSFT]

But then, when comparing a native bool value. It is supposed to equal? I think this allows the byte value to be passed to a native bool and then get the value back again.

That is an odd test. All Win32 boolean based types normalize to a "true" or "false" state. As long as we are in managed code the actual value does matter beyond 0 and non-zero. All non-zero are equal from a boolean type's perspective.

[elachlan]

I think its because some Win32APIs return BOOL, but indicate -1 for an error state.
https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmessage
#339
#362
#363

Thank git and its version history for me being able to track that down.

[AaronRobinsonMSFT]

I think its because some Win32APIs return BOOL, but indicate -1 for an error state. https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmessage #339 #362 #363

Thank git and its version history for me being able to track that down.

This is news to me - thanks for tracking it down. That code/tests likely warrant a comment as it isn't intuitive and is likely limited to a narrow set of APIs.

This doesn't change the marshal to unmanaged logic though. Basically, going from C# bool to Win32 BOOL, the correct logic is BOOL res = value ? 1 : 0;

[elachlan]

@AArnott, can you explain why BOOL/BOOLEAN to bool conversion is not lossy (keeping the underlying value)?

[AArnott]

Because of what you found in #624 (comment). We allow implicit conversions across the bool types, and implicit conversions should not be lossy. So either we make BOOL and BOOLEAN convert to bool only through explicit conversions (which would be a terrible experience) or we can preserve the data so that -1 or other hidden things that are totally allowed in native code don't get lost in translation.

It sounds like we don't have a bug at this point because we in fact don't yet set SkipLocalsInit, but when we do for #595, we should revisit this and find a safe way to update this code.

[jnm2]

Instead of lossy implicit conversions to bool, what if you instead have an explicit conversion to bool but also declare operator true and operator false?

[AaronRobinsonMSFT]

It sounds like we don't have a bug at this point because we in fact don't yet set SkipLocalsInit

Aren't these types injected into the user's project as source?

[AArnott]

Aren't these types injected into the user's project as source?

Yes. Are you calling out that SkipLocalsInit can be applied assembly-wide, thus the generated code can misbehave?

what if you instead have an explicit conversion to bool but also declare operator true and operator false?

That's a very interesting idea. I didn't know about operator true. Thanks for sharing.

I haven't deeply studied the problem yet, but in passing it seems like the problem is a risk that we read more bytes than are guaranteed to be initialized. Isn't that solvable without changing the fundamental idea of preserving the underlying value (the part of it that's guaranteed to be initialized)?

[AaronRobinsonMSFT]

Yes. Are you calling out that SkipLocalsInit can be applied assembly-wide, thus the generated code can misbehave?

Yes. I can accept that data coming from an unmanaged context may not want to be normalized but going in the other direction the code should really just be this.value = v ? 1 : 0;

[AArnott]

What value is there in not being lossy in one conversion but being lossy in the other? Since C# makes extracting the non-binary value from a bool very difficult, it seems to me the value in being non-lossy is primarily in being able to jump across BOOL -> bool -> BOOL and having the original value.

[AaronRobinsonMSFT]

BOOL -> bool -> BOOL and having the original value.

I don't see how that would work in practice. It would mean the bool, which is always passed by value, maintains the interop binary value which isn't guarantee nor a good idea in practice. It is basically assuming the runtime will define its bool ABI to be what the interop layer decided is appropriate.

[elachlan]

@stephentoub, sorry to snipe you like this. Could you please chime in on how we should be handling interop BOOL/BOOLEAN to bool conversion or blitting? Should bool ever have an underlying value other than true or false?

[AArnott]

It is basically assuming the runtime will define its bool ABI to be what the interop layer decided is appropriate.

The runtime cannot change the memory layout of bool at this point, can it? We expect it to be 1 byte long, and we may copy 1 byte into that memory. C# emits IL for bool conditions that treat any non-zero value as truthy, just as C does. I'm struggling to see how it's problematic then to copy the value from BOOL or BOOLEAN into bool in an implicit conversion so that another implicit conversion can change it back. And this isn't me just trying to be clever... I had to work this out to resolve another issue (linked above).

[elachlan]

Do we just initialize the bytes ourselves when doing the conversion?

[JeremyKuhne]

@AArnott this checked conversion is a hard blocker for WinForms. We're getting back BOOL values from Windows APIs that overflow. I've tried providing true and false operators, but they don't take precedence unfortunately. :(

It's way too risky for us to have checked operations with BOOL. We're probably going to have to shelve working on our porting exercise until we can get this resolved.