microsoft / win32metadata

Tooling to generate metadata for Win32 APIs in the Windows SDK.
Other
1.33k stars 113 forks source link

DIPROP_* constants don't match Win32 API #1720

Open Barinzaya opened 1 year ago

Barinzaya commented 1 year ago

Suggestion

The DIPROP_* constants (from DirectInput) are defined in the Rust bindings in a fundamentally different way than they are in the original Win32 SDK.

In the original header (dinput.h), for C code they are defined as such (simplified for brevity):

#define MAKEDIPROP(prop)    ((const GUID *)(prop))

#define DIPROP_BUFFERSIZE       MAKEDIPROP(1)
#define DIPROP_AXISMODE         MAKEDIPROP(2)
#define DIPROP_GRANULARITY      MAKEDIPROP(3)
#define DIPROP_RANGE            MAKEDIPROP(4)
...

Note that these constants do not represent actual GUIDs, but numeric values cast as a pointer to a GUID (const GUID *). For C++, they are additionally dereferenced to make a C++ reference rather than a pointer, but are fundamentally the same.

In the Rust bindings, they are defined as such (simplified for comparison's sake):

pub const DIPROP_BUFFERSIZE: ::windows_core::GUID = ::windows_core::GUID::from_u128(1);
pub const DIPROP_AXISMODE: ::windows_core::GUID = ::windows_core::GUID::from_u128(2);
pub const DIPROP_GRANULARITY: ::windows_core::GUID = ::windows_core::GUID::from_u128(3);
pub const DIPROP_RANGE: ::windows_core::GUID = ::windows_core::GUID::from_u128(4);
...

In the Rust binding, they are actual GUIDs whose contents are derived from the associated numeric values. Their meaning (and usage) is different than with the original SDK.

These constants are intended to be passed to the DirectInput GetProperty and SetProperty methods, which both have an argument that takes a const GUID */*const GUID. I am not currently aware of any other uses of these constants. From an MSDN example:

hr = idirectinputdevice9_GetProperty(pdid, DIPROP_BUFFERSIZE, &dipdw.diph); 

Due to the mismatch, rather than being able to pass the constant directly in, the constants have to be shuffled around in order to function as intended. An equivalent example using the Rust bindings would have to be:

hr = pdid.GetProperty(DIPROP_BUFFERSIZE.to_u128() as *const GUID, &mut dipdw.diph as *mut DIPROPHEADER);

As a side effect, at a first glance at the Rust API, one might assume that they would be used differently:

hr = pdid.GetProperty(&DIPROP_BUFFERSIZE as *const GUID, &mut dipdw.diph as *mut DIPROPHEADER);

This is safe, but results in a the function returning an error (DIERR_UNSUPPORTED/E_NOTIMPL) as these GUIDs are not handled by DirectInput and it isn't immediately obvious why.

It seems to me that the DIPROP_* constants should be defined as:

pub const DIPROP_BUFFERSIZE: *const ::windows_core::GUID = 1 as *const ::windows_core::GUID;
pub const DIPROP_AXISMODE: *const ::windows_core::GUID = 2 as *const ::windows_core::GUID;
pub const DIPROP_GRANULARITY: *const ::windows_core::GUID = 3 as *const ::windows_core::GUID;
pub const DIPROP_RANGE: *const ::windows_core::GUID = 4 as *const ::windows_core::GUID;
...

I feel that this more closely matches the original intent and usage of these constants, though any change would be a breaking change to any code currently using them. As far as I can reason it seems like it should result in a compile error for most reasonable usages, rather than resulting in incorrect behavior, but I'm not sure if there would be any potentially unsafe results from changing the types of these constants.

kennykerr commented 1 year ago

Yikes, that's an interesting way to define GUIDs! 😏

Anyway, these constants are defined by the Win32 metadata project so I'll transfer this issue to that repo for consideration.

mikebattista commented 11 months ago
[Guid(0u, 0, 0, 0, 0, 0, 0, 0, 0, 0, 24)]
public static Guid DIPROP_VIDPID;

This is how they're defined in the metadata. How should this be changed?

kennykerr commented 11 months ago

That's just it - these aren't GUID values at all. They're GUID* values (pointers).

mikebattista commented 11 months ago

I still need to know how to change the metadata.

kennykerr commented 11 months ago

I'm not sure how the metadata represents this - it is similar to constants like RT_ICON where the type is different to the value.

public const PWSTR RT_ICON = 3;

So I would imagine it would be something like this:

public const GUID* DIPROP_BUFFERSIZE = 1;
mikebattista commented 11 months ago

Thanks. I should be able to follow the RT_ICON pattern.