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

Add core and alloc over std Lints #15281

Merged
merged 36 commits into from
Sep 27, 2024

Conversation

bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Sep 18, 2024

Objective

Solution

  • Added the following lints to the workspace:
    • std_instead_of_core
    • std_instead_of_alloc
    • alloc_instead_of_core
  • Used cargo +nightly fmt with item level use formatting to split all use statements into single items.
  • Used cargo clippy --workspace --all-targets --all-features --fix --allow-dirty to attempt to resolve the new linting issues, and intervened where the lint was unable to resolve the issue automatically (usually due to needing an extern crate alloc; statement in a crate root).
  • Manually removed certain uses of std where negative feature gating prevented --all-features from finding the offending uses.
  • Used cargo +nightly fmt with crate level use formatting to re-merge all use statements matching Bevy's previous styling.
  • Manually fixed cases where the fmt tool could not re-merge use statements due to conditional compilation attributes.

Testing

  • Ran CI locally

Migration Guide

The MSRV is now 1.81. Please update to this version or higher.

Notes

  • This is a massive change to try and push through, which is why I've outlined the semi-automatic steps I used to create this PR, in case this fails and someone else tries again in the future.
  • Making this change has no impact on user code, but does mean Bevy contributors will be warned to use core and alloc instead of std where possible.
  • This lint is a critical first step towards investigating no_std options for Bevy.

@bushrat011899 bushrat011899 added C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR A-Cross-Cutting Impacts the entire engine D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 18, 2024
@bushrat011899 bushrat011899 marked this pull request as draft September 18, 2024 06:31
Copy link
Contributor

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

@bushrat011899 bushrat011899 marked this pull request as ready for review September 18, 2024 09:17
@alice-i-cecile
Copy link
Member

I think we should do this. This is easy to enforce and for contributors to adapt to, and the auto-fix is particularly nice. Import consistency is a nice side benefit too.

I'm not convinced that Bevy will ever be able to be fully no-std, but doing this now will make supporting weird platforms (notably consoles) in the future much easier and give us a better sense of how far away we are, what changes we need upstream and what parts of the standard library the teams will need to reimplement for their platform.

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 18, 2024
@@ -45,15 +45,53 @@ ptr_as_ptr = "warn"
ptr_cast_constness = "warn"
ref_as_ptr = "warn"

std_instead_of_core = "warn"
Copy link
Member

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:

  1. This is a large / sweeping change that permanently changes how we all write Bevy code.
  2. This introduces slightly more boilerplate (we need extern crate alloc in most crates now).
  3. This takes us further from the "internal Bevy code looks like user-facing Bevy code" principle, because the majority of users / plugins can (and should) continue to use 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:

  1. With this change, what are the remaining blockers to "no_std Bevy"
  2. With just this change, what scenarios are we unlocking?
  3. For (2), can we unlock those scenarios with targeted ports of specific crates? (ex: doing this for bevy_ecs)

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 an std console port (even if it starts as a partial / shimmed port) would be preferable from an ecosystem perspective.

Copy link
Contributor Author

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:

  1. This is a large / sweeping change that permanently changes how we all write Bevy code.

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 write std-reliant code, but on completion they'll fix this lint the same way we already do for things like fmt and taplo.

  1. This introduces slightly more boilerplate (we need extern crate alloc in most crates now).

This is true, effectively every crate root (lib.rs) will include extern 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.

  1. This takes us further from the "internal Bevy code looks like user-facing Bevy code" principle, because the majority of users / plugins can (and should) continue to use std.

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 assuming bevy_reflect is available, but contributors can't make that same assumption.

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").

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 the agb crate with Bevy to create some games for the GameBoy Advance. To do this, I need the following crates to be no_std at a minimum:

  • bevy_ptr
  • bevy_utils
  • bevy_tasks
  • bevy_ecs
  • bevy_app

Out of the above 5 crates, bevy_ptr and bevy_utils are already no_std, and I have a draft no_std for bevy_tasks already published. bevy_app is trivial to make no_std, as it only uses the standard library for thiserror, Mutex, and panic capturing, all of which have suitable no_std replacements (or just not-applicable in the case of panic capturing). This leaves bevy_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 to std::thread) and the schedule graph using a single type and algorithm from petgraph (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:

  1. With this change, what are the remaining blockers to "no_std Bevy"
  1. thiserror
  2. petgraph's GraphMap data structure
  3. non-Send resources
  4. Refactoring bevy_ecs and bevy_app to include a std feature

Obviously this excludes other aspects of Bevy, (bevy_time, etc.) which I want to be no_std, but everything else is optional, bevy_app and bevy_ecs are the minimum to provide an end-user appropriate no_std experience.

  1. With just this change, what scenarios are we unlocking?

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 make bevy_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 around no_std Bevy, and we keep all of Bevy consistent ("Oh you can't write that code here, we have the std lint on").

  1. For (2), can we unlock those scenarios with targeted ports of specific crates? (ex: doing this for bevy_ecs)

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 and alloc types. These lints provide a consistent experience for all Bevy contributions, regardless of whether you're in a no_std crate or not. Since Bevy already has two no_std crates, these lints will make working on those crates consistent with the rest of the library.

I also question the viability of this effort for things like consoles when the majority of plugins and user code will still target std.

I see this as a chicken-and-egg problem; Bevy's plugins are std because Bevy is std. You actually can't write a no_std Bevy plugin right now because the required traits are "stuck" inside an std-only crate. I can personally attest to my intention to make a bevy_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 using bevy_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.

Seems like an std console port (even if it starts as a partial / shimmed port) would be preferable from an ecosystem perspective.

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 the std 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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thiserror

I'd like to keep this if possible, I think with core::Error stabilizing we can actually fix this upstream.

petgraph's GraphMap data structure

Not long for this world frankly, the ECS folks kinda hate petgraph

non-Send resources

Feature-flagging this is fine

Refactoring bevy_ecs and bevy_app to include a std feature

Very doable.

Copy link
Contributor Author

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 of bevy_ecs and bevy_app on this branch. All that really had to change was adding an Instant type appropriate for no_std environments, and porting petgraph's required parts to a custom solution. That in of itself was actually just an almost straight copy-paste of petgraph's implementation (simplified), since it was already almost no_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`
[package]
name = "bevy_no_std_test"
version = "0.1.0"
edition = "2021"

[dependencies]
bevy_app = { path = "../bevy_app", version = "0.15.0-dev", default-features = false }
bevy_ecs = { path = "../bevy_ecs", version = "0.15.0-dev", default-features = false }

[profile.dev]
opt-level = 3
debug = true
panic = "abort"
`main.rs`
//! Demonstrates a [`no_std`] Bevy application.

#![allow(unsafe_code)]
#![no_std]
#![no_main]

// Since we don't have access to `std`, and the `core` library can't assume there's
// a global allocator, we have to explicitly include it using `extern crate`.
extern crate alloc;

// Explicitly importing bevy sub-crates because I haven't threaded the `std` feature
// through bevy_internal and then bevy proper.
use bevy_app::prelude::*;
use bevy_ecs::prelude::*;

#[derive(Resource, Default)]
struct MyResource {
    hello_world: bool,
}

#[derive(Component)]
struct Player;

fn main() {
    App::empty()
        .init_resource::<MyResource>()
        .add_systems(Update, |my_resource: Res<MyResource>| {
            assert_eq!(my_resource.hello_world, false);
        })
        .add_systems(Startup, |mut commands: Commands| {
            commands.spawn(Player);
        })
        .add_systems(Update, |query: Query<&Player>| {
            assert_eq!(query.iter().count(), 1);
        })
        .run();
}

/// You cannot rely on [`main`] being called for you in a `no_std` environment.
/// The exact name of your start function will depend on the platform you're developing for.
/// It's convention to name this function `_start`, and we use `#[no_mangle]` to ensure
/// the Rust compiler doesn't change this name.
#[no_mangle]
pub extern "C" fn _start() -> ! {
    // Our start function is just going to call main, and if main exits we'll
    // just loop forever. Quitting in a `no_std` context isn't defined, since
    // you might be running on bare-metal hardware without power management!
    main();
    loop {}
}

/// Bevy requires access to a global allocator, another item which is platform specific
/// and so must be setup manually.
#[global_allocator]
static ALLOCATOR: allocator::Dummy = allocator::Dummy;

/// Obviously _your_ code will never panic. But, Rust still requires setting up
/// a panic handler _just in case_.
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
    loop {}
}

/// Custom allocator for our no_std environment
mod allocator {
    use alloc::alloc::{GlobalAlloc, Layout};
    use core::ptr::null_mut;

    /// This is a demonstration allocator and is _intentionally_ broken.
    pub struct Dummy;

    unsafe impl GlobalAlloc for Dummy {
        unsafe fn alloc(&self, _layout: Layout) -> *mut u8 {
            null_mut()
        }

        unsafe fn dealloc(&self, _ptr: *mut u8, _layout: Layout) {
            panic!("dealloc should be never called")
        }
    }
}

After applying the lints in this PR, it's about another 3000 diff-lines to make bevy usable in no_std applications with all the high-level functionality people rely on Bevy for.

Copy link
Contributor

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.

Copy link
Member

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!

Copy link
Contributor Author

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!

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Sep 26, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-SME Decision or review from an SME is required labels Sep 26, 2024
@alice-i-cecile
Copy link
Member

@bushrat011899 Ping me when merge conflicts are resolved and I'll get this in ASAP.

@mockersf mockersf dismissed their stale review September 27, 2024 00:30

changes have been done

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
Tried to import `CString` from `core::ffi`, when it should come from `alloc::ffi`. Onlt detected during merge attempt as Android compilation isn't included in the standard CI.
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 27, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 27, 2024
Merged via the queue into bevyengine:main with commit d70595b Sep 27, 2024
30 checks passed
@andriyDev andriyDev mentioned this pull request Sep 27, 2024
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1698 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefer core and alloc over std where possible
7 participants