Make if_then_panic handle situation of BinOpKind::And || BinOpKind::Or

[surechen]

fixes #7731

Make if_then_panic handle situation of cond.kind = ExprKind::DropTemps(ExprKind::Binary(BinOpKind::And || BinOpKind::Or, left, right), ..) =

changelog: [if_then_panic] Fix suggestion for more complex conditions

[surechen]

fixes #7731

Make if_then_panic handle situation of cond.kind = ExprKind::DropTemps(ExprKind::Binary(BinOpKind::And || BinOpKind::Or, left, right), ..) =

Hello, @flip1995. There is no automatic allocation of reviewer, I do not know how to deal with it, can you help me? Thank you

[flip1995]

There is no automatic allocation of reviewer

That's weird. I just self-assign it to me :)

[flip1995]

r? @flip1995

(FYI @surechen this is how you can assign/request a specific reviewer)

[flip1995]

I think your solution is more difficult than it has to be. The easiest fix for this would be to put the expr in if <expr> into parentheses and be done with it. You can do that more easily by just using clippy_utils::Sugg. There should be a maybe_paren function somewhere in there that will at parenthesis to the suggestion, if necessary.

[teor2345]

I think your solution is more difficult than it has to be. The easiest fix for this would be to put the expr in if <expr> into parentheses and be done with it. You can do that more easily by just using clippy_utils::Sugg. There should be a maybe_paren function somewhere in there that will at parenthesis to the suggestion, if necessary.

This would also handle operators like == and !=, and other complex expressions.

Edit: there doesn't seem to be a simplification for !(a != b) -> a == b, but that's a totally separate issue.

[surechen]

I think your solution is more difficult than it has to be. The easiest fix for this would be to put the expr in if <expr> into parentheses and be done with it. You can do that more easily by just using clippy_utils::Sugg. There should be a maybe_paren function somewhere in there that will at parenthesis to the suggestion, if necessary.

Hi, Done. clippy_utils::Sugg is very cool. Thank you very much.

[surechen]

I think your solution is more difficult than it has to be. The easiest fix for this would be to put the expr in if <expr> into parentheses and be done with it. You can do that more easily by just using clippy_utils::Sugg. There should be a maybe_paren function somewhere in there that will at parenthesis to the suggestion, if necessary.

This would also handle operators like == and !=, and other complex expressions.

Edit: there doesn't seem to be a simplification for !(a != b) -> a == b, but that's a totally separate issue.

Agree, the various possibilities here are very complicated. Using sugg is better than trying to cover various possible situations in this lint.

[flip1995]

cond and e should have the same Span , so the two format! calls are pretty much equivalent. I think you only have to change the else case and not the ExprKind::Unary case.

[surechen]

cond and e should have the same Span , so the two format! calls are pretty much equivalent. I think you only have to change the else case and not the ExprKind::Unary case.

Hi, I thought the same at the beginning, but if I only modify the else block:

let cond_sugg = if let ExprKind::DropTemps(Expr{kind: ExprKind::Unary(UnOp::Not, not_expr), ..}, ..) = cond.kind {
     sugg::Sugg::hir_with_applicability(cx, not_expr, "..", &mut applicability).maybe_par().to_string()
} else {
     format!("!{}", sugg::Sugg::hir_with_applicability(cx, cond, "..", &mut applicability).maybe_par().to_string())
};

result will be like this：

error: only a `panic!` in `if`-then statement
  --> $DIR/if_then_panic.rs:45:5
   |
LL | /     if a.is_empty() && !b.is_empty() {
LL | |         panic!("panic3");
LL | |     }
   | |_____^ help: try: `assert!(!a.is_empty() && !b.is_empty(), "panic3");`

So I think "a.is_empty() && !b.is_empty()" is ExprKind::DropTemps{Expr{kind: ExprKind::Binary(BinOpKind::And, left, right), ..}} here.

[flip1995]

Hm, that's weird. I also just tested it locally. In that case, let's merge it as it is.

[flip1995]

Edit: there doesn't seem to be a simplification for !(a != b) -> a == b, but that's a totally separate issue.

@teor2345 There is the nonminimal_bool lint that should take care of this. But this lint is a bit restricted, since simplifying logical expressions is a NP-hard problem (or checking if two expressions are equivalent or something like that...).

[flip1995]

@bors r+

Thanks!

[bors]

📌 Commit 41e2c68 has been approved by flip1995

[bors]

⌛ Testing commit 41e2c68 with merge b48ba90...

[bors]

💔 Test failed - checks-action_test

[flip1995]

@bors retry (added changelog)

[bors]

⌛ Testing commit 41e2c68 with merge fe999e8...

[bors]

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