Refactor: arrange lints in `methods` module #6826

TaKO8Ki · 2021-03-02T15:21:46Z

This PR arranges methods lints so that they can be accessed more easily.
Basically, I refactored them following the instruction described in #6680.

changelog: Move lints in methods module into their own modules.

rust-highfive · 2021-03-02T15:21:49Z

r? @phansch

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

phansch

I really like these refactors! Feel free to @ me again, when you're ready for a review =)

TaKO8Ki · 2021-03-02T15:56:31Z

@phansch
Thank you! :)

camsteffen · 2021-03-11T05:31:43Z

You should probably split into a new PR every 10 or so lints for the reviewers' sanity 😄

TaKO8Ki · 2021-03-11T05:44:39Z

@camsteffen
You’re right. I've already refactored most of lints in methods module. The others things to do is refactoring the rest of lints and moving some functions to clippy_lints/src/utils/methods/. I'm going to do them in a new PR!

TaKO8Ki · 2021-03-11T05:46:39Z

@phansch
I'm ready for your review! ref: comment above

phansch · 2021-03-11T07:23:04Z

@TaKO8Ki great! I will try to finish the review in the next 12 hours so that we don't run in too many conflicts with other PRs

flip1995 · 2021-03-11T08:56:15Z

clippy_lints/src/methods/mod.rs

-        }
-    }
-}
-
 fn fn_header_equals(expected: hir::FnHeader, actual: hir::FnHeader) -> bool {


If those utils functions are used in more than one lint, please move it to a methods/utils.rs module. If it is only used in one lint, move it to the lint module. If it is only used in mod.rs, just leave it here.

A quick search would suggest, that this function should stay in mod.rs, but e.g. derefs_to_slice is used in multiple lint files and should therefore be moved to methods/utils.rs

You're right. I think I should move some functions such as derefs_to_slice to methods/utils.rs, however this PR is too big. So, I'm going to fix this in a new PR. ref comment above

Y-Nak · 2021-03-11T09:13:55Z

clippy_lints/src/methods/mod.rs

                let self_ty = cx.typeck_results().expr_ty_adjusted(&args[0]);
                if args.len() == 1 && method_call.ident.name == sym::clone {
-                    lint_clone_on_copy(cx, expr, &args[0], self_ty);
-                    lint_clone_on_ref_ptr(cx, expr, &args[0]);
+                    clone_on_copy::check(cx, expr, &args[0], self_ty);
+                    clone_on_ref_ptr::check(cx, expr, &args[0]);
                }
                if args.len() == 1 && method_call.ident.name == sym!(to_string) {
-                    inefficient_to_string::lint(cx, expr, &args[0], self_ty);
+                    inefficient_to_string::check(cx, expr, &args[0], self_ty);
                }

                if let Some(fn_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) {
                    if match_def_path(cx, fn_def_id, &paths::PUSH_STR) {
-                        lint_single_char_push_string(cx, expr, args);
+                        single_char_push_string::check(cx, expr, args);
                    } else if match_def_path(cx, fn_def_id, &paths::INSERT_STR) {
-                        lint_single_char_insert_string(cx, expr, args);
+                        single_char_insert_string::check(cx, expr, args);
                    }
                }

                match self_ty.kind() {
                    ty::Ref(_, ty, _) if *ty.kind() == ty::Str => {
                        for &(method, pos) in &PATTERN_METHODS {
                            if method_call.ident.name.as_str() == method && args.len() > pos {
-                                lint_single_char_pattern(cx, expr, &args[pos]);
+                                single_char_pattern::check(cx, expr, &args[pos]);
                            }
                        }
                    },
                    ty::Ref(..) if method_call.ident.name == sym::into_iter => {
-                        lint_into_iter(cx, expr, self_ty, *method_span);
+                        into_iter_on_ref::check(cx, expr, self_ty, *method_span);
                    },
                    _ => (),
                }


These lines seem to do too many tasks.
I think checks that decide whether the lint is triggered should be moved to each module if duplications are not so much.

Thank you for your suggestions! I think so too. However, this PR is too big and I have to commit many changes in order to move them to each module. So I'm going to do it in a new PR 👍 ref: #6826 (comment)

bors · 2021-03-11T10:22:42Z

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

TaKO8Ki · 2021-03-11T10:23:30Z

for all reviewers

This PR is too big. So, if your suggestions are big, I'm going to accept them in a new PR. ref #6826 (comment)

flip1995 · 2021-03-11T10:25:51Z

@TaKO8Ki sounds good to me! (And sorry for the merge conflict (-.-) )

phansch · 2021-03-11T10:33:11Z

I agree that we should keep further improvements out of this PR 👍

Y-Nak · 2021-03-11T10:37:01Z

I agree that too.

…to their own modules

TaKO8Ki · 2021-03-11T11:30:20Z

@flip1995 @phansch @Y-Nak
I finished resolving conflicts!

Y-Nak · 2021-03-11T14:03:45Z

@TaKO8Ki Could you summarize the future works and reviews you received here in #6680 or a new issue?
It really helps us to track them easily.

TaKO8Ki · 2021-03-11T15:29:45Z

@Y-Nak
I opened a tracking issue #6886.

phansch

Left some comments unrelated to this refactoring that can be fixed in later PRs

phansch · 2021-03-11T19:13:08Z

clippy_lints/src/methods/filter_map_flat_map.rs

+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx hir::Expr<'_>,
+    _filter_args: &'tcx [hir::Expr<'_>],


We should review all new functions for unused arguments

phansch · 2021-03-11T19:24:36Z

clippy_lints/src/methods/iter_next_slice.rs

+    } else if is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(caller_expr), sym::vec_type)
+        || matches!(
+            &cx.typeck_results().expr_ty(caller_expr).peel_refs().kind(),
+            ty::Array(_, _)
+        )


This condition (not the body) could probably be extracted into its own function

phansch · 2021-03-11T19:26:39Z

clippy_lints/src/methods/iter_skip_next.rs

+    // lint if caller of skip is an Iterator
+    if match_trait_method(cx, expr, &paths::ITERATOR) {
+        if let [caller, n] = skip_args {
+            let hint = format!(".nth({})", snippet(cx, n.span, ".."));


No need to have this as a separate variable

phansch · 2021-03-11T19:30:06Z

clippy_lints/src/methods/ok_expect.rs

+}
+
+/// This checks whether a given type is known to implement Debug.
+fn has_debug_impl<'tcx>(ty: Ty<'tcx>, cx: &LateContext<'tcx>) -> bool {


Might be worth moving to utils

phansch · 2021-03-11T19:36:30Z

clippy_lints/src/methods/string_extend_chars.rs

+
+pub(super) fn check(cx: &LateContext<'_>, expr: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
+    let obj_ty = cx.typeck_results().expr_ty(&args[0]).peel_refs();
+    if is_type_diagnostic_item(cx, obj_ty, sym::string_type) {


Returning early here or using if_chain would reduce some indentation in a lot of lints, like here.

phansch · 2021-03-11T19:54:43Z

@bors r+ This is a huge step forward, thanks!

bors · 2021-03-11T19:54:44Z

📌 Commit 83a9553 has been approved by phansch

bors · 2021-03-11T19:54:52Z

⌛ Testing commit 83a9553 with merge 6ed6f1e...

bors · 2021-03-11T20:06:41Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: phansch
Pushing 6ed6f1e to master...

…nsch Refactor lints in methods module This PR refactors methods lints other than the lints I refactored in #6826 and moves some functions to methods/utils.rs. Basically, I follow the instruction described in #6680. **For ease of review, I refactored step by step, keeping each commit small.** closes #6886 cc: `@phansch,` `@flip1995,` `@Y-Nak` changelog: Move lints in methods module to their own modules and some function to methods/utils.rs.

rust-highfive assigned phansch Mar 2, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 2, 2021

TaKO8Ki changed the title ~~[WIP] Refactor: arrange methods lints~~ [WIP] Refactor: arrange lints in methods mod Mar 2, 2021

phansch reviewed Mar 2, 2021

View reviewed changes

TaKO8Ki changed the title ~~[WIP] Refactor: arrange lints in methods mod~~ [WIP] Refactor: arrange lints in methods module Mar 2, 2021

TaKO8Ki changed the title ~~[WIP] Refactor: arrange lints in methods module~~ [WIP] Refactor: arrange lints in methods module Mar 2, 2021

magurotuna mentioned this pull request Mar 3, 2021

[Tracking issue] Rearrange location of some of the existing lints #6680

Open

8 tasks

TaKO8Ki force-pushed the refactor-methods-mod branch from 81d4dbe to 62a1119 Compare March 4, 2021 10:47

TaKO8Ki changed the title ~~[WIP] Refactor: arrange lints in methods module~~ Refactor: arrange lints in methods module Mar 11, 2021

TaKO8Ki requested a review from phansch March 11, 2021 05:44

flip1995 reviewed Mar 11, 2021

View reviewed changes

Y-Nak reviewed Mar 11, 2021

View reviewed changes

TaKO8Ki added 7 commits March 11, 2021 19:37

move expect_used, filter_next, get_unwrap, ok_expect and unwrap_used …

f0f07ac

…to their own modules

move skip_while_next to its own module

483bac2

move option_as_ref_deref to its own module

110dac7

move suspicious_map to its own module

60a0537

move filetype_is_file to its own module

8623b33

move uninit_assumed_init to its own module

35147d4

move iter_cloned_collect to its own module

45ee914

TaKO8Ki added 11 commits March 11, 2021 19:40

move flat_map_identity to its own module

37ba779

move map_flatten and search_is_some to their own modules

24909fa

move iter_skip_next to its own module

171c4c1

move clone_on_copy to its own module

caaba82

move useless_asref to its own module

78e572c

unnecessary_fold to its own module

bbed852

move option_map_or_none to its own module

5557596

move map_unwrap_or to its own module

b0824bf

move or_fun_call to its own module

f49349b

move expect_fun_call to its own module

c711de2

remove unused imports

99f8607

TaKO8Ki force-pushed the refactor-methods-mod branch from d502a56 to 99f8607 Compare March 11, 2021 11:02

fix interning-defined-symbol error

83a9553

TaKO8Ki requested a review from flip1995 March 11, 2021 11:30

TaKO8Ki mentioned this pull request Mar 11, 2021

[Tracking Issue] Refactor lints in methods module #6886

Closed

8 tasks

phansch approved these changes Mar 11, 2021

View reviewed changes

bors merged commit 6ed6f1e into rust-lang:master Mar 11, 2021

This was referenced Mar 11, 2021

Lint: filter(Option::is_some).map(Option::unwrap) #6342

Merged

New Lint: needless_for_each #6706

Merged

wrong_self_convention: fix lint in case of to_*_mut method #6828

Merged

TaKO8Ki deleted the refactor-methods-mod branch March 12, 2021 02:49

TaKO8Ki mentioned this pull request Mar 13, 2021

Refactor lints in methods module #6896

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: arrange lints in `methods` module #6826

Refactor: arrange lints in `methods` module #6826

TaKO8Ki commented Mar 2, 2021 •

edited

Loading

rust-highfive commented Mar 2, 2021

phansch left a comment

TaKO8Ki commented Mar 2, 2021 •

edited

Loading

camsteffen commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021

phansch commented Mar 11, 2021

flip1995 Mar 11, 2021

flip1995 Mar 11, 2021

TaKO8Ki Mar 11, 2021

Y-Nak Mar 11, 2021

TaKO8Ki Mar 11, 2021

bors commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021

flip1995 commented Mar 11, 2021

phansch commented Mar 11, 2021

Y-Nak commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021 •

edited

Loading

Y-Nak commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021 •

edited

Loading

phansch left a comment

phansch Mar 11, 2021

phansch Mar 11, 2021

phansch Mar 11, 2021

phansch Mar 11, 2021

phansch Mar 11, 2021

phansch commented Mar 11, 2021

bors commented Mar 11, 2021

bors commented Mar 11, 2021

bors commented Mar 11, 2021

Refactor: arrange lints in methods module #6826

Refactor: arrange lints in methods module #6826

Conversation

TaKO8Ki commented Mar 2, 2021 • edited Loading

rust-highfive commented Mar 2, 2021

phansch left a comment

Choose a reason for hiding this comment

TaKO8Ki commented Mar 2, 2021 • edited Loading

camsteffen commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021

phansch commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021

flip1995 commented Mar 11, 2021

phansch commented Mar 11, 2021

Y-Nak commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021 • edited Loading

Y-Nak commented Mar 11, 2021

TaKO8Ki commented Mar 11, 2021 • edited Loading

phansch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansch commented Mar 11, 2021

bors commented Mar 11, 2021

bors commented Mar 11, 2021

bors commented Mar 11, 2021

Refactor: arrange lints in `methods` module #6826

Refactor: arrange lints in `methods` module #6826

TaKO8Ki commented Mar 2, 2021 •

edited

Loading

TaKO8Ki commented Mar 2, 2021 •

edited

Loading

TaKO8Ki commented Mar 11, 2021 •

edited

Loading

TaKO8Ki commented Mar 11, 2021 •

edited

Loading