Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Bring back newtypes instead of type aliases #1393

Closed
poliorcetics opened this issue Dec 20, 2021 · 11 comments · Fixed by #1423
Closed

Feature request: Bring back newtypes instead of type aliases #1393

poliorcetics opened this issue Dec 20, 2021 · 11 comments · Fixed by #1423
Labels
enhancement New feature or request

Comments

@poliorcetics
Copy link
Contributor

Motivation

Before 0.29, Foundation::HWND and friends were defined as wrappers around a usize/isize or something else. It made code a little verbose but it was very easy to differentiate values and to see exactly what was what.

This is now impossible and can easily lead to confusion when passing defaults/values defined far away.

For example, I liked to use Type::default() or HWND(0) because it made the type very clear, even in code reviews where type hint are hover docs are missing.

Drawbacks

Verbose, probably worse compile times.

Rationale and alternatives

Keep current format.

Additional context

One thing that was missing was Binary operations, like Or/And/Xor for such types. Since those are now type aliases, that makes them available by default, which is nice. Maybe find a way to have both ?

@poliorcetics poliorcetics added the enhancement New feature or request label Dec 20, 2021
@tim-weis
Copy link
Contributor

tim-weis commented Dec 20, 2021

It's not just clarity either. The following code compiles with v0.29:

unsafe fn foo() {
    let dc = GetDC(0);
    let _ = GetDC(dc); // Should take an HWND, not an HDC
}

Likewise, you can pass a BCRYPT_ALG_HANDLE anywhere a BCRYPT_KEY_HANDLE is expected (or an HINSTANCE, for that matter), and so on. Up to v0.28 the type system prevented those errors.

After moving to v0.29 some of my code got less verbose (mostly around conversions between concrete GDI object handles and the generic HGDIOBJ), though I do feel a bit uneasy about the type system no longer helping me out in detecting this specific class of bugs.

@ryancerium
Copy link
Contributor

One thing that was missing was Binary operations, like Or/And/Xor for such types. Since those are now type aliases, that makes them available by default, which is nice. Maybe find a way to have both ?

With the tuple struct versions it would be easy enough to add impl blocks for the various traits in std::ops, something like

impl std::ops::BitAnd<HWND> for HWND {
    type Output = HWND;
    fn bitand(self, rhs: HWND) -> Self::Output {
        HWND(self.0 & rhs.0)
    }
}

@kennykerr
Copy link
Collaborator

I figured consistency with windows-sys and the simplicity of the model was beneficial but if you all feel strongly 😉 about strongly-typed handles I'm happy to bring that back as well.

@avitex
Copy link
Contributor

avitex commented Dec 23, 2021

Just to throw my opinion into the mix, I prefer the type aliases from experience with using the unsafe Win32 APIs, especially with types that wrap pointers. Seeing the types are being in a unsafe context, the uttermost care should be taken regardless. Some values are not returned wrapped even though the value is of that type, which requires manual construction regardless.

  • I'm always referring back to MSDN to check usage documentation to double-triple check.
  • Once you add a wrapper type, critical functionality like bit ops for flags requires additional implementation + slower compiles
    • This increases the temptation to add functionality onto generated types that I find is actually a hindrance. I think BSTR is a good example of this in effect. BSTR provides very minimal functionality. In my usage, I'd want to use a safe wrapper for BSTR leaning on NonNull<u16> so rather than passing BSTR::new() for an optional argument, Option<NiceBStr> - None could be used with the additional optimization.
    • Making an external wrapper with a nicer interface doesn't negate from the fact you still have to return the type for IntoParam, or more specifically for BSTR a &BSTR because BSTR owns the allocation and we have to prevent the dealloc and any UAFs. Providing a reference to BSTR means the owning type has to wrap BSTR internally, or have a compatible layout to mem::transmute a reference to. This then limits what you can do like making a DST for BSTRs for supporting Cow without some additional trickery. If IntoParam (a private trait) for BSTR could accept a raw pointer, or specifically the ABI type, this would solve my usecase but perhaps is not very elegant.
  • Types wrapping pointers currently don't account for *const T, if there are plans for that in the future that will further increase the impl generation and casting between *const and *mut will have to be taken into account
  • Making variable names explicit for the values you're dealing I find is critical for review (regardless of the wrapper)
  • If I want a nicer API with documentation I would write a safe interface over the raw types anyway rather than repeatedly calling the same unsafe functionality and having to reason about the same constraints each time

On a side note, I find the duplication between the windows crate and windows-sys strange. If both windows and windows-sys are to both provide essentially the same unsafe Win32/* APIs as an example with only minor differences I would prefer library authors use windows-sys in my dependency tree to save compile time as a user, with the worse case being different libraries using both. If the windows crate is to focus on safe interfaces then I would cut out the duplication pushing library authors to the crate best supporting their requirements.

Is there a future plan to consume windows-sys within windows for higher level wrappers? I find windows with Win32 in the weird position between being a generated library to be consumed and wrapped by a higher level crate, and providing some helper types for some common things like a higher level crate would. Perhaps I'm getting a bit side tracked here.

Note: This could boil down to that I prefer what windows-sys provides, but currently interface functions are missing. I don't like the helper allocations (eg. UTF-8 to wide) with IntoParam and prefer handle it externally to amortize the allocation where possible.

I'll also add that I greatly appreciate the amount of effort you've put into this project @kennykerr. You have a very intense workflow and I hope you are getting some rest over these holidays!

@tim-weis
Copy link
Contributor

tim-weis commented Dec 23, 2021

@avitex raises a valid question: What is the scope of windows going forward?

Prior to the split of windows into the windows and windows-sys crates, things were a fair bit fuzzy. As a single library trying to cater to several different audiences, there had always been a tension between faithfully reproducing the Windows API's ABI in terms of Rust's type system, and the desire to more generously employ the latter to strengthen the guarantees when interacting with the former.

With the windows-sys crate covering the ABI, the windows crate is pushed back into that same limbo it had been all along, trying to find its sweet spot. The spectrum ranges between:

  1. Staying as consistent with windows-sys as possible, at which point the windows crate may as well be deprecated in favor of windows-sys.
  2. Applying the new type idiom to guard against using logically unrelated values that have coinciding ABI layout.
  3. All of the above plus providing implicit conversions for common Rust → Windows types (such as Stringwchar_t*).
  4. All of the above plus mapping Windows' error reporting onto Rust's Result enum.
  5. All of the above plus some sort of "middle ground".
  6. All of the above plus mapping the remaining API types to Rust's type system.
  7. Going full over-board by codifying all of the above plus API contracts.

1 is a no-op (modulo administration), 2-4 are mostly mechanical, 5 requires outstanding engineering skills (such as Kenny's), and 6 through 7 would be down to crowd sourcing.

As a user I'd absolutely hope for 7 to happen. As a realist, though, I'm going to have to ask: Where will the windows crate actually land in the foreseeable future?

@ryancerium
Copy link
Contributor

Regarding #4, I actually started writing a documentation parser that made a pretty good effort to figure out what type a function returns, which *GetLastError() function it uses, and what the return value for the error condition is. Most of them are 0/null/FALSE, but enough of them aren't. I was going through the remaining documentation with the tool to figure out the rest by hand. It was turning out to be fairly successful, but I never finished it. I'd be happy to contribute to the metadata project if anyone thinks it would have a snowball's chance in Jamaica of getting used.

@kennykerr
Copy link
Collaborator

Briefly regarding windows vs windows-sys, the latter exists to combat the build time concerns with the windows crate (#1310). If build time were free, the windows-sys crate would not exist. As a result, the windows crate aims to provide rich access to all APIs, even if it costs a bit more to compile, while the windows-sys crate forgoes many features and capabilities if it hampers compile time. For example, the windows-sys crate currently has no support for COM or WinRT APIs.

@jminer
Copy link

jminer commented Jan 9, 2022

I like the type aliases better than newtypes, largely because they are less verbose.

Another reason is that there are some functions that take different types than the parameter. For example, CreateFont returns an HFONT, and CreateCompatibleBitmap returns an HBITMAP. But you pass them both to SelectObject and DeleteObject (which take an HGDIOBJ).

And I don't remember having a bug due to passing the wrong type to a function, but I don't have a whole lot of code using the Windows API.

Regarding windows vs windows-sys, the only thing I don't like about the windows crate's functions is the heap allocation to convert strings. I haven't benchmarked if it makes a difference in my usage, but I like having the option of using stack allocation to convert small strings to UTF-16. This is what I'm doing with winapi:

https://github.com/jminer/dynamin/blob/fd755a379b284cc4a9b56daf725ddff44c9ffd22/src/windows_backend/window_backend.rs#L368-L370

https://github.com/jminer/dynamin/blob/fd755a379b284cc4a9b56daf725ddff44c9ffd22/src/windows_backend/mod.rs#L17-L23

@roblabla
Copy link
Contributor

roblabla commented Jan 9, 2022

But you pass them both to SelectObject and DeleteObject (which take an HGDIOBJ).

Seems like SelectObject and DeleteObject should take a From<HGDIOBJ>, and that trait should be implemented for HFONT and HBITMAP.

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jan 11, 2022

Hey all, this thread asks some good questions. Specifically about the change to the handles, but also more generally about the relationship between windows-sys and windows. I'll attempt to answer both questions as well as I can here.

TL;DR: We're planning to bring back newtypes for the windows crate for ergonomics reasons, but leave type aliases as the interface for windows-sys as they're faster to compile (see: #1423).

What is the relationship between windows-sys and windows

As Kenny explains in #1393 (comment) the windows and windows-sys crates are built for different purposes:

Crate name Win32 support WinRT and COM support Focus Best for
windows Good ergonomics Applications
windows-sys Fast compile times Libraries

We wish the windows would both provide both good ergonomics and compile-times, but the unfortunate reality is that we're limited by the performance of both cargo 1 and the compiler. Which is not surprising, because with over over 230.000 unique (!) types Windows is definitely pushing the boundaries of what the compiler expects to handle in a library 2.

We hope the situation will improve over time, and we can eventually recommend windows for both applications and libraries, but unfortunately we're not there yet.

WinRT and COM for libraries

As shown in the table above, windows-sys doesn't support WinRT and COM APIs. This means that for now if you want to use those APIs in a library you have to use the windows crate for now.

We're still thinking through a solution to improve on this. Bare WinRT and COM bindings are incredibly hard to use. And adding ergonomic bindings to windows-sys would drastically increase compile times. Right now we're thinking of bringing windows-bindgen to a good enough spot so library authors can generate their own types where needed, but that doesn't seem like a great long-term solution either 3.

What shape should handles have?

First off: what are handles? Unlike what some might expect, handles are not necessarily pointers: they tend to be opaque values representing a resource. windows-sys uses the semantics exposed by the platform's underlying types, so in most cases pointer semantics won't apply. 4

With that out of the way, we can attempt to answer the question in this thread: how should we expose the handles? After talking about it yesterday with @kennykerr, we're thinking of doing different things for the different crates:

  • windows: focus on ergonomics by exposing a new-type, enabling things like Type::default() to work again.
  • windows-sys: focus on performance by exposing a type-alias, which provides fewer guarantees but compiles faster.

We hope that this will satisfy the varying needs folks have, as right now we unfortunately can't reconcile them in a single crate.

Closing notes

We'd love to hear what you think of this! We hope to provide better documentation on the differences between windows and windows-sys, and this is a first attempt at that. Also are there any uses we missed? Would this change be problematic for certain folks? Let us know!

Footnotes

  1. Download sizes are a real concern too.

  2. Not all types add the same cost. For the windows crate we've observed that in particular generating function bodies is expensive.

  3. windows-rs used to only expose codegen functionality. We moved away from that because it made it difficult to align types between crates, and reuse generated code by the compiler. While windows-bindgen has its place, we are aware of the tradeoffs that come with it.

  4. Some work in Rust has gone in to model OS resource handles using borrow semantics: RFC 3128: IO Safety. But it's an open question how much of this we may be able to leverage for handles windows/windows-sys, and whether we can make them fit their intended goals.

@rpjohnst
Copy link

rpjohnst commented Jan 11, 2022

Another, perhaps more actionable, way to phrase the "for libraries" or "for performance" vs "for applications" distinction is that -sys crates should be purely FFI declarations, as unopinionated as possible, and exposing the foreign API as directly as possible.

The primary reason the -sys convention was introduced to the ecosystem in the first place was to avoid situations where idiomatic wrapper APIs (inadvertently, or intentionally for safety or ergonomics reasons) force some choice that the underlying API does not, and working around that requires patching the wrapper or duplicating the FFI declarations.

This goal does tend to overlap with faster compile times, but it's also about flexibility, and a clean separation between what is provided by the foreign API and what is specific to Rust. So in that sense, type aliases are the right choice for a -sys crate, specifically because that is how the foreign API works at the level of direct FFI declarations. And, for example, while bare COM is quite difficult to use directly, it does need to be exposed somewhere (whether via bindgen, a pre-generated -sys crate, etc). The analogous situation for "native" use of COM is the C bindings generated by MIDL- they're also quite ugly, but they are still available for the rare situations where they're necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants