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

Allow also allow literals as first item of single line let chain #6492

Merged
merged 1 commit into from
Mar 12, 2025

Conversation

est31
Copy link
Member

@est31 est31 commented Mar 8, 2025

Also allow literals as the first item in a single line let chain, not just idents. This will most probably not occur in most real-world code, but it makes things a little bit more consistent. While rustfmt allows any literal type, like also string literals, those will not create valid rust code. Only bool literals will.

Implements suggestion from rust-lang/rust#110568 (comment) .

@est31 est31 changed the title Allow also allow bool literals as first item of single line let chain Allow also allow literals as first item of single line let chain Mar 8, 2025
@est31
Copy link
Member Author

est31 commented Mar 8, 2025

@calebcartwright @joshtriplett I found this suggestion when I read over rust-lang/rust#110568 (comment) . From what I can tell, it is consensus of the style team to allow also literals (outside of bool literals, no literal is really valid here).

@joshtriplett
Copy link
Member

@est31 Confirmed; we don't want to put false && or true && on a line by itself.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

@est31 A few minor things. I'd like the code to be a little more explicit about only allowing bool literals. Also, let-chains are still unstable, right?

src/pairs.rs Outdated
match &expr.kind {
ast::ExprKind::Path(None, path) if path.segments.len() == 1 => true,
ast::ExprKind::Lit(_) => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the match more explicit for bool literals.

Copy link
Member Author

Choose a reason for hiding this comment

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

type checking would reject any literal that's not a bool literal, but shrug, changed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

rustfmt operates at an earlier stage of compilation, so if it parses and could be in the AST then we need to be mindful of that.

src/pairs.rs Outdated
@@ -266,13 +266,14 @@ struct PairList<'a, 'b, T: Rewrite> {
span: Span,
}

fn is_ident(expr: &ast::Expr) -> bool {
fn is_ident_or_lit(expr: &ast::Expr) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think we should be more explicit about bool literals.

Suggested change
fn is_ident_or_lit(expr: &ast::Expr) -> bool {
fn is_ident_or_bool_lit(expr: &ast::Expr) -> bool {

Might even be helpful to link to text from the style guide explaining the rules if they've been codified yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

they have not been codified yet, all there is to link is discussion threads :).

Copy link
Contributor

Choose a reason for hiding this comment

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

All good then. No need to link to discussions.

@est31
Copy link
Member Author

est31 commented Mar 12, 2025

let-chains are still unstable, right?

indeed they are, and this PR is one of the things I am doing to get them stabilized: to get all open questions resolved around formatting.

@est31 est31 requested a review from ytmimi March 12, 2025 00:55
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 12, 2025

Just ran our Diff-Check job. Looks like there will be two clippy test cases that'll change once we sync these changes with rust-lang/rust:

Diff in /tmp/rust-lang-rust-W6U1GxGa/src/tools/clippy/tests/ui/needless_late_init.rs:246:
     }
 
     let x;
-    if true
-        && let Some(n) = Some("let chains too")
-    {
+    if true && let Some(n) = Some("let chains too") {
         x = 1;
     } else {
         x = 2;
Diff in /tmp/rust-lang-rust-W6U1GxGa/src/tools/clippy/tests/ui/needless_if.rs:46:
     if let true = true
         && true
     {}
-    if true
-        && let true = true
-    {}
+    if true && let true = true {}
     // Can lint nested `if let`s
     if {
         //~^ needless_if

@ytmimi ytmimi merged commit ee329d3 into rust-lang:master Mar 12, 2025
26 checks passed
@ytmimi ytmimi added release-notes Needs an associated changelog entry and removed pr-waiting-on-author labels Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Needs an associated changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants