No longer auto-implement Default for bitfields#1630
Conversation
…Default, for DuplicateFlags.
Bromeon
added
breaking-change
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1630 |
Bromeon
left a comment
Bromeon
left a comment
There was a problem hiding this comment.
Thanks! I added some comments.
Please also rebase, fix CI and squash commits to 1 🙂
| /// For types where a [`Default`](Default) is helpful, but the default value should | ||
| /// be something other than what would be returned by a `#[derive(Default)]` implementation. | ||
| /// | ||
| /// An example of this is `DuplicateFlags`, where the default flags to `duplicate` are non-zero, | ||
| /// so if one wants to use "all flags except one" in `duplicate_ex`, the most natural way to do | ||
| /// so would be to pass `DuplicateFlags::default() & !DuplicateFlags::USE_INSTATANTIATION` to `flags`. | ||
| /// | ||
| /// * `Some(TokenStream)` -> function body which will be psted into the `Default::default` implementation. | ||
| /// * `None` -> no special default, i.e. if a Default is generated it will be with an automatic derive. | ||
| #[rustfmt::skip] | ||
| pub fn does_enum_have_special_default(class_name: Option<&TyName>, enum_name: &str) -> Option<TokenStream> { | ||
| let class_name = class_name.map(|c| c.godot_ty.as_str()); | ||
| match (class_name, enum_name) { | ||
| (Some("Node"), "DuplicateFlags") => Some(quote! { | ||
| Self::SIGNALS | Self::GROUPS | Self::SCRIPTS | Self::USE_INSTANTIATION | ||
| }), | ||
|
|
||
| _ => None | ||
| } | ||
| } |
There was a problem hiding this comment.
Please use our line length of 140-145 for comments/RustDoc. Also fix typos like USE_INSTATANTIATION, but I think this may become obsolete, see other comment.
You can avoid the Some(...) by doing this:
let tokens = match (...) {
(...) => quote! {
...
},
_ => return None,
}
Some(tokens)| if self.is_bitfield { | ||
| if self.is_bitfield && self.override_default.is_none() { | ||
| derives.push("Default"); | ||
| } |
There was a problem hiding this comment.
I don't think we should generate a Default without a manual implementation, there's just no good way to automate this in meaningful ways. It would also silently implement defaults for newly added bitfields, and changing them would then become breaking.
If we add impls manually through special_cases.rs, then:
- We're sure the
Defaultvalues are human-reviewed - We know that the ones listed there are actually useful (since we'll add new impls based on user requests)
- There's no risk of breaking changes (once 0.6 is there), as we'll only add new impls.
This has implications beyond this condition:
- "override" naming is no longer appropriate (since there won't be
Defaultout of the box) - same with "special default", now there's just a "default"
- docs in
special_cases.rsneed updating, too
There was a problem hiding this comment.
Perhaps, due to the Default impl changing for types where it is not outright removed, this strategy makes it most clear to users of the crate?
- 0.6.x: Remove the Default impls entirely, for types where it will change. Anyone upgrading will naturally have to refactor, to get their code to build, instead of having to diagnose strange issues where Default has quietly changed but code still compiles as-is.
- 0.7.x: Re-add the Default impls for only types where there is a manual impl, but still ditch the derives.
In the 0.6.x case, a non-Default "meaningful_default" method could be impl'ed so that, even though we force people to look at code that we intentionally break, there is a holdover for Default that's nearly as obvious and convenient. Or just let people use the DEFAULT variant in case of enum bitfields.
There was a problem hiding this comment.
Skipping a cycle just means we'll have worse ergonomics during 0.6, and extra backlog for the future.
I expect users to read migration guides when upgrading minor versions, that's why we put a lot of effort into writing them. We also add deprecations wherever possible, but Rust doesn't support it for impls.
Bromeon
changed the title
Default for bitfields
3 participants
This was split off from: #1627
This would be a breaking change since any code that currently uses
Default::default()and gets0, for the types which are receiving a more specific implementation here, will get something different.It will be a silently breaking change too (code that is broken by this change will likely still compile). It may be preferable to remove the derived
Default, instead of implementing it differently, so that code broken by this change catches API users' attention much more noticeably.Needs: