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

Upgrade to Rust edition 2024 #17960

Closed
andriyDev opened this issue Feb 21, 2025 · 3 comments · Fixed by #17967
Closed

Upgrade to Rust edition 2024 #17960

andriyDev opened this issue Feb 21, 2025 · 3 comments · Fixed by #17967
Labels
A-Cross-Cutting Impacts the entire engine C-Code-Quality A section of code that is hard to understand or change D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon

Comments

@andriyDev
Copy link
Contributor

Rust edition 2024 has recently released with some new features (my personal favourite is the combined doc tests for speedy testing). We should upgrade to the new edition!

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! A-Cross-Cutting Impacts the entire engine X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Feb 21, 2025
@BD103
Copy link
Member

BD103 commented Feb 21, 2025

I personally think this should be split into multiple PRs to ease the transition. Some crates are trivial to migrate, while others are not (bevy_ecs due to the Never type fallback change).

Not to mention, the rustfmt changes should be done at the very end and merged very quickly, to avoid breaking others.

Resources

@bushrat011899
Copy link
Contributor

I personally think this should be split into multiple PRs to ease the transition. Some crates are trivial to migrate, while others are not (bevy_ecs due to the Never type fallback change).

Totally agree, but I have got a full PR for all the changes done in #17967. It ended up being +/- 600 lines. I don't mind the idea of splitting this up into smaller PRs, but if this is acceptable the "hard work" has been done.

Not to mention, the rustfmt changes should be done at the very end and merged very quickly, to avoid breaking others.

Absolutely. The change to import ordering alone makes that a +/- 3k diff straight away. I've postponed that issue by overriding the style_edition in rustfmt.toml to continue using 2021 formatting. A followup PR switching to 2024 would be trivial.

@BD103
Copy link
Member

BD103 commented Feb 22, 2025

Totally agree, but I have got a full PR for all the changes done in #17967. It ended up being +/- 600 lines. I don't mind the idea of splitting this up into smaller PRs, but if this is acceptable the "hard work" has been done.

I'm surprised by the small diff, honestly, that's not too bad! It probably doesn't need to be split in that case :)

github-merge-queue bot pushed a commit that referenced this issue Feb 24, 2025
# Objective

- Fixes #17960

## Solution

- Followed the [edition upgrade
guide](https://doc.rust-lang.org/edition-guide/editions/transitioning-an-existing-project-to-a-new-edition.html)

## Testing

- CI

---

## Summary of Changes

### Documentation Indentation

When using lists in documentation, proper indentation is now linted for.
This means subsequent lines within the same list item must start at the
same indentation level as the item.

```rust
/* Valid */
/// - Item 1
///   Run-on sentence.
/// - Item 2
struct Foo;

/* Invalid */
/// - Item 1
///     Run-on sentence.
/// - Item 2
struct Foo;
```

### Implicit `!` to `()` Conversion

`!` (the never return type, returned by `panic!`, etc.) no longer
implicitly converts to `()`. This is particularly painful for systems
with `todo!` or `panic!` statements, as they will no longer be functions
returning `()` (or `Result<()>`), making them invalid systems for
functions like `add_systems`. The ideal fix would be to accept functions
returning `!` (or rather, _not_ returning), but this is blocked on the
[stabilisation of the `!` type
itself](https://doc.rust-lang.org/std/primitive.never.html), which is
not done.

The "simple" fix would be to add an explicit `-> ()` to system
signatures (e.g., `|| { todo!() }` becomes `|| -> () { todo!() }`).
However, this is _also_ banned, as there is an existing lint which (IMO,
incorrectly) marks this as an unnecessary annotation.

So, the "fix" (read: workaround) is to put these kinds of `|| -> ! { ...
}` closuers into variables and give the variable an explicit type (e.g.,
`fn()`).

```rust
// Valid
let system: fn() = || todo!("Not implemented yet!");
app.add_systems(..., system);

// Invalid
app.add_systems(..., || todo!("Not implemented yet!"));
```

### Temporary Variable Lifetimes

The order in which temporary variables are dropped has changed. The
simple fix here is _usually_ to just assign temporaries to a named
variable before use.

### `gen` is a keyword

We can no longer use the name `gen` as it is reserved for a future
generator syntax. This involved replacing uses of the name `gen` with
`r#gen` (the raw-identifier syntax).

### Formatting has changed

Use statements have had the order of imports changed, causing a
substantial +/-3,000 diff when applied. For now, I have opted-out of
this change by amending `rustfmt.toml`

```toml
style_edition = "2021"
```

This preserves the original formatting for now, reducing the size of
this PR. It would be a simple followup to update this to 2024 and run
`cargo fmt`.

### New `use<>` Opt-Out Syntax

Lifetimes are now implicitly included in RPIT types. There was a handful
of instances where it needed to be added to satisfy the borrow checker,
but there may be more cases where it _should_ be added to avoid
breakages in user code.

### `MyUnitStruct { .. }` is an invalid pattern

Previously, you could match against unit structs (and unit enum
variants) with a `{ .. }` destructuring. This is no longer valid.

### Pretty much every use of `ref` and `mut` are gone

Pattern binding has changed to the point where these terms are largely
unused now. They still serve a purpose, but it is far more niche now.

### `iter::repeat(...).take(...)` is bad

New lint recommends using the more explicit `iter::repeat_n(..., ...)`
instead.

## Migration Guide

The lifetimes of functions using return-position impl-trait (RPIT) are
likely _more_ conservative than they had been previously. If you
encounter lifetime issues with such a function, please create an issue
to investigate the addition of `+ use<...>`.

## Notes

- Check the individual commits for a clearer breakdown for what
_actually_ changed.

---------

Co-authored-by: François Mockers <francois.mockers@vleue.com>
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-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants