New lint `if_then_panic` by Labelray · Pull Request #7669 · rust-lang/rust-clippy

Labelray · 2021-09-14T08:33:55Z

changelog: add the new lint [if_then_panic]
fix #7645

rust-highfive · 2021-09-14T08:33:58Z

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

camsteffen · 2021-09-14T13:34:11Z

Use cx.sess().source_map().is_multiline(span) to make sure the condition fits on on line as noted in the issue. Add a negative test for this.

I think providing a code suggestion is important for this lint. The fix may not be intuitive and people may not be aware that assert! accepts a message and formatting args. We probably need PanicMacroExpn similar to FormatExpn.

clippy_utils/src/higher.rs

camsteffen · 2021-09-18T15:30:09Z

I'll leave it to @giraffate do to a full review. But note I still don't see the multi-line check. @Labelray don't hesitate to ask if something doesn't make sense!

Labelray · 2021-09-18T15:37:16Z

I'll leave it to @giraffate do to a full review. But note I still don't see the multi-line check. @Labelray don't hesitate to ask if something doesn't make sense!

@camsteffen I didn't check the number of lines but I checked the number of statements. I think it is better.

camsteffen · 2021-09-18T15:45:09Z

A long condition can be a single statement.

if some_function(foo, bar, baz) == another_function(hi, whats, up)
    && some_function(a, b, c) == another_function(a, b, c) {
    panic!();
}

clippy_lints/src/if_then_panic.rs

giraffate

I made a comment but overall looks good, thanks!

giraffate · 2021-09-24T00:18:17Z

tests/ui/fallible_impl_from.stderr

+error: only a `panic!` in `if`-then statement
+  --> $DIR/fallible_impl_from.rs:28:9
+   |
+LL | /         if i != 42 {
+LL | |             panic!();
+LL | |         }
+   | |_________^ help: try: `assert!(!i != 42, "explicit panic");`
+   |
+   = note: `-D clippy::if-then-panic` implied by `-D warnings`
+


Can you add #![allow(clippy::if_then_panic)] and remove these warnings in this test file? It looks a little confusing.

If this suggestion even valid?

!i != 42 is (!i) != 42 which is not what is intended to be suggested here. It should be !(i != 42).

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a84683ba815f737b90acb285876d2365

Though, already reported: #7731

giraffate

Can the conflict be resolved?

giraffate · 2021-09-24T13:55:13Z

We follow a no merge-commit policy. Can you do a rebase, not a merge?

giraffate · 2021-09-24T14:35:53Z

@bors r+

Thanks!

bors · 2021-09-24T14:35:55Z

📌 Commit 543b638 has been approved by giraffate

bors · 2021-09-24T14:43:55Z

⌛ Testing commit 543b638 with merge fb61d04...

bors · 2021-09-24T14:56:28Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing fb61d04 to master...

Most of the changes are for the new if-then-panic lint added in rust-lang/rust-clippy#7669. Error messages look like: error: only a `panic!` in `if`-then statement --> zenith_utils/src/lsn.rs:195:9 | 195 | / if prev.checked_add(val).is_none() { 196 | | panic!("AtomicLsn overflow"); 197 | | } | |_________^ help: try: `assert!(!prev.checked_add(val).is_none(), "AtomicLsn overflow");` | = note: `-D clippy::if-then-panic` implied by `-D warnings` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#if_then_panic

Most of the changes are for the new if-then-panic lint added in rust-lang/rust-clippy#7669.

Jasperav · 2021-10-19T18:15:14Z

@camsteffen @giraffate This issue causes regression: panic! can be catch with std::panic::catch_unwind, but assert! not. This broke my code and can break other people's code as well if panic! is unconditionally replaced with assert! (when using the catch_unwind since there won't be a catch but the whole program is terminated), so it should not be suggested as a 'fix'.

I would suggest reverting this lint or don't make it active by default (not sure if this is possible)

Jasperav · 2021-10-19T19:27:04Z

This lint also transformed this code:

let x = 10;
if x == 0 {
    panic!("fail");
}

to

let x = 10;
assert!(!x == 0, "fail");

which... is weird. Why negate an integer? This also breaks my code since this assertion is now triggered, while the initial code wasn't being triggered.

camsteffen · 2021-10-19T21:41:15Z

Hi @Jasperav, thanks for the feedback.

This issue causes regression: panic! can be catch with std::panic::catch_unwind, but assert! not.

I don't think this is true, unless I misunderstood. See playground.

I would suggest reverting this lint or don't make it active by default

The lint is being moved to pedantic (off by default) in #7810.

which... is weird. Why negate an integer?

This should be fixed from #7741.

Jasperav · 2021-10-20T06:55:39Z

Edit: apparently, it can indeed catch assertions. Not sure what went wrong with my code.

rust-highfive assigned giraffate Sep 14, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 14, 2021

Labelray force-pushed the if_then_panic branch 4 times, most recently from bf3194e to 5761bb7 Compare September 14, 2021 09:15

Labelray force-pushed the if_then_panic branch from 5761bb7 to 0e5ccd7 Compare September 17, 2021 02:30

camsteffen reviewed Sep 17, 2021

View reviewed changes

clippy_utils/src/higher.rs Outdated Show resolved Hide resolved

Labelray force-pushed the if_then_panic branch from 0e5ccd7 to 9e85dc8 Compare September 18, 2021 15:12

Labelray force-pushed the if_then_panic branch from 9e85dc8 to 73040c3 Compare September 19, 2021 04:33

giraffate reviewed Sep 21, 2021

View reviewed changes

clippy_lints/src/if_then_panic.rs Show resolved Hide resolved

clippy_lints/src/if_then_panic.rs Outdated Show resolved Hide resolved

Labelray force-pushed the if_then_panic branch 3 times, most recently from 0d3c4f9 to 44ad749 Compare September 22, 2021 00:45

giraffate reviewed Sep 24, 2021

View reviewed changes

Labelray force-pushed the if_then_panic branch from 44ad749 to 5f750a8 Compare September 24, 2021 11:13

giraffate approved these changes Sep 24, 2021

View reviewed changes

Labelray force-pushed the if_then_panic branch from 4807d0d to 5f750a8 Compare September 24, 2021 13:47

Labelray force-pushed the if_then_panic branch from 91d7588 to 1d8d337 Compare September 24, 2021 14:10

Add new lint if_then_panic

543b638

Labelray force-pushed the if_then_panic branch from 1d8d337 to 543b638 Compare September 24, 2021 14:14

bors merged commit fb61d04 into rust-lang:master Sep 24, 2021

camsteffen mentioned this pull request Sep 24, 2021

Manual assert_eq! (enhance if_then_panic) #7716

Open

Labelray deleted the if_then_panic branch September 25, 2021 03:08

sharnoff mentioned this pull request Sep 30, 2021

More nightly clippy fixes (2021-09-29) neondatabase/neon#691

Merged

sharnoff added a commit to neondatabase/neon that referenced this pull request Oct 1, 2021

Fix clippy errors on nightly (2021-09-29) (#691)

84f7dcd

Most of the changes are for the new if-then-panic lint added in rust-lang/rust-clippy#7669.

Comments

Conversation

Labelray commented Sep 14, 2021 • edited by giraffate Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Sep 14, 2021

Uh oh!

camsteffen commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

camsteffen commented Sep 18, 2021

Uh oh!

Labelray commented Sep 18, 2021

Uh oh!

camsteffen commented Sep 18, 2021

Uh oh!

Uh oh!

Uh oh!

giraffate left a comment

Choose a reason for hiding this comment

Uh oh!

giraffate Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

pravic Oct 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giraffate left a comment

Choose a reason for hiding this comment

Uh oh!

giraffate commented Sep 24, 2021

Uh oh!

giraffate commented Sep 24, 2021

Uh oh!

bors commented Sep 24, 2021

Uh oh!

bors commented Sep 24, 2021

Uh oh!

bors commented Sep 24, 2021

Uh oh!

Jasperav commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jasperav commented Oct 19, 2021

Uh oh!

camsteffen commented Oct 19, 2021

Uh oh!

Jasperav commented Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Labelray commented Sep 14, 2021 •

edited by giraffate

Loading

camsteffen commented Sep 14, 2021 •

edited

Loading

pravic Oct 1, 2021 •

edited

Loading

Jasperav commented Oct 19, 2021 •

edited

Loading

Jasperav commented Oct 20, 2021 •

edited

Loading