Missing type casts when using bitfields with remapped types

[Exanite]

This is an edge case that occurs when using remappings with bitfields.

I discovered this issue when attempting to map Vulkan Flags enums to FlagBits in order to have ClangSharpPInvokeGenerator output FlagBits as the property type rather than uint.

One example of this issue happening is when generating the struct, VkAccelerationStructureInstanceKHR, with the mapping, VkGeometryInstanceFlagsKHR -> VkGeometryInstanceFlagBitsKHR. (Search for the struct name in vulkan_core.h)

This is a simplified example that I implemented as a test case:

typedef enum FlagBits {
    Member = 0x7FFFFFFF
} FlagBits;
typedef unsigned int Flags;

typedef struct Bitfield {
    uint bits : 8;
    Flags flags : 8;
} Bitfield;

This outputs the following, without the proper casts. This causes a compile error.
The expected is that the get return value is cast to FlagBits and value is cast to uint.

public uint _bitfield;

[NativeTypeName("Flags : 8")]
public FlagBits flags
{
    readonly get
    {
        return (_bitfield >> 8) & 0xFFu;
    }

    set
    {
        _bitfield = (_bitfield & ~(0xFFu << 8)) | ((value & 0xFFu) << 8);
    }
}

---

You can find the full test cases here in my fork: https://github.com/Exanite/ClangSharp/blob/d3a1dc1a90b85ba465f15b72f60992cbe4c28be7/tests/ClangSharp.PInvokeGenerator.UnitTests/CTest.cs#L456

Note that BitfieldEnumPropertyTypeCastTest already passes.
BitfieldEnumPropertyTypeCastWithRemappingTest is the test that fails.

This shows that ClangSharp already handles this case to some extent and that it is the remapping that causes issues.

---

If this is a case that ClangSharp should handle, I'm interested in opening a PR for fixing this issue.

[Exanite]

When looking through the codebase, this line of code caught my attention:

ClangSharp/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs

Line 2725 in 4d4e062

var isRemappedToSelf = _config.RemappedNames.TryGetValue(typeName, out var remappedTypeName) && typeName.Equals(remappedTypeName, StringComparison.Ordinal);

---

For reference, this is the full section of code:

ClangSharp/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs

Lines 2724 to 2731 in 4d4e062

	var isSmallType = currentSize < 4;
	var isRemappedToSelf = _config.RemappedNames.TryGetValue(typeName, out var remappedTypeName) && typeName.Equals(remappedTypeName, StringComparison.Ordinal);
	var isTypeMismatch = type != builtinTypeBacking;
	var isUnsignedToSigned = !isTypeBackingSigned && isTypeSigned;
	var needsCast = isSmallType || isRemappedToSelf || isTypeMismatch || isUnsignedToSigned;
	var needsParenFirst = !isSmallType && isUnsignedToSigned;
	var needsParenSecond = !needsParenFirst || isRemappedToSelf;

---

This line implies that if I add a mapping, FlagBits -> FlagBits, that this mapping will force the generator to insert a cast.
This is in fact the case, and leads to the BitfieldEnumPropertyTypeCastWithRemappingTest case passing when using the following mappings:

remappedNames: new Dictionary<string, string>()
{
    { "Flags", "FlagBits" },
    { "FlagBits", "FlagBits" }
});

This is how it affects the generated Silk.NET Vulkan bindings: dotnet/Silk.NET@ead8e31

---

The issue I have with this is that this seems hacky and I'm struggling to verify whether this is the intended usage of this line of code.

What is the intended purpose of this line of code?

[tannergooding]

Remapping to self exists to support certain kinds of typedefs that exist in C/C++.

Namely there are scenarios where you may have a T and the bindings are exposing it as T, but the actual type in C is U. This frequently occurs for some "transparent struct" wrappers that can be generated for convenience.

For enums this probably shouldn't be required and it should handle it automatically. However, there's some nuance if there exists something like enum XBits and typedef uint X where you're wanting to use the friendly name X instead of XBits for the enum.

[Exanite]

Namely there are scenarios where you may have a T and the bindings are exposing it as T, but the actual type in C is U. This frequently occurs for some "transparent struct" wrappers that can be generated for convenience.

Ah, so the self remapping is to prevent the type from being excessively simplified / reduced down.

For enums this probably shouldn't be required and it should handle it automatically.

Yup, this is already handled properly.

However, there's some nuance if there exists something like enum XBits and typedef uint X where you're wanting to use the friendly name X instead of XBits for the enum.

This is the case that isn't handled right now.

I can work on a PR for this. I assume that I just need to change the condition under which the casts are added to include any remapping and exclude cases where the casts are identifiable as redundant.

[tannergooding]

Yep that sounds correct and reasonable

[Exanite]

I'm looking into the existing behavior as I attempt to fix this since I don't want to break any existing behavior.

One thing I noticed is that the backing uint, current sbyte case isn't handled correctly.
The current output doesn't match the comment and also doesn't compile (not shown here).

I also believe the corresponding comment is incorrect and should be (sbyte)((sbyte)(value << cns) >> cns) instead. This is to correctly handle the sign extension when right shifting.

ClangSharp/sources/ClangSharp.PInvokeGenerator/PInvokeGenerator.VisitDecl.cs

Lines 2733 to 2740 in e577463

	// backing int, current int (value << cns) >> cns
	// backing int, current uint (uint)((value >> cns) & msk)
	// backing uint, current int ((int)value << cns) >> cns
	// backing uint, current uint (value >> cns) & msk
	// backing uint, current byte (byte)((value >> cns) & msk)
	// backing uint, current sbyte (sbyte)((value << cns) >> cns)

Here is a .NET Fiddle showing the difference in behavior: https://dotnetfiddle.net/DFKSFY
The uint is initialized to all 1s, so any sbyte extracted from the uint should be -1.

If this is correct, I can change fix this alongside the original issue.