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 bool_comparison lint usage with macros #3983

Closed
wants to merge 1 commit into from

Conversation

nbaksalyar
Copy link

Fixes #3973.

Clippy gives wrong suggestions on bool comparisons with results of macros, such as e.g. bool == cfg!(feature = abc). This PR fixes the problem by checking if any side of a binary expresion is expanded from a macro.

There's a similar problem present with rustfix, so the bool_comparison.fixed file has been changed manually for now.

Clippy gives wrong suggestions on bool comparisons with results of
macros, such as e.g. bool == cfg!(feature = abc). This commit fixes
the problem by checking if any side of a binary expresion is expanded
from a macro.
@oli-obk
Copy link
Contributor

oli-obk commented Apr 17, 2019

There's a similar problem present with rustfix, so the bool_comparison.fixed file has been changed manually for now.

rustfix should only apply clippy's suggestions, if there's a problem I presume it's due to clippy. What was the issue you encountered?

@nbaksalyar
Copy link
Author

What was the issue you encountered?

The same one: cfg!(feature = "debugging") got replaced with just false and the expression transformed to if !abc {}.

@@ -226,6 +226,11 @@ fn check_comparison<'a, 'tcx>(

if let ExprKind::Binary(_, ref left_side, ref right_side) = e.node {
let (l_ty, r_ty) = (cx.tables.expr_ty(left_side), cx.tables.expr_ty(right_side));

if in_macro(left_side.span) || in_macro(right_side.span) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a little bit to restrictive: Playground

This won't suggest anything for macro! == true. But this could be simplified to macro!.


While testing this, I found another "bug". Clippy suggests for this:

warning: equality checks against true are unnecessary
 --> src/main.rs:8:8
  |
8 |     if m!(func) == true {
  |        ^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `$func()`
  |
  = note: #[warn(clippy::bool_comparison)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison

This suggestion is not applicable, obviously. This can be fixed by changing this line

let hint = Sugg::hir_with_applicability(cx, expr, "..", &mut applicability);

with
pub fn hir_with_macro_callsite(cx: &LateContext<'_, '_>, expr: &hir::Expr, default: &'a str) -> Self {

But that won't be that easy, since we need a function, which adapts the applicability. So either

  1. you can try to fix this in this PR
  2. you can open an issue and leave this for later

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review!

I'd love to fix this in this PR. :)

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Apr 18, 2019
@flip1995
Copy link
Member

Ping from triage @nbaksalyar. Any updates on this? The bottom part of #3983 (comment) is just additional work, which you can leave out in this PR and we can open an issue for this.

@flip1995
Copy link
Member

flip1995 commented Jul 3, 2019

Ping from triage @nbaksalyar. This PR didn't get an update for quite some time now, therefore I'll close this to keep the PR queue clean. Thanks for your contribution and feel free to reopen it whenever you got time to work on this!

@flip1995 flip1995 closed this Jul 3, 2019
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 3, 2019
@nbaksalyar
Copy link
Author

Hi @flip1995, sorry for lack of progress on this one.
I'm still interested to complete this, so I'll address your comments and reopen it soon. Thank you!

@flip1995
Copy link
Member

flip1995 commented Jul 3, 2019

Thanks! If you have any questions just ask here.

@HKalbasi
Copy link
Member

finished in #6211
@rustbot label -S-inactive-closed

@rustbot rustbot removed the S-inactive-closed Status: Closed due to inactivity label Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

needless_bool triggers on cfg! comparisons
5 participants