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.

Sign up for GitHub

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

Jump to bottom

feat(new lint): new lint `manual_retain` #8972

Merged

bors merged 10 commits into rust-lang:master from kyoto7250:use_retain

Jun 27, 2022

Contributor

kyoto7250 commented Jun 8, 2022 •

edited by llogiq

Loading

close #8097

This PR is a new lint implementation.
This lint checks if the retain method is available.

Thank you in advance.

changelog: add new [`manual_retain`] lint

rust-highfive assigned llogiq

rust-highfive commented Jun 8, 2022

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive added the S-waiting-on-review label

kyoto7250 force-pushed the use_retain branch 2 times, most recently from 1f94ae4 to 983c443 Compare

June 8, 2022 15:48

kyoto7250 marked this pull request as ready for review

June 8, 2022 16:01

llogiq reviewed

View reviewed changes

Contributor

llogiq left a comment

This looks pretty good. I only have some small-ish nits regarding code style and one suggestion for extending the test. With those out of the way this is good to merge.

clippy_lints/src/use_retain.rs Outdated Show resolved Hide resolved

clippy_lints/src/use_retain.rs Outdated

+                  /// ```
+                  #[clippy::version = "1.63.0"]
+                  pub USE_RETAIN,
+                  style,

Contributor

llogiq Jun 8, 2022

I don't think this is a question of style. I'd place it in either performance (because it helps avoid allocating a new container) or complexity (because it simplifies the code) with a small preference to the former.

clippy_lints/src/use_retain.rs Outdated

+              impl<'tcx> LateLintPass<'tcx> for UseRetain {
+                  fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
+                      if_chain! {

Contributor

llogiq Jun 8, 2022

We can now use "let chains". That is we can if let ... && let ... && foo ... without any macro. Just replace the second and following ifs by &&, lose the ; in the end and the then and it should work.

clippy_lints/src/use_retain.rs Outdated

+                  left_expr: &hir::Expr<'_>,
+                  target_expr: &hir::Expr<'_>,
+              ) {
+                  if_chain! {

Contributor

llogiq Jun 8, 2022

Feel free to also use let chains here.

clippy_lints/src/use_retain.rs Outdated

+                  left_expr: &hir::Expr<'_>,
+                  target_expr: &hir::Expr<'_>,
+              ) {
+                  if_chain! {

Contributor

llogiq Jun 8, 2022

and here.

clippy_lints/src/use_retain.rs Outdated

+                  left_expr: &hir::Expr<'_>,
+                  target_expr: &hir::Expr<'_>,
+              ) {
+                  if_chain! {

Contributor

llogiq Jun 8, 2022

need I say more? 😄

clippy_lints/src/use_retain.rs Outdated

+              }
+              fn suggest(cx: &LateContext<'_>, parent_expr: &hir::Expr<'_>, left_expr: &hir::Expr<'_>, filter_expr: &hir::Expr<'_>) {
+                  if_chain! {

Contributor

llogiq Jun 8, 2022

Another one

clippy_lints/src/use_retain.rs Outdated

Comment on lines 223 to 225

+                  return ACCEPTABLE_METHODS
+                      .iter()
+                      .any(|&method| match_def_path(cx, collect_def_id, method));

Contributor

llogiq Jun 8, 2022

Suggested change

      
                return ACCEPTABLE_METHODS
          
                    .iter()
          
                    .any(|&method| match_def_path(cx, collect_def_id, method));
          
                ACCEPTABLE_METHODS
          
                    .iter()
          
                    .any(|&method| match_def_path(cx, collect_def_id, method))

Why wasn't this caught by needless-return!?

Contributor Author

kyoto7250 Jun 9, 2022

I made the bug ticket, I'll check when I have time.

clippy_lints/src/use_retain.rs Outdated

Comment on lines 230 to 232

+                  return ACCEPTABLE_TYPES
+                      .iter()
+                      .any(|&ty| is_type_diagnostic_item(cx, expr_ty, ty));

Contributor

llogiq Jun 8, 2022

Suggested change

      
                return ACCEPTABLE_TYPES
          
                    .iter()
          
                    .any(|&ty| is_type_diagnostic_item(cx, expr_ty, ty));
          
                ACCEPTABLE_TYPES
          
                    .iter()
          
                    .any(|&ty| is_type_diagnostic_item(cx, expr_ty, ty))

same here

tests/ui/use_retain.rs Outdated

+                  // https://github.com/rust-lang/rust/issues/71503
+                  let mut heap = BinaryHeap::from([1, 2, 3]);
+                  heap = heap.into_iter().filter(|x| x % 2 == 0).collect();
+                  heap = heap.iter().filter(|&x| x % 2 == 0).copied().collect();

Contributor

llogiq Jun 8, 2022

Perhaps also one case with .cloned() instead of .copied() (allowing the respective lint of course)?

ghost commented Jun 9, 2022 •

edited by ghost

Loading

The name does follow the naming guidelines. It should probably be called something like 'manual_retain' or 'slow retain'.

--

I ran this on the top 1000 (more or less) crates of crates.io.

I got one warning and the fix worked so that is looking good.

---> annotate-snippets-0.9.1/src/display_list/from_snippet.rs:344:9   
warning: this expression can be written more simply using `.retain()` 
   --> src/display_list/from_snippet.rs:344:9
    |
344 | /         annotations = annotations
345 | |             .into_iter()
346 | |             .filter(|annotation| {
347 | |                 let body_idx = idx + annotation_line_count;
...   |
507 | |             })
508 | |             .collect();
    | |______________________^
    |
    = note: requested on the command line with `-W clippy::use-retain`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#use_retain
help: consider calling `.retain()` instead
    |
344 ~         annotations.retain(|annotation| {
345 +                 let body_idx = idx + annotation_line_count;
346 +                 let annotation_type = match annotation.annotation_type {
347 +                     snippet::AnnotationType::Error => DisplayAnnotationType::None,
348 +                     snippet::AnnotationType::Warning => DisplayAnnotationType::None,
349 +                     _ => DisplayAnnotationType::from(annotation.annotation_type),
  ...
annotate-snippets-0.9.1 - fix succeeded

Contributor Author

kyoto7250 commented Jun 10, 2022

The name does follow the naming guidelines. It should probably be called something like 'manual_retain' or 'slow retain'.

yes, I thinkmanual_retain would be better.
I'm going to rename, thanks for letting me know!

I ran this on the top 1000 (more or less) crates of crates.io.
I got one warning and the fix worked so that is looking good.

Thanks for this too!

Contributor Author

kyoto7250 commented Jun 10, 2022 •

edited

Loading

I added some commits for your suggestions and I changed the lint name to manual_retain.

In addition to, I noticed that this lint needs to check msrv, so I added this commit.

Please let me know if we need git squash :)

kyoto7250 requested a review from llogiq

June 12, 2022 12:24

Contributor

bors commented Jun 15, 2022

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

kyoto7250 force-pushed the use_retain branch from d7ffab1 to f098f8c Compare

June 15, 2022 07:05

Contributor

bors commented Jun 17, 2022

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

kyoto7250 force-pushed the use_retain branch from f098f8c to 1601a41 Compare

June 18, 2022 09:57

Contributor

bors commented Jun 24, 2022

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

llogiq changed the title ~~feat(new lint): new lint use_retain~~ feat(new lint): new lint manual_retain

Contributor

llogiq commented Jun 25, 2022

Sorry, I had a lot to do this week, so I didn't get to review stuff. This looks good and can be merged after a rebase.

kyoto7250 force-pushed the use_retain branch from 1601a41 to c853f57 Compare

June 26, 2022 06:38

kyoto7250 and others added 9 commits

June 27, 2022 08:11


          feat(new lint): new lint use_retain

5f2b8e6


          Update clippy_lints/src/use_retain.rs

b20b8f1

Co-authored-by: llogiq <[email protected]>


          remove needless return

e3afc72


          change lint type from style to perf

3953c53


          check method

fd629c0


          rewrite without if_chain macro


          check msrv

4decfde


          rename use_retain => manual_retain

dd3d0fd


          fix for git rebase

3a9c0ef

kyoto7250 force-pushed the use_retain branch from c853f57 to 3a9c0ef Compare

June 26, 2022 23:12


          cargo dev update_lints

676af45

Contributor Author

kyoto7250 commented Jun 26, 2022 •

edited

Loading

This looks good and can be merged after a rebase.

I rebased latest master.

Do We need a git squash?

Sorry, I had a lot to do this week, so I didn't get to review stuff.

I don't mind.

Thanks again for your reviews :)

Contributor

llogiq commented Jun 27, 2022

Contributor

bors commented Jun 27, 2022

📌 Commit 676af45 has been approved by llogiq

Contributor

bors commented Jun 27, 2022

⌛ Testing commit 676af45 with merge eaa03ea...

Contributor

bors commented Jun 27, 2022

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

bors merged commit eaa03ea into rust-lang:master

kyoto7250 mentioned this pull request

want to lint binary_heap in manual_retain when API is stabilized #9059

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review