needless_bool loses comments when invoked with --fix

[yuzisee]

Summary

warning: this if-then-else expression returns a bool literal
    --> src/model_view_impl/project_main/mvc_models.rs:1580:11
     |
1580 | /           if acp_spec.cancellation_required() == false {
1581 | |             // ... This is an important comment with a detailed discusion justifying the business logic / business decisions behind this specific behavior.
1582 | |             true
1583 | |           } else {
1584 | |             false
1585 | |           }
     | |___________^ help: you can reduce it to: `acp_spec.cancellation_required() == false`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_bool
     = note: `#[warn(clippy::needless_bool)]` on by default

With the lint above, when used with --fix causes the following code changes:

         (false, _) => true,
         // Cancellation info not available...
         (true, Some(acp_spec)) => {
-          if acp_spec.cancellation_required() == false {
-            // ... This is an important comment with a detailed discusion justifying the business logic / business decisions behind this specific behavior.
-            true
-          } else {
-            false
-          }
+          !acp_spec.cancellation_required()
         }
         _ => false,
       }

but the comment line is lost.

While I'm not sure where the comments should be retained it seems like a bad idea to remove them, especially if they are describing some important business logic that justifies the behavior.

Reproducer

I tried this code: cargo clippy --fix

if acp_spec.cancellation_required() == false {
  // ... This is an important comment with a detailed discusion justifying the business logic / business decisions behind this specific behavior.
  true
} else {
  false
}

I expected to see this happen:

The comment is somehow preserved, and at the very least, not completely deleted.

Instead, this happened:

The comment was removed, and only !acp_spec.cancellation_required() remained.

Version

• cargo --version cargo 1.69.0 (6e9a83356 2023-04-12)
• rustc --version rustc 1.69.0 (84c898d65 2023-04-16)
• cargo clippy --version clippy 0.1.69 (84c898d6 2023-04-16)

Additional Labels

No response

[blyxyas]

I'm sorry but this probably won't ever get fixed. needless_bool is a "late" lint, so it works on the HIR level. This means that we cannot detect really comments without doing tremendous amounts of work. Maybe displaying a warning about this behaviour would "solve" the problem?

[yuzisee]

Is there a way to mark lints as "unsafe for --fix"? Mostly I'm trying to figure out which lints are safe to use with --fix since ideally we can run this from CI (or even a pre-commit hook)

The documentation has a "Known Problems" section,

but reading every single Known Problems entry inside every single lint, so that we can go and build a custom list of what is considered "safe to use with --fix" seems like not quite the right developer workflow (maybe I'm missing something); or perhaps are there eventual plans for --fix-safe or --fix-lossless option instead?

[blyxyas]

AFAIK there aren't any plans to implement a safe fix. But I think that there should an option like that.

I'll comment it on the Zulip chat and open a PR in a few hours if everything goes smoothly.

[samueltardieu]

I'm sorry but this probably won't ever get fixed. needless_bool is a "late" lint, so it works on the HIR level. This means that we cannot detect really comments without doing tremendous amounts of work. Maybe displaying a warning about this behaviour would "solve" the problem?

I'm not sure I follow this reasoning: NEEDLESS_BOOL_ASSIGN, a late lint in the same file as NEEDLESS_BOOL, does this check and does not rewrite the if expression if there is any comment inside a branch (and there are tests for this). I used span_extract_comment() on the whole if expression to detect the presence of a comment.

I'll have a look at this issue.

@rustbot claim