-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add core
and alloc
over std
Lints
#15281
Changes from all commits
a939e93
120e9e0
abb01eb
864631f
145ab01
8a99654
9e4c516
273cd3a
6960fd1
389310a
46aa899
f3c95da
ecc8d33
5c7fa79
270a49f
50e34bd
a6780ba
e28ff2a
f62b660
6bbca4e
cdbc80a
fff160e
4704134
a9e1664
12b5c66
e7efe08
b89e0d1
7a2b7a5
de8b0d6
8e453c7
a377ecb
6534168
546abdc
5418ce2
5d9b12f
99bb6f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,15 +48,55 @@ ref_as_ptr = "warn" | |
# see: https://github.com/bevyengine/bevy/pull/15375#issuecomment-2366966219 | ||
too_long_first_doc_paragraph = "allow" | ||
|
||
std_instead_of_core = "warn" | ||
std_instead_of_alloc = "warn" | ||
alloc_instead_of_core = "warn" | ||
Comment on lines
+51
to
+53
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These lints direct Bevy contributors to use |
||
|
||
[workspace.lints.rust] | ||
missing_docs = "warn" | ||
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(docsrs_dep)'] } | ||
unsafe_code = "deny" | ||
unsafe_op_in_unsafe_fn = "warn" | ||
unused_qualifications = "warn" | ||
|
||
[lints] | ||
workspace = true | ||
Comment on lines
-58
to
-59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We explicitly don't want examples to be subject to the same linting requirements as the rest of Bevy, since it is encouraged that users rely on the Rust There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment to this effect here and in the workspace lints to reduce the risk of desync. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
# Unfortunately, cargo does not currently support overriding workspace lints | ||
# inside a particular crate. See https://github.com/rust-lang/cargo/issues/13157 | ||
# | ||
# We require an override for cases like `std_instead_of_core`, which are intended | ||
# for the library contributors and not for how users should consume Bevy. | ||
# To ensure examples aren't subject to these lints, below is a duplication of the | ||
# workspace lints, with the "overrides" applied. | ||
# | ||
# [lints] | ||
# workspace = true | ||
|
||
[lints.clippy] | ||
doc_markdown = "warn" | ||
manual_let_else = "warn" | ||
match_same_arms = "warn" | ||
redundant_closure_for_method_calls = "warn" | ||
redundant_else = "warn" | ||
semicolon_if_nothing_returned = "warn" | ||
type_complexity = "allow" | ||
undocumented_unsafe_blocks = "warn" | ||
unwrap_or_default = "warn" | ||
|
||
ptr_as_ptr = "warn" | ||
ptr_cast_constness = "warn" | ||
ref_as_ptr = "warn" | ||
|
||
too_long_first_doc_paragraph = "allow" | ||
|
||
std_instead_of_core = "allow" | ||
std_instead_of_alloc = "allow" | ||
alloc_instead_of_core = "allow" | ||
|
||
[lints.rust] | ||
missing_docs = "warn" | ||
unexpected_cfgs = { level = "warn", check-cfg = ['cfg(docsrs_dep)'] } | ||
unsafe_code = "deny" | ||
unsafe_op_in_unsafe_fn = "warn" | ||
unused_qualifications = "warn" | ||
|
||
[features] | ||
default = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -86,7 +86,7 @@ fn generic_bench<P: Copy>( | |
|
||
fn all_added_detection_generic<T: Component + Default>(group: &mut BenchGroup, entity_count: u32) { | ||
group.bench_function( | ||
format!("{}_entities_{}", entity_count, std::any::type_name::<T>()), | ||
format!("{}_entities_{}", entity_count, core::any::type_name::<T>()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most changes look like this, where some inline call to a |
||
|bencher| { | ||
bencher.iter_batched_ref( | ||
|| { | ||
|
@@ -110,8 +110,8 @@ fn all_added_detection_generic<T: Component + Default>(group: &mut BenchGroup, e | |
|
||
fn all_added_detection(criterion: &mut Criterion) { | ||
let mut group = criterion.benchmark_group("all_added_detection"); | ||
group.warm_up_time(std::time::Duration::from_millis(500)); | ||
group.measurement_time(std::time::Duration::from_secs(4)); | ||
group.warm_up_time(core::time::Duration::from_millis(500)); | ||
group.measurement_time(core::time::Duration::from_secs(4)); | ||
for &entity_count in ENTITIES_TO_BENCH_COUNT { | ||
generic_bench( | ||
&mut group, | ||
|
@@ -129,7 +129,7 @@ fn all_changed_detection_generic<T: Component + Default + BenchModify>( | |
entity_count: u32, | ||
) { | ||
group.bench_function( | ||
format!("{}_entities_{}", entity_count, std::any::type_name::<T>()), | ||
format!("{}_entities_{}", entity_count, core::any::type_name::<T>()), | ||
|bencher| { | ||
bencher.iter_batched_ref( | ||
|| { | ||
|
@@ -158,8 +158,8 @@ fn all_changed_detection_generic<T: Component + Default + BenchModify>( | |
|
||
fn all_changed_detection(criterion: &mut Criterion) { | ||
let mut group = criterion.benchmark_group("all_changed_detection"); | ||
group.warm_up_time(std::time::Duration::from_millis(500)); | ||
group.measurement_time(std::time::Duration::from_secs(4)); | ||
group.warm_up_time(core::time::Duration::from_millis(500)); | ||
group.measurement_time(core::time::Duration::from_secs(4)); | ||
for &entity_count in ENTITIES_TO_BENCH_COUNT { | ||
generic_bench( | ||
&mut group, | ||
|
@@ -179,7 +179,7 @@ fn few_changed_detection_generic<T: Component + Default + BenchModify>( | |
let ratio_to_modify = 0.1; | ||
let amount_to_modify = (entity_count as f32 * ratio_to_modify) as usize; | ||
group.bench_function( | ||
format!("{}_entities_{}", entity_count, std::any::type_name::<T>()), | ||
format!("{}_entities_{}", entity_count, core::any::type_name::<T>()), | ||
|bencher| { | ||
bencher.iter_batched_ref( | ||
|| { | ||
|
@@ -208,8 +208,8 @@ fn few_changed_detection_generic<T: Component + Default + BenchModify>( | |
|
||
fn few_changed_detection(criterion: &mut Criterion) { | ||
let mut group = criterion.benchmark_group("few_changed_detection"); | ||
group.warm_up_time(std::time::Duration::from_millis(500)); | ||
group.measurement_time(std::time::Duration::from_secs(4)); | ||
group.warm_up_time(core::time::Duration::from_millis(500)); | ||
group.measurement_time(core::time::Duration::from_secs(4)); | ||
for &entity_count in ENTITIES_TO_BENCH_COUNT { | ||
generic_bench( | ||
&mut group, | ||
|
@@ -227,7 +227,7 @@ fn none_changed_detection_generic<T: Component + Default>( | |
entity_count: u32, | ||
) { | ||
group.bench_function( | ||
format!("{}_entities_{}", entity_count, std::any::type_name::<T>()), | ||
format!("{}_entities_{}", entity_count, core::any::type_name::<T>()), | ||
|bencher| { | ||
bencher.iter_batched_ref( | ||
|| { | ||
|
@@ -252,8 +252,8 @@ fn none_changed_detection_generic<T: Component + Default>( | |
|
||
fn none_changed_detection(criterion: &mut Criterion) { | ||
let mut group = criterion.benchmark_group("none_changed_detection"); | ||
group.warm_up_time(std::time::Duration::from_millis(500)); | ||
group.measurement_time(std::time::Duration::from_secs(4)); | ||
group.warm_up_time(core::time::Duration::from_millis(500)); | ||
group.measurement_time(core::time::Duration::from_secs(4)); | ||
for &entity_count in ENTITIES_TO_BENCH_COUNT { | ||
generic_bench( | ||
&mut group, | ||
|
@@ -308,7 +308,7 @@ fn multiple_archetype_none_changed_detection_generic<T: Component + Default + Be | |
"{}_archetypes_{}_entities_{}", | ||
archetype_count, | ||
entity_count, | ||
std::any::type_name::<T>() | ||
core::any::type_name::<T>() | ||
), | ||
|bencher| { | ||
bencher.iter_batched_ref( | ||
|
@@ -356,8 +356,8 @@ fn multiple_archetype_none_changed_detection_generic<T: Component + Default + Be | |
|
||
fn multiple_archetype_none_changed_detection(criterion: &mut Criterion) { | ||
let mut group = criterion.benchmark_group("multiple_archetypes_none_changed_detection"); | ||
group.warm_up_time(std::time::Duration::from_millis(800)); | ||
group.measurement_time(std::time::Duration::from_secs(8)); | ||
group.warm_up_time(core::time::Duration::from_millis(800)); | ||
group.measurement_time(core::time::Duration::from_secs(8)); | ||
for archetype_count in [5, 20, 100] { | ||
for entity_count in [10, 100, 1000, 10000] { | ||
multiple_archetype_none_changed_detection_generic::<Table>( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downsides I can see are:
extern crate alloc
in most crates now).std
.As such, before making such a change, it would be good to qualify exactly what it wins us. For example, getting closer to some potential
no_std
Bevy future doesn't do us much if there are true blockers that prevent us from finishing that journey. Same goes for console support (which is currently phrased like "this might help").Before merging, I'd want satisfying answers to:
I also question the viability of this effort for things like consoles when the majority of plugins and user code will still target
std
. Seems like anstd
console port (even if it starts as a partial / shimmed port) would be preferable from an ecosystem perspective.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to look at this! As an immediate response, here's what I personally think in regards to those listed downsides:
Agreed, this will affect all contributors for Bevy going forward. However, I believe this is a relatively minor impact, as
clippy
can suggest the exact changes required to pass these lints. In effect, contributors can continue to writestd
-reliant code, but on completion they'll fix this lint the same way we already do for things likefmt
andtaplo
.This is true, effectively every crate root (
lib.rs
) will includeextern crate alloc
. However, it is only a single line of code per crate, so I think, while annoying, its real impact is negligible. Especially considering this PR adds the lines already, so contributors will only encounter this boilerplate when creating a new Bevy crate.This is true and I don't have a meaningful rebuttal; this will add one layer of indirection between user and contributor code. What I would say is that we already have that for things like
bevy_reflect
being feature-gated. The majority of users write code for Bevy assumingbevy_reflect
is available, but contributors can't make that same assumption.I personally don't have access to any modern console SDKs so I can't comment on the necessity for a
no_std
Bevy for platforms like the Nintendo Switch, Xbox Series, PS5, etc. However, I can say that for the homebrew and retro community,no_std
is an absolute must. For some context on my personal motivations here, I'm wanting to use theagb
crate with Bevy to create some games for the GameBoy Advance. To do this, I need the following crates to beno_std
at a minimum:bevy_ptr
bevy_utils
bevy_tasks
bevy_ecs
bevy_app
Out of the above 5 crates,
bevy_ptr
andbevy_utils
are alreadyno_std
, and I have a draftno_std
forbevy_tasks
already published.bevy_app
is trivial to makeno_std
, as it only uses the standard library forthiserror
,Mutex
, andpanic
capturing, all of which have suitableno_std
replacements (or just not-applicable in the case ofpanic
capturing). This leavesbevy_ecs
as the only core crate remaining, and I'm already about half-way through a port of that too, with the biggest issues being non-Send
resources (requires access tostd::thread
) and the schedule graph using a single type and algorithm frompetgraph
(GraphMap
).Once the above changes are made, I will be able to use Bevy on any platform, including the GameBoy Advance. What's more, I'd be able to use the high level abstractions like systems, queries, and the
App
and plugin infrastructure too, making Bevy the single best engine for retro-consoles.With the above context, here are my answers to your three questions:
thiserror
petgraph
'sGraphMap
data structureSend
resourcesbevy_ecs
andbevy_app
to include astd
featureObviously this excludes other aspects of Bevy, (
bevy_time
, etc.) which I want to beno_std
, but everything else is optional,bevy_app
andbevy_ecs
are the minimum to provide an end-user appropriateno_std
experience.To make a crate like
bevy_ecs
no_std
, the first step will be this very refactor, and then it'll be meaningful work (e.g.,petgraph
, etc.). The problem is that any PR which attempts to makebevy_ecs
no_std
will be made vastly more controversial by mixing this PR's work and their actual changes to Bevy's code, making review substantially harder and more likely to fail. By applying this lint in its own PR and across the workspace, we remove that controversy and noise from future discussions aroundno_std
Bevy, and we keep all of Bevy consistent ("Oh you can't write that code here, we have thestd
lint on").Yes, but I believe this introduces the very inconsistency in Bevy that we'd like to avoid. These lints make no restrictions around what code we write, or even how they're written, only the names of
core
andalloc
types. These lints provide a consistent experience for all Bevy contributions, regardless of whether you're in ano_std
crate or not. Since Bevy already has twono_std
crates, these lints will make working on those crates consistent with the rest of the library.I see this as a chicken-and-egg problem; Bevy's plugins are
std
because Bevy isstd
. You actually can't write ano_std
Bevy plugin right now because the required traits are "stuck" inside anstd
-only crate. I can personally attest to my intention to make abevy_agb
plugin to allow working on the GameBoy Advance, and I'm certain that other community members would have similar excitement around a new area of Bevy to explore. Bare-metal games on the Raspberry Pi, Playdate support, embedded projects usingbevy_ecs
as a database, etc. are projects I have seen discussed in the Bevy Discord as fantastical, but I genuinely believe are achingly close to realisation.For the modern consoles, definitely, but this would not be viable for anything retro. Additionally, with Bevy relying on the whole
std
(right now), knowing how much of thestd
library you need to port is difficult. If Bevy's reliance on the standard library was reduced to what it needs, then that work also becomes more actionable.I apologise for the long response, especially considering just how large this PR itself is, and I thank you for taking the time to read it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this if possible, I think with
core::Error
stabilizing we can actually fix this upstream.Not long for this world frankly, the ECS folks kinda hate petgraph
Feature-flagging this is fine
Very doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, to provide some hard evidence for how close
no_std
Bevy is, I have made some rough ports ofbevy_ecs
andbevy_app
on this branch. All that really had to change was adding anInstant
type appropriate forno_std
environments, and portingpetgraph
's required parts to a custom solution. That in of itself was actually just an almost straight copy-paste ofpetgraph
's implementation (simplified), since it was already almostno_std
, just needed to disable a couple features we weren't using.These changes allow this application to compile on
x86_64-unknown-none
, bare-metal x86:`Cargo.toml`
`main.rs`
After applying the lints in this PR, it's about another 3000 diff-lines to make
bevy
usable inno_std
applications with all the high-level functionality people rely on Bevy for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a big win for bevy as the core 😉 of the ecosystem for a very small price.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'm convinced. Lets do this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant! I'm going to resolve the merge conflicts now so hopefully this could be merged soon!