Add suspicious_operation_groupings lint #6086

Ryan1729 · 2020-09-25T20:52:54Z

This is my ( ~~currently WIP~~ ) attempt to close #6039.

changelog: Added suspicious_operation_groupings lint.

rust-highfive · 2020-09-25T20:52:57Z

(rust_highfive has picked a reviewer for you, use r? to override)

Ryan1729 · 2020-09-25T21:16:57Z

So at the moment several portions of the lint code are incomplete and only compile via the todo macro.

The main reason I'm submitting this as a draft PR intiialy is that as part of this lint I would like to provide a suggestion that swaps a dynamically discovered identifier in an expression with another identifier. Creating a suggestion by changing an identifier in an expression seems like the kind of things that might have already been needed elsewhere. But after searching for a while in the clippy codebase I wasn't able to find a previous example of that.

Before attempting to create a way to do that, I'd like to know whether I've missed someplace where identifier swapping, or something similar has already been done. Alternatively, I'd also be interested in ideas anyone has on how to implement identifier swapping, given it has not been implemented already.

My best guess at how to implement it would be to recurse down the AST and find the Span of the identifier in question, then use that to create a Span with the previous part of the surrounding expression, and another Span with the part after that, then combine that with Span for the desired new identifier to make a suggestion string. For whatever reason I find myself unsure of that approach, but if I don't hear any objections I suppose I'll try it.

Ryan1729 · 2020-09-28T22:13:31Z

After taking the time to do a more thorough search of the clippy code base, I have concluded that the most similar thing to what I need here that already exists is mirroed_exprs which, while related, is solving a different problem that doesn't really fit into what I would want here, since it expects you to know the idents beforehand.

bors · 2020-10-08T21:55:32Z

☔ The latest upstream changes (presumably #6117) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ebroto · 2020-10-11T14:36:58Z

Hey @Ryan1729, I've seen there is another WIP PR that this one depends on, and that you plan to rework that PR using a visitor. Feel free to ping me when you think this one is ready for review :)

Ryan1729 · 2020-10-20T18:40:00Z

tests/ui/suspicious_operation_groupings.stderr

+error: This sequence of operators looks suspiciously like a bug.
+  --> $DIR/suspicious_operation_groupings.rs:11:9
+   |
+LL |         self.x == other.y && self.y == other.y && self.z == other.z


This lint message is emitted twice, and my best guess as to why, is that both self.x == other.y && self.y == other.y && self.z == other.z and self.x == other.y && self.y == other.y have detectable errors, and they both get visited separately by the lint pass.

I'm unaware of a good way to prevent these double messages. Is this something that has been addressed for other lints before?

I can't remember now a similar case. I think you can leave it like this for now and once we get to review we can try to find a solution.

I think that having a double message is acceptable because the lint will likely find a lot of real bugs, so we can look into this as a follow-up PR.

With that said, I believe the correct solution would be to look at the parent Expr if any and collect all exprs that trigger the lint into one multi-suggestion.

How do we prevent the sub-expressions from triggering the lint again after the parent expression collects all the suggestions after that? As far as I can tell check_expr gets called for each expr and each expr contained inside that expr.

Ryan1729 · 2020-10-31T02:22:22Z

@ebroto As far as I understand the current CI failure is due to a change in rustc so I think this ready for review.

ebroto · 2020-10-31T08:42:51Z

r? @llogiq I'm a bit overwhelmed by PRs and quite short on time, would you mind taking over this one? Feel free to reassign it to me if you can't!

I think once this is ready we should remember to squash as there are many intermediate commits.

llogiq · 2020-10-31T09:24:28Z

Sure thing, @ebroto!

Ryan1729 · 2020-10-31T17:09:34Z

Yeah, I can tell that this PR has gotten large. That’s why I decided to postpone a feature and make issue #6275 about getting to that later.

Would it be useful to squash commits containing consecutive rewrites of the same parts of the code, now, before review?

Is there anything I can do to make reviewing this PR easier? Splitting the PR into smaller PRs maybe?

llogiq · 2020-11-01T06:48:56Z

No, it's all good, I'll just need more time. Give me about two weeks.

clippy_lints/src/utils/ast_utils.rs

tests/ui/eq_op.rs

llogiq

First round of review, no blockers so far but a few suggestions.

llogiq · 2020-11-01T09:18:27Z

tests/ui/suspicious_operation_groupings.stderr

+error: This sequence of operators looks suspiciously like a bug.
+  --> $DIR/suspicious_operation_groupings.rs:11:9
+   |
+LL |         self.x == other.y && self.y == other.y && self.z == other.z


I think that having a double message is acceptable because the lint will likely find a lot of real bugs, so we can look into this as a follow-up PR.

With that said, I believe the correct solution would be to look at the parent Expr if any and collect all exprs that trigger the lint into one multi-suggestion.

llogiq · 2020-11-01T09:26:57Z

clippy_lints/src/suspicious_operation_groupings.rs

+    /// }
+    /// ```
+    pub SUSPICIOUS_OPERATION_GROUPINGS,
+    correctness,


I feel that correctness has a higher bar than flagging suspicious operations. Correctness lints are denied, not warned, and I feel that'd be a bad idea. On the other hand, I am not sure where to place the lint. Neither complexity nor style seem an obvious solution (although I'd lean towards style if we want to make it a warn by default lint), and perf is obviously the wrong choice. That would leave pedantic or restriction, so when choosing between the two, I would choose pedantic.

suspicious_arithmetic_impl is a complexity lint, but I guess that triggers on a much smaller set of possible code. If it turned out that, in practice, the accuracy of this lint was sufficiently high, (that is, if no one ever actually needed to do the operations this lint flags,) then I would argue that correctness would still make sense. But admittedly, we don't know how accurate this lint it in practice.

The unfortunate thing is that if the lint doesn't at least warn by default, then fewer people will end up using it, and we will have less data about how often people really want to do operations that this lint flags as suspicious. I think style is also a good choice since if the lint does turn out to have false positives, then (I assume) people would be more comfortable disabling it.

So given all that I'd rather change it to style, but if there is strong opposition to that, then changing it to pedantic makes sense to me too.

Ok, then let's make it a style lint for now. We really need another category for lints like this.

clippy_lints/src/suspicious_operation_groupings.rs

bors · 2020-11-07T17:27:23Z

☔ The latest upstream changes (presumably #6134) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

llogiq · 2020-11-21T22:42:18Z

r=me with lint group changed to style.

Ryan1729 · 2020-11-22T19:41:28Z

@llogiq I've squashed some of the commits as @ebroto suggested above, and the pushed commits now include making the lint a style lint.

llogiq · 2020-11-22T22:57:37Z

I think @ebroto meant to squash all the commits. Otherwise great job! 👍

Ryan1729 · 2020-11-22T23:41:53Z

Hmm. If I look at the recent history on this repo I see series of multiple commits that seem to come from the same PR. @ebroto , can you clarify what you wanted? I was under the impression that we only wanted to squash commits containing code that was replaced later by other code.

bors · 2020-11-25T16:50:48Z

☔ The latest upstream changes (presumably #6333) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Ryan1729 · 2020-11-26T02:24:24Z

@llogiq I decided to just go ahead and squash all the commits, since it made fixing an upstream merge conflict easier.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-11-27T07:15:22Z

☔ The latest upstream changes (presumably #6389) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

run `cargo dev new_lint --category correctness --name suspicious_chained_operators --pass early` add (currently failing) tests for suspicious_chained_operators add some tests to answer a question that came up during implementation write usage code for functions we'll need to find or create Complete left-right tracking TODO get it compiling with several `todo!` invocations. refactor to a set of incomplete functions that don't expect to be able to edit a `Span` create placeholder for `suggestion_with_swapped_ident` function and correct some comments add `inside_larger_boolean_expression` test fill out `get_ident` and `suggestion_with_swapped_ident` Implementi the `IdentIter` start on implementing the `IdentIter` handle the `ExprKind::Path` case in `IdentIter` on second thought, make the iterator type dynamic so we don't need an explicit type for each one we will need handle `ExprKind::MacCall` in `IdentIter` Try handling `box x` expressions restructure `IdentIter` set `self.done` when returning `None` Handle `ExprKind::Array` reduce duplication with a macro that we expect to use several more times handle ExprKind::Call add `new_p` convenience method handle `MethodCall` handle `Tup` and `Binary` handle `Unary` simplify by not returning an additional `Expr` from the `IdentIter` add cross product test against false positives rename suspicious_chained_operators to suspicious_operation_groupings within files For the record, the exact commands run were: find . -type f -name "*.md" -exec sed -i 's/suspicious_chained_operators/suspicious_operation_groupings/g' {} + find . -type f -name "*.rs" -exec sed -i 's/suspicious_chained_operators/suspicious_operation_groupings/g' {} + find . -type f -name "*.rs" -exec sed -i 's/SUSPICIOUS_CHAINED_OPERATORS/SUSPICIOUS_OPERATION_GROUPINGS/g' {} + find . -type f -name "*.rs" -exec sed -i 's/SuspiciousChainedOperators/SuspiciousOperationGroupings/g' {} + Also: rename file to match module name rename test file to match lint name start implementing `IdentDifference` creation add `IdentIter` utility use `ident_iter::IdentIter` fix bug in `suggestion_with_swapped_ident` add `inside_if_statements` test implement `Add` `todo`s register `SuspiciousOperationGroupings` lint pass fill in `chained_binops`, and fill in a stopgap version of `ident_difference_expr`, but then notice that the lint does not seem to ever be run in the tests run `cargo dev update_lints` and not that the `suspicious_operation_groupings` lint still does not seem to be run fix base index incrementing bug fix paired_identifiers bug, and remove ident from `Single` change help prefix and note our first successful lint messages! add odd_number_of_pairs test get the `non_boolean_operators` test passing, with two copies of the error message extract `is_useless_with_eq_exprs` so we can know when `eq_op` will already handle something add `not_caught_by_eq_op` tests since `s1.b * s1.b` was (reasonably) not caught by `eq_op` cover the case where the change should be made on either side of the expression with `not_caught_by_eq_op` tests produce the expected suggestion on the `not_caught_by_eq_op_middle_change_left` test confirm that the previous tests still pass and update references fix early continue bug and get `not_caught_by_eq_op_middle_change_right` passing note that `not_caught_by_eq_op_start` already passes fix bugs based on misunderstanding of what `Iterator::skip` does, and note that `not_caught_by_eq_op_end` now passes add several parens tests and make some of them pass handle parens inside `chained_binops_helper` and note that this makes several tests pass get `inside_larger_boolean_expression_with_unsorted_ops` test passing by extracting out `check_same_op_binops` function also run `cargo dev fmt` note that `inside_function_call` already passes add another `if_statement` test remove the matching op requirement, making `inside_larger_boolean_expression_with_unsorted_ops` pass prevent non-change suggestions from being emitted get the `Nested` tests passing, and remove apparently false note about eq_op add a test to justify comment in `ident_difference_expr_with_base_location` but find that the failure mode seems different than expected complete `todo` making `do_not_give_bad_suggestions_for_this_unusual_expr` pass and add some more tests that already pass add test to `eq_op` note that `inside_fn_with_similar_expression` already passes fix `inside_an_if_statement` and note that it already passes attempt to implement if statement extraction and notice that we don't seem to handle unary ops correctly add `maximum_unary_minus_right_tree` test and make it pass add two tests and note one of them passes filter out unary operations in several places, and find that the issue seems to be that we don't currently recognize the error in `multiple_comparison_types_and_unary_minus` even so. remove filtering that was causing bad suggestions remove tests that were deemed too much for now run `cargo dev fmt` correct eq_op post-merge fill out the description and delete debugging code run `cargo dev update_lints` update eq_op references add parens to work around rustfmt issue rust-lang#3666 and run rustfmt rust-lang/rustfmt#3666 (comment) update references after formatting fix dogfood issues fix multi-cursor edit fix missed dogfood error fix more dogfood pedantic issues, including function length even more nesting insert hidden definition of Vec3 so docs compile add spaces to second struct def reword test description comment Co-authored-by: llogiq <bogusandre@gmail.com> add local `use BinOpKind::*;` Apply suggestions from code review Co-authored-by: llogiq <bogusandre@gmail.com> switch `SUSPICIOUS_OPERATION_GROUPINGS` to a style lint run `cargo dev update_lints` put both usages of `op_types` in the same closure to satisfy `borrowck` fix compile error

Ryan1729 · 2020-11-28T00:51:21Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

llogiq · 2020-11-28T08:17:20Z

Sorry for the delays. Thank you for seeing this through with us. @bors r+

bors · 2020-11-28T08:17:21Z

📌 Commit af1cc5c has been approved by llogiq

bors · 2020-11-28T08:17:29Z

⌛ Testing commit af1cc5c with merge 7a73a25...

bors · 2020-11-28T08:38:06Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 7a73a25 to master...

rust-highfive assigned matthiaskrgr Sep 25, 2020

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

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch from 9a1e611 to b85ac8d Compare September 30, 2020 22:32

Ryan1729 mentioned this pull request Oct 1, 2020

Add ast ident iterator #6100

Closed

ebroto assigned ebroto and unassigned matthiaskrgr Oct 8, 2020

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch from 442bb91 to 8658110 Compare October 10, 2020 00:34

Ryan1729 changed the title ~~Add suspicious chained operators lint~~ Add suspicious_operation_groupings lint Oct 10, 2020

ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 11, 2020

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch 3 times, most recently from bfcae7b to 09eacac Compare October 17, 2020 23:48

Ryan1729 commented Oct 20, 2020

View reviewed changes

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch 3 times, most recently from 4bcb20b to 91f4063 Compare October 31, 2020 02:09

Ryan1729 marked this pull request as ready for review October 31, 2020 02:20

Ryan1729 requested a review from ebroto October 31, 2020 02:31

Ryan1729 mentioned this pull request Oct 31, 2020

Add support for if statements to suspicious_operation_groupings lint #6275

Open

rust-highfive assigned llogiq and unassigned ebroto Oct 31, 2020

llogiq reviewed Nov 1, 2020

View reviewed changes

clippy_lints/src/utils/ast_utils.rs Show resolved Hide resolved

llogiq reviewed Nov 1, 2020

View reviewed changes

tests/ui/eq_op.rs Outdated Show resolved Hide resolved

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch from 2d138fb to 2e05439 Compare November 1, 2020 20:16

llogiq reviewed Nov 7, 2020

View reviewed changes

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch from e6342d0 to 6d444a2 Compare November 7, 2020 23:02

llogiq mentioned this pull request Nov 22, 2020

Missing lint categories? #6366

Closed

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch from ee6e149 to 89a9c27 Compare November 22, 2020 19:37

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch 2 times, most recently from abae85d to 219a71f Compare November 26, 2020 02:22

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Nov 26, 2020

Ryan1729 force-pushed the add-suspicious_chained_operators-lint branch from 219a71f to af1cc5c Compare November 28, 2020 00:50

bors merged commit 7a73a25 into rust-lang:master Nov 28, 2020

MarijnS95 mentioned this pull request Dec 19, 2020

Add support for binary function calls in suspicious_operation_groupings lint #6474

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add suspicious_operation_groupings lint #6086

Add suspicious_operation_groupings lint #6086

Ryan1729 commented Sep 25, 2020 •

edited

Loading

rust-highfive commented Sep 25, 2020

Ryan1729 commented Sep 25, 2020

Ryan1729 commented Sep 28, 2020 •

edited

Loading

bors commented Oct 8, 2020

ebroto commented Oct 11, 2020

Ryan1729 Oct 20, 2020

ebroto Oct 22, 2020

llogiq Nov 1, 2020

Ryan1729 Nov 7, 2020

Ryan1729 commented Oct 31, 2020

ebroto commented Oct 31, 2020

llogiq commented Oct 31, 2020

Ryan1729 commented Oct 31, 2020

llogiq commented Nov 1, 2020

llogiq left a comment

llogiq Nov 1, 2020

llogiq Nov 1, 2020

Ryan1729 Nov 7, 2020 •

edited

Loading

llogiq Nov 21, 2020

bors commented Nov 7, 2020

llogiq commented Nov 21, 2020

Ryan1729 commented Nov 22, 2020

llogiq commented Nov 22, 2020

Ryan1729 commented Nov 22, 2020

bors commented Nov 25, 2020

Ryan1729 commented Nov 26, 2020

bors commented Nov 27, 2020

Ryan1729 commented Nov 28, 2020

llogiq commented Nov 28, 2020

bors commented Nov 28, 2020

bors commented Nov 28, 2020

bors commented Nov 28, 2020

Add suspicious_operation_groupings lint #6086

Add suspicious_operation_groupings lint #6086

Conversation

Ryan1729 commented Sep 25, 2020 • edited Loading

rust-highfive commented Sep 25, 2020

Ryan1729 commented Sep 25, 2020

Ryan1729 commented Sep 28, 2020 • edited Loading

bors commented Oct 8, 2020

ebroto commented Oct 11, 2020

Ryan1729 Oct 20, 2020

Choose a reason for hiding this comment

ebroto Oct 22, 2020

Choose a reason for hiding this comment

llogiq Nov 1, 2020

Choose a reason for hiding this comment

Ryan1729 Nov 7, 2020

Choose a reason for hiding this comment

Ryan1729 commented Oct 31, 2020

ebroto commented Oct 31, 2020

llogiq commented Oct 31, 2020

Ryan1729 commented Oct 31, 2020

llogiq commented Nov 1, 2020

llogiq left a comment

Choose a reason for hiding this comment

llogiq Nov 1, 2020

Choose a reason for hiding this comment

llogiq Nov 1, 2020

Choose a reason for hiding this comment

Ryan1729 Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

llogiq Nov 21, 2020

Choose a reason for hiding this comment

bors commented Nov 7, 2020

llogiq commented Nov 21, 2020

Ryan1729 commented Nov 22, 2020

llogiq commented Nov 22, 2020

Ryan1729 commented Nov 22, 2020

bors commented Nov 25, 2020

Ryan1729 commented Nov 26, 2020

bors commented Nov 27, 2020

Ryan1729 commented Nov 28, 2020

llogiq commented Nov 28, 2020

bors commented Nov 28, 2020

bors commented Nov 28, 2020

bors commented Nov 28, 2020

Ryan1729 commented Sep 25, 2020 •

edited

Loading

Ryan1729 commented Sep 28, 2020 •

edited

Loading

Ryan1729 Nov 7, 2020 •

edited

Loading