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: uninlined_format_args shouldn't inline panic! before 2021ed #9605

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Oct 8, 2022

Before 2021 edition, panic!("...") was not treated as a format string.
Clippy autofix of panic!("{}", foo) into panic!("{foo}") is incorrect.

changelog: [uninlined_format_args]: Do not inline panic! macros before 2021 edition

@rust-highfive
Copy link

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 8, 2022
@llogiq
Copy link
Contributor

llogiq commented Oct 11, 2022

The correct way to determine the edition of code is span.edition(). The cx.sess().opts.edition is just the default from the compiler IIRC.

Since I'm already looking into it, r? @llogiq (Sorry/You're welcome Manish for stealing the review 😄 )

@rust-highfive rust-highfive assigned llogiq and unassigned Manishearth Oct 11, 2022
@nyurik
Copy link
Contributor Author

nyurik commented Oct 11, 2022

The correct way to determine the edition of code is span.edition(). The cx.sess().opts.edition is just the default from the compiler IIRC.

@llogiq I searched all of clippy code - the only edition usage I see anywhere is cx.tcx.sess.edition() and cx.sess().opts.edition. I haven't seen any span.edition() anywhere - is it used in somewhere else? I tried to use cx.tcx.sess.edition() instead in my code, but it produces identical results (Edition 2018 is being treated as if it's 2021). I suspect the test needs to do something else to indicate its 2018?

@llogiq
Copy link
Contributor

llogiq commented Oct 11, 2022

I'm not sure, it may be a relatively recent addition.

@nyurik
Copy link
Contributor Author

nyurik commented Oct 11, 2022

@llogiq I tried this line instead, but it produces the same result. I think somehow my test is not being ran as edition2018 - can you take a look what it might be? I could try to add another file of course, but i was hoping the same file ran with multiple edition settings is a better option.

    if args.format_string.span.edition() < Edition2021 && is_panic(cx, def_id) {

@nyurik
Copy link
Contributor Author

nyurik commented Oct 12, 2022

@llogiq ok, so seems like the testing framework is broken when working with multiple editions in the same file. No idea why - and not really worth digging into it -- I simply created an additional 2018 file to test specifically for the older edition. And I also used your suggestion with span.edition. Ready to be reviewed/merged :)

@@ -0,0 +1,23 @@
// run-rustfix
// edition:2021
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default edition, so this shouldn't be required here. But that's not keeping me from r+ing it.

@llogiq
Copy link
Contributor

llogiq commented Oct 12, 2022

Thank you! And yes, I remember that compiletest only lets you set the edition once per file, just as you would only be able to set the edition once per crate in your code. This is not a bug, it's an intrinsic property of the compilation model.

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

📌 Commit 74ba7e1 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

⌛ Testing commit 74ba7e1 with merge 2d58817...

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 2d58817 to master...

@bors bors merged commit 2d58817 into rust-lang:master Oct 12, 2022
@nyurik nyurik deleted the fix-inline-edition branch October 13, 2022 00:22
bors added a commit that referenced this pull request Oct 13, 2022
Fix edition revision ui tests

#9605 had me wondering how the edition revision tests were working for `manual_assert` but not for `@nyurik,` but it turns out `manual_assert`'s tests weren't working either. I checked how `rust-lang/rust` does it and apparently it comes down to whitespace, `//[rev] edition:X` works 😬

Removes the revisions from `match_wild_err_arm` as I couldn't find any edition dependant behaviour there

r? `@llogiq`

changelog: none
bors added a commit that referenced this pull request Dec 10, 2022
uninlined_format_args: Ignore assert! and debug_assert! before 2021 edition

Similar to #9605, but for `assert!` and `debug_assert!` macros. ([non_fmt_panics lint triggers them](https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=18b20c408ec62a67f1503cd5d284424b))

changelog: [`uninlined_format_args`]: Do not inline `assert!` and `debug_assert!` macros before 2021 edition

r? `@llogiq`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants