Use #[non_exhaustive] attribute on structures
#774
Conversation
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
There was a problem hiding this comment.
The .monocodus config not found in your repo. Default config is used.
Check config documentation here
| @@ -162,46 +162,39 @@ impl<B: hal::Backend> Adapter<B> { | |||
|
|
|||
| let adapter_limits = raw.physical_device.limits(); | |||
|
|
|||
| let default_limits = wgt::Limits::default(); | |||
| let mut limits = wgt::Limits::default(); | |||
There was a problem hiding this comment.
A disadvantage is that we cannot explicitly initialize the fields of this structure in wgpu-core, since it's treated as an external crate. We have to initialize the default then mutate it.
| /// This struct should be partially initalized using the default method, but binding, visibility, | ||
| /// and ty should be set. | ||
| pub _non_exhaustive: NonExhaustive, |
There was a problem hiding this comment.
The advantage is that this field no longer has to exist in the public API. And we can enforce the fact that binding, visibility and ty should be set because people are now forced to use the new() method.
There was a problem hiding this comment.
Looks nice!
@cwfitzgerald do you recall why we didn't go this path originally?
I think one of the reasons was that we had to keep the MSRV low enough for Debian to be able to build any of our stuff.
|
From what I gathered from conversations in the matrix, it seemed like going this was wasn't idiomatic, because we couldn't use initialization syntax. |
|
I'm tempted to say let's drop the current hack and just use the attribute like in the PR. We'll lose the ability to have struct initialization syntax (unfortunately!) but we can cover some of that with builders. |
|
So our solution to non-exhaustive needs to fulfill 4 main requirements:
This is complicated by wgpu not being a single crate, but 3 separate crates. For the purposes of ensuring forwards comparability, rustc sees no difference between wgpu-core and a user crate in terms of "is it us". This requirement really puts us in a bind in terms what to do. Our current implementation style fulfills 1a, 2, 3, and 4. Non-exhaustive on its own has issues. First it prevents both partial and exhaustive construction entirely causing people to have to use ugly out of line mutation to set any extension data, which is a very unrustic interface. Second it thinks that wgpu-core is a user, so it also has to follow non-exhaustive rules, causing wgpu-core to also need to add mutation. This fulfills only 1. The other option is to keep a non-exhaustive member as a private field. This also prevents partial construction due to what I would consider a rust bug (rust-lang/rust#63538). It also thinks of wgpu-core as a user, also fulfilling only 1. So this situation is kinda terrible all around. Our current solution unfortunately fails mandatory requirement 1b, so it is actually no good. I propose an extension of the method @kvark mentioned and combine non-exhaustive with the builder pattern. This would fulfill 1, and arguably 3 and 4. It's not ideal, but I frankly can't spin any other way of doing this that correctly prevents issues. I have a couple ideas for a decl macros that would make the generation of the builder pattern easier. Additionally derive_builder and typed-builder both exist and are worth taking a closer look at, though they would add back our dep on syn. Once we get a solution for this problem, feel free to keep working on things for #689. I'm going to close this as I don't think this will work as is. |
|
As I asked on the chat, @cwfitzgerald this is great analysis. I don't fully understand why we are closing the PR though. Isn't the plan to go with Another wish, if I may, that we be very careful before introducing the auto-generated builder patterns. It might be better to just write them by hand, this would allow builder methods to make more sense. For example, in |
|
I closed the PR because it will force us to change the initialization code twice, first to this mutation based approach, then to the builder based approach and I would rather just do it all in one fell swoop than have to change back and forth. There's no real urgency to fix 1b right now, 0.6 is still a bit away 😄
I entirely agree with this design. I am actually very surprised that the existing proc macros don't support that at all, and means that they are both not useful for our purposes. I think I can come up with a simple-enough decl macro solution that both gets us what we want, and means we don't have to hand write all our builders. |
|
So to summarize a discussion @kvark and I had on the matrix, we've come to the conclusion that it just too early in wgpu's lifecycle to be making decisions about stability. We should revisit the decision when we are thinking about moving from 0.X to 1.X. Until then, let's remove all non-exhaustive and the non-exhaustive struct from all the types, and just continue releasing wgpu as a series of breaking changes. @GabrielMajeri if you would be interested in doing that work, that would be very helpful, if not, I can do it. |
|
No problem, I'll can work on that. |
787: Remove non-exhaustive markers r=kvark a=GabrielMajeri Based on the previous [discussion](#774 (comment)), this PR removes the `NonExhaustive` structure and any `#[non_exhaustive]` attribute since it's too early to decide how to ensure forward compatibility. **Testing** Checked the `core`, `types` and `player` crates. Co-authored-by: Gabriel Majeri <gabriel.majeri6@gmail.com>
Description
The
#[non_exhaustive]attribute can also be applied on structures. This will make it impossible to explicitly initialize those structures by specifying the field values outside thewgpu-typescrate.Testing
Checked with
playerandwgpu-rs.