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

Fix CI for Rust 1.72 #9562

Merged
merged 10 commits into from
Aug 25, 2023
Merged

Fix CI for Rust 1.72 #9562

merged 10 commits into from
Aug 25, 2023

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Aug 24, 2023

Objective

Rust 1.72.0 is now stable.

Notes

  • let-else formatting has arrived!

  • I chose to allow explicit_iter_loop due to explicit_iter_loop: x.iter_mut() vs &mut *x rust-lang/rust-clippy#11074.

    We didn't hit any of the false positives that prevent compilation, but fixing this did produce a lot of the "symbol soup" mentioned, e.g. for image in &mut *image_events {.

    Happy to undo this if there's consensus the other way.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change P-High This is particularly urgent, and deserves immediate attention labels Aug 24, 2023
rparrett and others added 2 commits August 24, 2023 08:20
…issing_deref.fail.stderr

Co-authored-by: François <mockersf@gmail.com>
…issing_deref.fail.stderr

Co-authored-by: François <mockersf@gmail.com>
@cart
Copy link
Member

cart commented Aug 24, 2023

Dang I really wanted one liners for cases like this:

image

@rparrett
Copy link
Contributor Author

rparrett commented Aug 24, 2023

Dang I really wanted one liners for cases like this:

Might be fixable: https://github.com/rust-lang/rustfmt/blob/v1.6.0/Configurations.md#single_line_let_else_max_width

@rparrett
Copy link
Contributor Author

rparrett commented Aug 24, 2023

I think that there must be some bug with single_line_let_else_max_width because even with it set very high, lots of seemingly narrow-enough lines get formatted on multiple lines.

edit: some discussion here: rust-lang/rustfmt#5849

@ickk
Copy link
Member

ickk commented Aug 24, 2023

Dang I really wanted one liners for cases like this:

image

@cart The style guide1 reads like it should just work as long as we remove the semicolon from after the return

Format the entire let-else statement on a single line if all the following are true:

  • the entire statement is short
  • the else block contains only a single-line expression and no statements
  • the else block contains no comments
  • the let statement components preceding the else block can be formatted on a single line
let Some(1) = opt else { return };

The reference2 grammar implies that any expression (including return expressions 3) which is followed by a semicolon becomes an ExpressionStatement, which would disqualify return; but not return.

Though it also says

An expression that consists of only a block expression or control flow expression, if used in a context where a statement is permitted, can omit the trailing semicolon. This can cause an ambiguity between it being parsed as a standalone statement and as a part of another expression; in this case, it is parsed as a statement

.. which makes it seem like sometimes expressions without a semicolon may be parsed as statements anyway, if they are a block or "control flow expression" (idk where that group is really defined).

@rparrett
Copy link
Contributor Author

This reads like it should just work as long as we remove the semicolon from after the return

It seems like that helps in the case of else { return };, but fmt adds a semicolon inside the block in the case of e.g. else { return true; };

@ickk
Copy link
Member

ickk commented Aug 24, 2023

It seems like that helps in the case of else { return };, but fmt adds a semicolon inside the block in the case of e.g. else { return true; };

That seems like it would be caused by this "unstable" rustfmt option.. https://rust-lang.github.io/rustfmt/?version=v1.6.0&search=semi#trailing_semicolon
rust-lang/rustfmt#3378

@rparrett
Copy link
Contributor Author

rparrett commented Aug 24, 2023

We seemingly can't set single_line_let_else_max_width greater than ~83 due to rust-lang/rustfmt#5404.

Doing so results in a spam of many lines of

`single_line_let_else_max_width` cannot have a value that exceeds `max_width`. `single_line_let_else_max_width` will be set to the same value as `max_width`

in the console while formatting.

examples/games/breakout.rs Show resolved Hide resolved
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

This needs to be merged ASAP

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 25, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 25, 2023
Merged via the queue into bevyengine:main with commit a788e31 Aug 25, 2023
21 checks passed
Trashtalk217 pushed a commit to Trashtalk217/bevy that referenced this pull request Aug 28, 2023
[Rust 1.72.0](https://blog.rust-lang.org/2023/08/24/Rust-1.72.0.html) is
now stable.

- `let-else` formatting has arrived!
- I chose to allow `explicit_iter_loop` due to
rust-lang/rust-clippy#11074.

We didn't hit any of the false positives that prevent compilation, but
fixing this did produce a lot of the "symbol soup" mentioned, e.g. `for
image in &mut *image_events {`.

  Happy to undo this if there's consensus the other way.

---------

Co-authored-by: François <mockersf@gmail.com>
Trashtalk217 added a commit to Trashtalk217/bevy that referenced this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change P-High This is particularly urgent, and deserves immediate attention S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants