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

trailing_semicolon = false changes the meaning of code, non-idempotent #5797

Closed
digama0 opened this issue Jun 23, 2023 · 3 comments · Fixed by #5798
Closed

trailing_semicolon = false changes the meaning of code, non-idempotent #5797

digama0 opened this issue Jun 23, 2023 · 3 comments · Fixed by #5798
Labels
bug Panic, non-idempotency, invalid code, etc.

Comments

@digama0
Copy link

digama0 commented Jun 23, 2023

The trailing_semicolon = false option, when run on this code:

fn foo() {}
fn main() {
    return;
    foo()
}

turns it into

fn foo() {}
fn main() {
    return
    foo()
}

which now runs foo() where it did not before, as a second run of rustfmt will make clearer:

fn foo() {}
fn main() {
    return foo()
}

Observed on rustfmt 1.5.3-nightly (065a1f5 2023-06-21).

@calebcartwright
Copy link
Member

thanks for the report! tagging tracking issue: #3378

@calebcartwright
Copy link
Member

calebcartwright commented Jun 23, 2023

@CosmicHorrorDev & @HarrisonHemstreet this is another one that should be a relatively quick fix if either of you are interested in working on it.

As discussed in chat don't feel obligated to work on any of these, we're just tagging things as they pop up if we think could be good opportunities for you.

I'm going to be a bit more lightweight in initial guidance on this one to give you more opportunity to poke around, so all I'll say for now is that you may want to start around here:

rustfmt/src/utils.rs

Lines 295 to 309 in 9386b32

pub(crate) fn semicolon_for_stmt(context: &RewriteContext<'_>, stmt: &ast::Stmt) -> bool {
match stmt.kind {
ast::StmtKind::Semi(ref expr) => match expr.kind {
ast::ExprKind::While(..) | ast::ExprKind::Loop(..) | ast::ExprKind::ForLoop(..) => {
false
}
ast::ExprKind::Break(..) | ast::ExprKind::Continue(..) | ast::ExprKind::Ret(..) => {
context.config.trailing_semicolon()
}
_ => true,
},
ast::StmtKind::Expr(..) => false,
_ => true,
}
}

@calebcartwright calebcartwright added the bug Panic, non-idempotency, invalid code, etc. label Jun 23, 2023
@CosmicHorrorDev
Copy link
Contributor

I'm up for digging into it this weekend if @HarrisonHemstreet is fine with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants