Expands `manual_memcpy` to lint ones with loop counters #5727

rail-rain · 2020-06-18T01:27:10Z

This PR expands manual_memcpy to lint ones with loop counters as described in #1670 (comment)

Although the current code is working, I have a couple of questions and concerns.

Firstly, I manually implemented Clone for Sugg because AssocOp lacks Clone. As AssocOp only holds an enum, which is Copy, as a value, it seems AssocOp can be Clone; but, I was not sure where to ask it. Should I make a PR to rustc? The PR was made.

Secondly, manual copying with loop counters are likely to trigger needless_range_loop and explicit_counter_loop along with manual_memcpy; in fact, I explicitly allowed them in the tests. Is there any way to disable these two lints when a code triggers manual_memcpy?

And, another thing I'd like to note is that Sugg adds unnecessary parentheses when expressions with parentheses passed to its hir function, as seen here:

error: it looks like you're manually copying between slices
  --> $DIR/manual_memcpy.rs:145:14
   |
LL |     for i in 3..(3 + src.len()) {
   |              ^^^^^^^^^^^^^^^^^^ help: try replacing the loop by: `dst[3..((3 + src.len()))].clone_from_slice(&src[..((3 + src.len()) - 3)])

However, using the hir function is needed to prevent the suggestion causing errors when users use bitwise operations; and also this have already existed, for example: verbose_bit_mask. Thus, I think this is fine.

changelog: Expands manual_memcpy to lint ones with loop counters

rust-highfive · 2020-06-18T01:27:13Z

r? @flip1995

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

flip1995

Quick early review (didn't read the PR description, only skimmed the code). I will do a in-depth review once the rustup is through and the below mentioned rustc PR is merged.

clippy_lints/src/utils/sugg.rs

flip1995 · 2020-06-22T19:04:20Z

cc rust-lang/rust#73629

bors · 2020-06-23T00:55:32Z

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

clippy_lints/src/utils/sugg.rs

…ishearth Make AssocOp Copy Found that this enum is not `Copy` while reviewing this Clippy PR: rust-lang/rust-clippy#5727 (comment) There shouldn't be a reason why this should not be `Copy`.

flip1995 · 2020-06-28T12:10:54Z

@rail-rain AssocOp now implements Copy. Regarding the CI failures: You can wait for #5751 to get merged and then rebase-squash this PR on top of the master branch.

rail-rain · 2020-06-28T22:25:43Z

Thank you for the information. I've subscribed #5751, so you don't have to ping me again when it gets merged. I will reflect the change in AccocOp after it because it's seemingly impossible to run the test with rust-lang/rust#73629 now for me.

bors · 2020-06-30T14:45:33Z

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

rail-rain · 2020-07-01T02:22:31Z

@flip1995 I've run git rebase master and force-pushed it (although I was not sure if this is what you mean by saying 'rebase-squash'; should I squash my commits?). Also I've removed the manual Clone implementation on Sugg in favour of deriving. I think now it's finally ready for a review.

flip1995 · 2020-07-03T10:12:55Z

should I squash my commits?

Yes, I think that would be reasonable in that case, especially, since there are multiple fmt commits and small refactor commits. (Good for initial implementation+review, but doesn't add value to the history of the master branch).

bors · 2020-07-03T15:13:10Z

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

rail-rain · 2020-07-04T03:22:09Z

Ah, I see. That perfectly make sense. However, I'm afraid squashing could really affects reviews (e.g. the fmt commits changed the level of indentation and hide what actually changed); has the review already done, or are you fine with reviewing one commit? And also, should I squash the changes I made after submitting the PR? It would break the references to the code in the past reviews (although I'm not sure if there would be anyone who would want to see these changes).

flip1995 · 2020-07-13T14:30:15Z

Yeah, let's squash right before merging, this is a rather complex PR. I try to leave an in depth review in the coming days (maybe even today).

flip1995 · 2020-07-26T16:29:08Z

I still didn't get to review this in-depth, and I'm sorry! I try to get to this soon.

rail-rain · 2020-07-28T01:02:43Z

That's all right. Take your time. Actually, I don't feel so energetic recently and even haven't been working on other issues.

rail-rain · 2020-08-16T03:30:30Z

Sorry, I wasn't aware of the number of the changes made in rustc and Clippy. This rebase might confuse you.

flip1995 · 2020-08-16T15:58:13Z

No problem, thanks for low key reminding me, that I still have to review this 😄

bors · 2020-09-24T14:05:37Z

☔ The latest upstream changes (presumably #6076) 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

…isitor

…ap`)

flip1995

Sorry for taking so long to review this. Expect multiple review cycles for this lint.

Secondly, manual copying with loop counters are likely to trigger needless_range_loop and explicit_counter_loop along with manual_memcpy; in fact, I explicitly allowed them in the tests. Is there any way to disable these two lints when a code triggers manual_memcpy?

Generally triggering two lints with conflicting suggestions at the same time is bad. Normally we can handle this, if they are in the same lint pass with something like this:

if !lint_a_linted {
    lint_b()
}

And then only run the lint B, if lint A didn't trigger.

And, another thing I'd like to note is that Sugg adds unnecessary parentheses when expressions with parentheses passed to its hir function, as seen here:

Don't worry about this for now. This is a easy fix in a second pass, because the unnecessary_parens lint will trigger, which is also auto-applicable.

The suggestion you currently generate only replaces the expression, that creates the range in the loop. You want to replace the whole loop. That means you need to use the span for the whole loop in the suggestion. You can keep the span you currently using for the lint message though (or make it so the span for the lint message starts with for).

tests/ui/manual_memcpy.stderr

clippy_lints/src/loops.rs

rail-rain · 2020-09-27T04:03:55Z

Hello,
Thanks a lot for reviwing!

I've addressed snippet_opt and conflicting lints.

Regarding the span for the suggestion, I've replaced the span with the whole loop; but I'm not sure if I've got what you meant by saying "You can keep the span you currently using". Can I use different spans for tools (namely rustfix) and messages? Having said that, I think the suggestion is fine as it is because other lints like while_let_loop uses this. (By the way, this weirdness have already existed for a while... and explicit_counter_loop is also suffered from this. The original was good, so I suspect rustup caused it at some point).

~~Oh, I see the test is falling because it's too long. It seems I need either to split up the test or to shorten the suggestion.~~ I've split the test for now; I might revert it if the suggestion became much shorter later.

PS: I've also implemented From<Sugg> for MinifyingSugg and removed other methods to construct MinifyingSugg.

flip1995 · 2020-10-01T12:19:08Z

Changes LGTM, I will do a complete review again right after writing this comment. I just want to write down my thoughts before forgetting them.

Can I use different spans for tools (namely rustfix) and messages?

Yes, in Clippy with span_lint_and_then(LINT, span1, ..., |diag| { diag.span_suggestion(span2, sugg) }). In the |diag| closure you can then use the full potential of the DiagnosticBuilder.

But seeing the suggestion as it is now, I don't think this is necessary and the suggestion is fine now.

I've split the test for now; I might revert it if the suggestion became much shorter later.

Splitting the tests definitely make sense here. I would keep it that way.

flip1995

Round 2/3?: Mostly documentation and some style things I noticed.

clippy_lints/src/loops.rs

flip1995 · 2020-10-01T13:03:07Z

clippy_lints/src/loops.rs

+    let print_limit =
+        |end: &Expr<'_>, end_str: &str, base: &Expr<'_>, sugg: MinifyingSugg<'static>| -> MinifyingSugg<'static> {


Is there a reason, that this is a closure and not a function? Scanning over the code I couldn't find any upvars, so it should be possible to make it a function for consistency.

print_limit captures cx and reduces the number of its argument. I actually prefer to make it a function; I kept it just because it was, and there's a couple of examples of this practice in Clippy.

Ah ok. I'm good with both, so you decide if you want to turn it into a function 👍

It turned out print_limit also captures limits, and print_offset_and_limit captures so many; and then I changed my mind. I'd like to keep these closures. (I even removed the explicit return type from print_limit)

...and also swap their position

* `dst.len()` as the end of the range with loop counters * the increment of the loop counter at the top of the loop

flip1995 · 2020-10-02T21:47:27Z

tests/ui/manual_memcpy/with_loop_counters.rs

+    for i in 3..src.len() {
+        dst[i] = src[count];
+        count += 1
+    }


This should be the last review round:

I would like two more tests (correct me if they won't make sense):

Use dst.len() instead of src.len()

put count += 1; in front of dst[i] = src[count];

I probably thought it was fine because the dst.len() case is tested in without_loop_counters.rs, and the 'loop counters at the top of the loop' (am I understand what you said correctly?) case is the almost same as #6076, a FP in expilcit_counter_loop which shares the same visitor to check this case. Having said that, I cannot say these cases don't make sense. ~~So I will add these.~~ Done.

flip1995 · 2020-10-10T06:32:11Z

Thanks for all the great work! And for addressing my reviews so quickly, even though I was really slow myself.

@bors r+ Let's finally merge this 🚀

bors · 2020-10-10T06:32:12Z

📌 Commit b541884 has been approved by flip1995

bors · 2020-10-10T06:32:37Z

⌛ Testing commit b541884 with merge dbc0285...

bors · 2020-10-10T06:53:09Z

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

rust-highfive assigned flip1995 Jun 18, 2020

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

flip1995 reviewed Jun 22, 2020

View reviewed changes

clippy_lints/src/utils/sugg.rs Outdated Show resolved Hide resolved

clippy_lints/src/utils/sugg.rs Outdated Show resolved Hide resolved

flip1995 mentioned this pull request Jun 22, 2020

Make AssocOp Copy rust-lang/rust#73629

Merged

flip1995 reviewed Jun 23, 2020

View reviewed changes

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

rail-rain force-pushed the manual_memcpy_with_counter branch from 20a4453 to 4ae85e0 Compare July 1, 2020 00:29

rail-rain force-pushed the manual_memcpy_with_counter branch from 4ae85e0 to 2579e43 Compare August 16, 2020 02:04

rail-rain added 6 commits September 25, 2020 08:59

Use if_chain in Increment/InitializeVisitor

dc89bb1

Use else blocks instead of return statements in Increment/InitializeV…

116f30d

…isitor

Check if it's after the loop earlier

b2d5b89

add concinient methods to Increment/InitializeVisitor

31cb110

Split VarState

c599e2f

Generalise InitializeVisitor

13c207d

rail-rain added 3 commits September 25, 2020 09:02

make clippy happy (needless_pass_by_value, filter_map and `find_m…

f410df3

…ap`)

use #[derive] instead of the manual implementation

ce653d6

Reflect the changes that has been made and fmt

e855fe3

rail-rain force-pushed the manual_memcpy_with_counter branch from 2579e43 to e855fe3 Compare September 24, 2020 21:06

flip1995 requested changes Sep 25, 2020

View reviewed changes

flip1995 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 Sep 25, 2020

rail-rain added 3 commits September 27, 2020 14:19

Replace snippet_opt + unwrap_or_else with snippet

4918e7a

Prevent unnecessary lints from triggering

5c71352

Use the spans of the entire for loops for suggestions

99aceeb

rail-rain added 2 commits September 28, 2020 02:27

Use the From trait to make MinifyingSugg

9725f00

split up the manual_memcpy test

ec94bd6

flip1995 reviewed Oct 1, 2020

View reviewed changes

rail-rain added 7 commits October 2, 2020 13:30

document MinifyingSugg and Offset

3883841

...and also swap their position

simplify the code

94d7b82

fix a FN where incr exprs with no semicolon at ends

1402d8a

add comments around loop_counters

41a0ccb

supress clippy::filter_map

2a0e45b

Add tests for

7820cb1

* `dst.len()` as the end of the range with loop counters * the increment of the loop counter at the top of the loop

remove the explicit return value of print_limit

b541884

flip1995 reviewed Oct 2, 2020

View reviewed changes

flip1995 approved these changes Oct 10, 2020

View reviewed changes

bors merged commit dbc0285 into rust-lang:master Oct 10, 2020

bors mentioned this pull request Oct 10, 2020

Add linter for a single element for loop #6109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expands `manual_memcpy` to lint ones with loop counters #5727

Expands `manual_memcpy` to lint ones with loop counters #5727

rail-rain commented Jun 18, 2020 •

edited

Loading

rust-highfive commented Jun 18, 2020

flip1995 left a comment

flip1995 commented Jun 22, 2020

bors commented Jun 23, 2020

flip1995 commented Jun 28, 2020

rail-rain commented Jun 28, 2020

bors commented Jun 30, 2020

rail-rain commented Jul 1, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

rail-rain commented Jul 4, 2020

flip1995 commented Jul 13, 2020

flip1995 commented Jul 26, 2020

rail-rain commented Jul 28, 2020

rail-rain commented Aug 16, 2020 •

edited

Loading

flip1995 commented Aug 16, 2020

bors commented Sep 24, 2020

flip1995 left a comment

rail-rain commented Sep 27, 2020 •

edited

Loading

flip1995 commented Oct 1, 2020

flip1995 left a comment

flip1995 Oct 1, 2020

rail-rain Oct 1, 2020

flip1995 Oct 2, 2020

rail-rain Oct 2, 2020

flip1995 Oct 2, 2020

rail-rain Oct 2, 2020 •

edited

Loading

flip1995 commented Oct 10, 2020

bors commented Oct 10, 2020

bors commented Oct 10, 2020

bors commented Oct 10, 2020

		let print_limit =
		\|end: &Expr<'_>, end_str: &str, base: &Expr<'_>, sugg: MinifyingSugg<'static>\| -> MinifyingSugg<'static> {

Expands manual_memcpy to lint ones with loop counters #5727

Expands manual_memcpy to lint ones with loop counters #5727

Conversation

rail-rain commented Jun 18, 2020 • edited Loading

rust-highfive commented Jun 18, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 commented Jun 22, 2020

bors commented Jun 23, 2020

flip1995 commented Jun 28, 2020

rail-rain commented Jun 28, 2020

bors commented Jun 30, 2020

rail-rain commented Jul 1, 2020

flip1995 commented Jul 3, 2020

bors commented Jul 3, 2020

rail-rain commented Jul 4, 2020

flip1995 commented Jul 13, 2020

flip1995 commented Jul 26, 2020

rail-rain commented Jul 28, 2020

rail-rain commented Aug 16, 2020 • edited Loading

flip1995 commented Aug 16, 2020

bors commented Sep 24, 2020

flip1995 left a comment

Choose a reason for hiding this comment

rail-rain commented Sep 27, 2020 • edited Loading

flip1995 commented Oct 1, 2020

flip1995 left a comment

Choose a reason for hiding this comment

flip1995 Oct 1, 2020

Choose a reason for hiding this comment

rail-rain Oct 1, 2020

Choose a reason for hiding this comment

flip1995 Oct 2, 2020

Choose a reason for hiding this comment

rail-rain Oct 2, 2020

Choose a reason for hiding this comment

flip1995 Oct 2, 2020

Choose a reason for hiding this comment

rail-rain Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

flip1995 commented Oct 10, 2020

bors commented Oct 10, 2020

bors commented Oct 10, 2020

bors commented Oct 10, 2020

Expands `manual_memcpy` to lint ones with loop counters #5727

Expands `manual_memcpy` to lint ones with loop counters #5727

rail-rain commented Jun 18, 2020 •

edited

Loading

rail-rain commented Aug 16, 2020 •

edited

Loading

rail-rain commented Sep 27, 2020 •

edited

Loading

rail-rain Oct 2, 2020 •

edited

Loading