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

Issue4983 bool equal not bool #5144

Closed
wants to merge 351 commits into from
Closed

Issue4983 bool equal not bool #5144

wants to merge 351 commits into from

Conversation

mgr-inz-rafal
Copy link
Contributor

@mgr-inz-rafal mgr-inz-rafal commented Feb 6, 2020

This is WIP-PR.

Fixes #4983.

if let BinOpKind::Eq = op.node;
if one_side_is_unary_not(&left_side, &right_side);
then {
span_lint_and_help(cx, BOOL_COMPARISON, e.span, "Here comes", "the suggestion");
Copy link
Member

Choose a reason for hiding this comment

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

When building a suggestion, you have to understand how spans work. A lint with a suggestion has 2 spans: 1 span (L) for the lint message and 1 span (S) for the suggestion. When running cargo fix on a suggestion like this, the span (S) is completely removed and replaced with the code inside the sugg parameter of the span_lint_and_sugg function. The output with differing spans may look like this:

error: Lint message
  --> $DIR/bool_comparison.rs:119:8
   |
LL |     if a == !b {};
   |        ^^^^^^^
   |
   = help: replace it with:
   |
LL |     if a != b {}
   |        ^^^^^^

As you may have noticed, the span_lint_and_sugg function only has one span argument. This is because most of the time, the span of te lint message and of the suggestion are the same. This is also the case here. The output of this case is most of the time more compact and looks nicer:

error: Here comes
  --> $DIR/bool_comparison.rs:119:8
   |
LL |     if a == !b {};
   |        ^^^^^^^ help: replace it with: `a != b`

The span of the thing you want to replace is e.span. To build the suggestion, you can use utils::snippet_with_applicability. Let's name some things:

a == !b
^^^^^^^ e
^       left_side
     ^^ right_side
      ^ ?

So you have to get the span of ?. This is the span of the unop expression in the is_unary_not function above. Building the complete suggestion, you have to do:

format!(
    "{} != {}",
    snippet_with_applicability(cx, left_side.span, &mut applicability),
    snippet_with_applicability(cx, ?.span, &mut applicability),
)

This won't be enough though, since you have to also build the suggestion for the symmetric case. But this is homework for you. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the thorough explanation. I need some time to figure things out :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,
At last, I have some spare time to work on the issue and... mess-up the PR in the process ;-)

Anyway, I think I got it and the message now looks like the one below. I will clean-up the code and submit a new PR for review soon:

 error: This comparison might be written more concisely
   --> $DIR/bool_comparison.rs:119:8
    |
 LL |     if a == !b {};
    |        ^^^^^^^ help: try simplifying it as shown: `a != b`
 
 error: This comparison might be written more concisely
   --> $DIR/bool_comparison.rs:120:8
    |
 LL |     if !a == b {};
    |        ^^^^^^^ help: try simplifying it as shown: `a != b`

@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 Feb 7, 2020
flip1995 and others added 28 commits February 14, 2020 14:37
Let update_lints also generate the internal lints

r? @phansch

changelog: none
Typo in literal_representation.rs

Octal numbers can't have 8 in them ;)

changelog: none
These regions will all be `ReErased` soon.
Avoid using regions from `TypeckTables`

These regions will all be `ReErased` soon. (rust-lang/rust#69189)

changelog: none
Don't lint `single_component_path_imports` in macros

Fixes #5154

changelog: Fix false positive in `single_component_path_imports`
Reduce `pulldown-cmark` size

Should reduce `pulldown-cmark` size.
ref. https://github.com/raphlinus/pulldown-cmark#build-options

changelog: none
…al, r=flip1995

Lint lossy whole number float literals

changelog: Extend `excessive_precision` to lint lossy whole number float literals

Fixes #5160
Rename `FunctionRetTy` to `FnRetTy`

Rustup to rust-lang/rust#69179

changelog: none
Expand `missing_errors_doc` to also work on async functions

This adds the `missing_errors_doc` lint to async functions.

changelog: Make [`missing_errors_doc`] lint also trigger on `async` functions
flip1995 and others added 28 commits March 18, 2020 14:55
new_lint.rs: encourage authors to write more detailed code samples in lint descriptions (linted as well as fixed code)

changelog: none
clean up a few lint docs

changelog: none
integer_arithmetic: detect integer arithmetic on references.

changelog: integer_arithmetic fix false negatives with references on integers

Fixes #5328
Update changelog to 1.43.0 beta

In the beta changelog update, I accidentally used the commit of the 1.43.0 beta, instead of the 1.42.0 beta. I fixed this in this PR.

[Rendered](https://github.com/flip1995/rust-clippy/blob/changelog/CHANGELOG.md)

r? @Manishearth

changelog: none
Changes the span on which the lint is reported to point to only the
function return type instead of the entire function body.
Fixes #5284
rustup rust-lang/rust#69189

rustups rust-lang/rust#69189 which is part of rust-lang/rust#70085
(at least I think this is the only pr that changes clippy test stdout)

changelog: none
Rollup of 4 pull requests

Successful merges:

 - #5326 (rustup rust-lang/rust#69838)
 - #5333 (rustup rust-lang/rust#69189)
 - #5336 (rustup rust-lang/rust#69920)
 - #5341 (Rustup to rust-lang/rust#66131)

Failed merges:

r? @ghost

changelog: none
Improvement: Don't show function body in needless_lifetimes

Changes the span on which the lint is reported to point to only the
function return type instead of the entire function body.
Fixes #5284

changelog: none
Fix documentation generation for configurable lints

In #5135, the configuration macro changed, but the documentation generation script wasn't updated. This PR catches up on this.

[Preview](https://flip1995.github.io/rust-clippy/master/index.html)

r? @Manishearth

changelog: Document configuration options of lints again.
Fix single binding closure

Fix the `match_single_binding` lint when triggered inside a closure.

Fixes: #5347

changelog: Improve suggestion for [`match_single_binding`]
…fal/rust-clippy into issue4983_bool_equal_not_bool
@mgr-inz-rafal
Copy link
Contributor Author

Will start from scratch :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New lint idea: suggest ^ when comparing bools