Implement BSTR and PWSTR using platform-agnostic widestring crate

[MarijnS95]

Widestrings are UTF-16 on Windows but UTF-32 on other platforms. The widestring crate abstracts this detail away behind its WideString, WideCString, WideStr and WideCStr type aliases for convenience. Aside that it provides useful helpers and makes a clear distinction between null-terminated strings and "binary strings" with an explicit length and interior NULLs.

The only downside of widestring is that it includes winapi under dev-dependencies for an example in the documentation.

This change adds some missing value comparison, debugging and printing functionality.

TODO

Shims for non-Windows platforms are not ready. They're in a separate commit and best reviewed (and PR'd) separately, but it'd be nice if we can discuss how to override the Sys* functions themselves. see also #770.

crates/gen/src/types ergonomics

Editing these massive generated quote! blocks is really cumbersome without autoformatting and rust-analyzer. Are there plans to promote these types to the windows crate (like HRESULT and IUnknown)? That makes editing easier and slims down all autogenerated files by a fair bit.

[MarijnS95]

I don't think OLECHAR * should be considered as PWSTR, but alas.

[MarijnS95]

Don't think we can perform the conversion at once, assuming we really want to use SysAllocStringLen to get a proper length-prefixed string without being able to do the conversion in-place.

[MarijnS95]

What are the ownership semantics around PWSTR pointers and cloning/copying? This doesn't seem correct 😬

[MarijnS95]

Given that as_wide performs null-checking on the self-pointer already (and panics), an unchecked string with interior nulls will never be equal. Perhaps better to remain on the safe side (from_str() -> Result) and map the error to false instead?

[MarijnS95]

crates/gen/src/types ergonomics

Editing these massive generated quote! blocks is really cumbersome without autoformatting and rust-analyzer. Are there plans to promote these types to the windows crate (like HRESULT and IUnknown)? That makes editing easier and slims down all autogenerated files by a fair bit.

Implemented it this way in the last commit (but it breaks the Linux implementation for now, even though it should make overriding functions easier in the end, as long as they are publicly exported and disallowed from the generator). Let me know what you think, if we end up going this route I'll reorder the PR to apply that commit first - and put it in a separate PR.

[kennykerr]

Sorry for the delay - been heads down working on #81. I'll take a look hopefully soon. Just a quick caution, those types were previously part of the Windows crate but it doesn't really scale to include them - where does it end? I realize its simpler that way but I'd like to simplify the way that types like this are extended in the generated bindings instead. Basically the rule is that types have to be defined in the Windows crate if and only if they are used by both WinRT and Win32 APIs and thus need a canonical definition.

[MarijnS95]

Just a quick caution, those types were previously part of the Windows crate but it doesn't really scale to include them - where does it end?

Can you clarify what "doesn't scale"? The same applies to the current generation overloads, those files and lists may explode over time too.

Do you mean namespaces? We can easily fix that by moving these types into the desired (as per metadata) namespace, and we can/should also reexport them from generated bindings instead of having these live somewhere in windows crate root or module (in turn saving on renaming churn again). I intended to do that but haven't gotten to it yet, would that address your concerns?

Basically the rule is that types have to be defined in the Windows crate if and only if they are used by both WinRT and Win32 APIs and thus need a canonical definition.

I still feel like not emitting completely static code to a generated file is a win, in particular because this also allows both autoformatting and development environments like rust-analyzer to work its magic in admittedly complex blocks of code.

[kennykerr]

It doesn't scale in the sense that there is an never-ending list of types that someone will feel deserves some form of extension and incorporation in the Windows crate. Its BSTR now, VARIANT tomorrow, a few D2D types the next, a few dozen D3D types, and so on. There's an admittedly fine line between what's core to most/all APIs and what can and should exist as specialized and idiomatic Rust wrapper library crates. I'm still working through where that line is, but I'd like to be cautious about just lumping too many nice-to-haves in the Windows crate as it can become a maintenance and support burden that is not easily managed. Ideally it could be driven entirely from metadata, but the Win32 metadata isn't nearly as rich and descriptive as that of WinRT.

[MarijnS95]

It feels pointless to discuss where these types should reside as I really don't care. Put them in a separate crate for all you want. Rather, we should discuss the main problem I see in the current approach and whether that's something you're willing to solve in the first place. Repeating that, it's quite simply formulated as: don't put static blocks of code (not influenced by external variables) in quote! blocks but have them as "normal" (non-generated) code somewhere. That way tools like rustfmt and rust-analyzer can be used, instead of having to work on generated output and copypasting back the results.

For now I've removed this commit as it has little relevance to widestring which should be discussed here instead. Changes are still available under https://github.com/MarijnS95/windows-rs/compare/widestring..widestring-in-windows-crate, let me know if I should open a PR or separate issue to move this further.

That said again, all the overrides moved by this implementation are already defined somewhere in this repository. Whether the override sits in the Windows crate or windows_gen doesn't make a difference to the "does not scale" argument as it's extra code and cognitive overhead either way. The suggested change simply aims to make it easier to work on that code irrelevant of "fine line" discussions whether a type should be eligible to overriding in the first place. If the only point is Win32 vs WinRT, simply make a Win32 "base" crate and case solved? Note that the core error/result types use BSTR/PSTR/PWSTR.

[kennykerr]

I appreciate some of these goals but this PR is pushing the Windows crate too far in different areas. Feel free to open and discuss the individual issues further.

[MarijnS95]

Feel free to open and discuss the individual issues further.

Closing PRs with associated conversational effort for sure isn't the right way to get that started. But it's an easy way to cut off a conversation.

[kennykerr]

This PR should have been preceded by an issue discussion. I'm sorry I didn't make that clear earlier. I love your work and enthusiasm but design changes need to be carefully considered as there are often conflicting requirements in a project of this nature.

[MarijnS95]

That is a fair point and largely backed by me pushing to move static code outside of quote! to get rustfmt and rust-analyzer to work, which isn't the scope of this PR at all. Apologies for that, those commits have been removed and could - if you want - be discussed elsewhere.

If we can go back to the actual subject here - widestring - sometimes it's just easier to try something, make a "proof of concept" and use a PR to convey what the final change could look like rather than attempting to discuss it up-front. That's what was supposed to happen here but thus far not a single word has been written on the actual idea and implementation of using widestring. This crate was mentioned a couple times in other issues thus far but not brought anything to fruition yet, hence this PR.
In my eyes a PR (can be marked draft if you want) should be just as good a discussion point as an issue. Are you open to reconsider that? I will of course remove the Linux implementations for BSTR too as those are "heavily depending" on DXC following the documented specification.

After all, I needed to write this to test if windows-rs would be viable for my intended usecase (which it totally is would have been!). I'll keep it in mind next time to create an issue and link to a compare page instead.

I love your work and enthusiasm

You closed every single one of my open PRs and issues within an hour or so, that seems pretty contradictory. How can I be reassured that won't happen again when I "Feel free to open and discuss the individual issues further"?

[kennykerr]

I'm sorry I didn't get around to commenting on widestring. I've been rather swamped and its not easy to find the time explore every suggestion. I haven't had a chance to look at the widestring crate. I'm generally weary of taking on additional dependencies. There are also quite a few open issues already having to do with string support and I have to consider how best to handle them all in a consistent, scalable, and complementary way. It's not a simple problem and not one I have the time for right now - I have other urgent priorities. I will however not likely do much more with strings until I've had the time to consider a more holistic solution to the problem.

I don't really mind whether you want to raise issues with a PR if it helps to illustrate or comment on code. But ultimately, I need to make the call on whether suggestions/issues/contributions/PRs are in the best interest of the project. And if they're not I'm going to eventually close them because not closing them gives the false impression that the specific issue or contribution is still open for consideration. Cross-platform support for example.

Feel free to disagree, open new issues or PRs, or refine suggestions on your own before coming back for more feedback. This is a big project and I'm doing my best to manage it while also being the primary contributor. Thanks for your patience.