Remove Scene/SceneList impls for unit (), replace with unit struct#24033
Remove Scene/SceneList impls for unit (), replace with unit struct#24033laundmo wants to merge 1 commit into
Conversation
| /// to add IDE support for nested type names, as it allows us to pass the input Ident from the input to the output code. | ||
| pub const fn touch_type<T>() {} | ||
|
|
||
| #[doc(hidden)] |
There was a problem hiding this comment.
Comments here about why this exists please. This is going to be very mysterious in the code in 5 years!
| } | ||
|
|
||
| #[test] | ||
| fn empty_scene_expressions() { |
There was a problem hiding this comment.
It might be nice to add a compile failure test for this + the ; case, but I won't block on it.
|
On board with this idea: that's a major footgun that we should avoid. |
|
|
||
| #[doc(hidden)] | ||
| #[derive(Clone, Copy)] | ||
| pub struct EmptyTuple; |
There was a problem hiding this comment.
I don't particularly like this pattern. () is semantically an empty tuple and Scene is implemented for tuples.
Having () represent an empty Scene is also useful:
- You can assign it in places like
type Scene = ();. BecauseSceneis implemented for tuples, this is natural. - It lets you do
fn scene() -> impl Scene {}as a form of "todo". Given thatfn scene() -> impl Scene{ todo!() }doesn't work anymore (thanks to some semi-recent rust changes), I quite like having this as an option.
A custom EmptyTuple type also just introduces a form of "jank" to the API that makes everything just a little bit harder to understand (as a reader of docs, an author of internals, and a consumer of public APIs).
Yes, bsn! papers over all of this for most people. But Scene is intended to be an API that is usable on its own.
There was a problem hiding this comment.
I like that you can do this:
fn scene() -> impl Scene {
()
}And then expand into this:
fn scene() -> impl Scene {
(
Sprite::patch(|value, context| { value.image = "sprite.png".into() }),
Transform::patch(|value, context| { }),
)
}There was a problem hiding this comment.
So your solution to the issue of accidental ; is to keep it, or are you proposing that theres some other solution besides something like this?
i should have mentioned: the motivation for this was https://discord.com/channels/691052431525675048/691052431974465548/1498960812662849637
There was a problem hiding this comment.
There really is a core tension in Rust that () stands both for "empty tuple" and "no return"
There was a problem hiding this comment.
Yeah my argument is "yeah this is annoying, but it is a behavior inherent to Rust semantics". This is a problem everywhere the "trait implememented for tuples" pattern is used:
fn my_bundle() -> impl Bundle {
(Transform::default(), Name::new("hello"));
}It is also true for any trait that () has an impl for:
fn some_default() -> impl Default {
vec![10usize];
}…vyengine#24040) # Objective See for more details - bevyengine#24033 tldr: prevent accidental `;` in `-> impl SceneList`/`-> impl SceneList` functions from just being accepted ## Solution add `#[must_use]` to `SceneScope` and `SceneListScope` as recommended by @chescock ## Testing - [x] cargo test - [x] adding semicolon to a feathers `-> impl Scene` function to see the must use error idk, its an attribute, theres not much to test
…vyengine#24040) # Objective See for more details - bevyengine#24033 tldr: prevent accidental `;` in `-> impl SceneList`/`-> impl SceneList` functions from just being accepted ## Solution add `#[must_use]` to `SceneScope` and `SceneListScope` as recommended by @chescock ## Testing - [x] cargo test - [x] adding semicolon to a feathers `-> impl Scene` function to see the must use error idk, its an attribute, theres not much to test
Objective
Having
SceneandSceneListimplemented for()(unit) can be a source of confusion if users forget a;. See this already happening before bsn is released here: https://discord.com/channels/691052431525675048/691052431974465548/1498960812662849637The intention of this is to cause an error when
;is accidentally used at the end of a expression which evaluates toimpl Scene/impl SceneListSolution
The issue was that
()is both the value any expression ending in;evaluates to, and the value foremptythe corresponding traits were implemented for byall_tuples!By raising the start of
all_tuples!to 1 instead of 0, and implementing SceneList for a new unit struct, the implementation for()can be avoided. Luckily, everywhere where an empty Scene/SceneList was used in at least Bevy itself, it was done throughbsn_list!()which ultimately created the()by calling out tobevy_scene::macro_utils::auto_nest_tuple!, allowing me to replace the()there with the new unit struct.This means, empty BSN scenes and scene lists now consist of the new
EmptyTupleunit struct instead of(), and an accidental;now raises "error[E0277]: the trait bound(): bevy_scene::Sceneis not satisfied"Testing
cargo testbsn_list!()changed to emptybsn_list!()changed to empty