Consider using pointers for handle types rather than isize

[ChrisDenton]

Suggestion

This would be more consistent with the Rust standard library which can't change its HANDLE type for stability reasons. There are also some types of handle that are actually pointers, rather than pointer-sized integers. For example HMODULE is the base address of a loaded module, although this isn't guaranteed by the docs (I suspect, however, that this could never be changed in practice).

This has been talked about extensively elsewhere but I'm creating an issue here for consideration. Note that making this change would likely cause a fair amount of churn, at least in the short term.

[kennykerr]

Thanks for the reminder. I've been meaning to just make the switch here.

[kennykerr]

The only real challenge is that the Win32 metadata assumes many of these have integer values for invalid values:

Then windows-bindgen currently gives up and doesn't give you the equivalent validity checking:

We'll need a solution to this before proceeding.

[kennykerr]

On the other hand, there are already some handles that use a pointer and use integer invalid values. 🤪

So updating code gen to manually cast for such pointer handles would seem like a suitable solution:

[ChrisDenton]

That looks good to me. It would be slightly nicer to specialize the 0 case to is_null() but I wouldn't consider than essential.

[riverar]

I don't agree with this change at all, just noting that here for the record. This issue opens with an invalid assertion that "some types of handle that are actually pointers". That's an irrelevant implementation detail.

[ChrisDenton]

Honestly, I just want to stop having to argue about this one way or the other.

[kennykerr]

I just don't see this as any less "correct" than the canonical definitions in the Windows headers that, for the most part, use void* and are thus defined as pointers to begin with. It's the Win32 metadata that has attempted to influence the type in a different direction by attempting to retype these handles instead of remaining faithful to the original definitions.

Whether this is more correct is debatable and subjective and since the Rust Standard Library has already taken a position on this, I don't see any benefit in windows-rs taking a different approach.

[kennykerr]

Another problematic example is LRESULT that is rightly defined as a pointer-sized integer in C++ but since the Win32 metadata rewrites most void* handles as pointer-sized integers, it's not immediately obvious how to tell which were originally pointers and which, like LRESULT, are rightly pointer-sized integers.

[riverar]

Remaining faithful to the original definitions would make handles typedef WORD HANDLE; (Windows 1.01 SDK).

[ChrisDenton]

Another problematic example is LRESULT that is rightly defined as a pointer-sized integer in C++ but since the Win32 metadata rewrites most void* handles as pointer-sized integers, it's not immediately obvious how to tell which were originally pointers and which, like LRESULT, are rightly pointer-sized integers.

Can metadata add some disambiguation? Even if the metadata types remain the same, perhaps an attribute could be added for C pointer types?

[riverar]

I have expressed my opinion in various threads and posts, but I am willing to move past that and align to the consensus here. Would it be easier if we made this change to the upstream metadata?

We should also, at the end of this, inform the folks working on Rust pointer provenance. (Not going to @ tag them until later.)

[ChrisDenton]

Would it be easier if we made this change to the upstream metadata?

That does sound a lot easier than hacking a workaround...

[riverar]

What's the proposed change for metadata? We seem to already emit most handle types as IntPtr/UIntPtr, aside from some errors (e.g. microsoft/win32metadata#1540) where some types got marked with void* directly.

[ChrisDenton]

IntPtr/UIntPtr is also used for pointer-sized integer types like LPARAM.

[riverar]

Ah right. I remember now we're using an older C# without native integer types. OK, so the change would be to emit all handle types as void* (unless they're specialized otherwise).

[RalfJung]

From a pointer provenance perspective, in Rust a raw pointer can hold strictly more values than a ptr-sized integer, so it is the "safer" choice in that sense.

FWIW, for validity checking I'd use self.0 as isize == -1 -- int2ptr casts are semantically the much more subtle operation than ptr2int casts. (Once strict provenance stabilizes, the proper way to compare this will be self.0.addr() = -1 as usize.)

[Zerowalker]

pointer's aren't thread safe (Send),
given that, what's the proper way to handle it with this change.
For example HWND, is safe to pass to other threads, but now it's not.

[kennykerr]

This is actually more correct in terms of safety. Handles are more like Rust pointers in the sense that while you can take their address/value and pass it to another thread, what you do with it will likely not be safe without some other form of synchronization/coordination. You can always wrap them inside another struct that implements Send/Sync once you've figured that out.

[elsnerpaul]

@kennykerr can you please give an example of such work around? I was using the handle of a window in my proxy object and used PostMessageW to safely send a message. Now after upgrading to 0.58 my object is not 'send' any more.

[kennykerr]

Please open an issue for any additional questions. It's hard to keep track of conversations on closed issues.