Refactor: organize loops file into loops module #6693

nahuakang · 2021-02-07T12:45:16Z

This WIP PR is an attempt to refactor files with many lints into a more accessible structure.

Notes:

All lint definitions, declare_clippy_lint! for lints and impl LintPass will reside in the loops/mod.rs.
Lint functions will reside in their corresponding files.
Utility functions of the loops module will be moved into loops/utils.rs and then reexported for the module in mod.rs.

This way, loops.rs will be re-arrange into the following structure under clippy_lints/src:

loops
├── manual_flatten.rs
├── mod.rs
├── ...other lints...
└── utils.rs

Followed [lint naming conventions][lint_naming]
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

Please write a short comment explaining your change (or "none" for internal only changes)
changelog:
Refactor loops.rs file into loops module.

rust-highfive · 2021-02-07T12:45:19Z

r? @phansch

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

nahuakang · 2021-02-09T22:44:33Z

r? @flip1995

bors · 2021-02-10T06:58:08Z

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

clippy_lints/src/loops/mod.rs

Y-Nak

About styles.

clippy_lints/src/loops/for_loop_arg.rs

clippy_lints/src/loops/for_loop_explicit_counter.rs

clippy_lints/src/loops/for_loop_arg.rs

flip1995 · 2021-02-21T16:05:27Z

@Y-Nak @nahuakang @magurotuna Thanks for your work and cross reviews. I'll do a final check of your PRs once this PR is ready. 👍

nahuakang · 2021-02-21T17:40:18Z

@Y-Nak @magurotuna @flip1995 Thanks for all the feedback. I've incorporated your comments as much as I can. This branch has also been updated with rust-lang:master and reflects the most recent changes to the loops.rs file from #6698. Let me know what you think!

Y-Nak · 2021-02-22T13:51:32Z

Could you use rebase instead of merge to resolve conflicts because we follow a rustc no merge-commit policy? See this for more details.

…ck_manual_flatten

…plicit counter to explicit counter loop

…lints to each file

bors · 2021-02-25T10:01:20Z

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

magurotuna

LGTM 👍 (except for the conflict)

Y-Nak · 2021-02-25T14:53:43Z

clippy_lints/src/loops/empty_loop.rs

+use rustc_hir::{Block, Expr};
+use rustc_lint::LateContext;
+
+pub(super) fn check_empty_loop(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'tcx Block<'_>) {


I think it would be great to unify the name of the main entry functions of all modules to check.

Y-Nak · 2021-02-25T15:12:19Z

clippy_lints/src/loops/mod.rs

+                if is_ref_iterable_type(cx, &args[0]) {
+                    explicit_iter_loop::lint_iter_method(cx, args, arg, method_name);
+                }


I think check_for_loop_arg should only decide which function to be called according to the method name. The current implementation of check_for_loop_arg seems to do too many works.
And is_ref_iterable_type should be called inside lint_iter_method because it's a quite detailed implementation of explicit_iter_loop.

Suggested change

if is_ref_iterable_type(cx, &args[0]) {

explicit_iter_loop::lint_iter_method(cx, args, arg, method_name);

}

explicit_iter_loop::lint_iter_method(cx, args, arg, method_name);

Y-Nak · 2021-02-25T15:30:08Z

clippy_lints/src/loops/mod.rs

+                let receiver_ty = cx.typeck_results().expr_ty(&args[0]);
+                let receiver_ty_adjusted = cx.typeck_results().expr_ty_adjusted(&args[0]);
+                if TyS::same_type(receiver_ty, receiver_ty_adjusted) {
+                    explicit_into_iter_loop::check_explicit_into_iter_loop(cx, args, arg);
+                } else {
+                    let ref_receiver_ty = cx.tcx.mk_ref(
+                        cx.tcx.lifetimes.re_erased,
+                        ty::TypeAndMut {
+                            ty: receiver_ty,
+                            mutbl: Mutability::Not,
+                        },
+                    );
+                    if TyS::same_type(receiver_ty_adjusted, ref_receiver_ty) {
+                        explicit_iter_loop::lint_iter_method(cx, args, arg, method_name)
+                    }


ditto.
The implemetation of methods/mod.rs would be great help to grab the idea.

Suggested change

let receiver_ty = cx.typeck_results().expr_ty(&args[0]);

let receiver_ty_adjusted = cx.typeck_results().expr_ty_adjusted(&args[0]);

if TyS::same_type(receiver_ty, receiver_ty_adjusted) {

explicit_into_iter_loop::check_explicit_into_iter_loop(cx, args, arg);

} else {

let ref_receiver_ty = cx.tcx.mk_ref(

cx.tcx.lifetimes.re_erased,

ty::TypeAndMut {

ty: receiver_ty,

mutbl: Mutability::Not,

},

);

if TyS::same_type(receiver_ty_adjusted, ref_receiver_ty) {

explicit_iter_loop::lint_iter_method(cx, args, arg, method_name)

}

explicit_into_iter_loop::check_explicit_into_iter_loop(cx, args, arg);

explicit_iter_loop::lint_iter_method(cx, args, arg, method_name);

nahuakang · 2021-02-25T15:54:17Z

Thanks a lot for the additional comments, @Y-Nak. I'm unfortunately on sick leave and will not work for the next 3-4 days. If you have time until then, could you please help address the comments? Otherwise I'll address them after :) Cheers.

flip1995 · 2021-03-02T09:05:05Z

Closing in favor of #6824

Refactor: organize loops file into loops module (Delegated) `@flip1995` `@nahuakang` closes #6693 r? `@flip1995` As we talked about in the PM of Zulip, this PR is a delegated PR from `@nahuakang.` Changes from the last commit of #6693: 1. Unify the name of the main entries of all modules to check, that was pointed out [here](#6693 (comment)) 2. Simplify ` check_for_loop_arg`, that was pointed out [here](#6693 (comment)) and [here](#6693 (comment)) 3. Resolve conflicts changelog: Refactor `loops.rs` file into `loops` module.

rust-highfive assigned phansch Feb 7, 2021

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

magurotuna mentioned this pull request Feb 7, 2021

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

Open

8 tasks

nahuakang changed the title ~~(WIP) Refactor: organize loops file into loops module~~ Refactor: organize loops file into loops module Feb 9, 2021

nahuakang marked this pull request as ready for review February 9, 2021 22:29

rust-highfive assigned flip1995 and unassigned phansch Feb 9, 2021

nahuakang force-pushed the refactor_loops_module branch from dd007fe to bb52d89 Compare February 9, 2021 23:26

Y-Nak reviewed Feb 16, 2021

View reviewed changes

clippy_lints/src/loops/for_loop_arg.rs Outdated Show resolved Hide resolved

clippy_lints/src/loops/for_loop_arg.rs Outdated Show resolved Hide resolved

clippy_lints/src/loops/for_loop_explicit_counter.rs Outdated Show resolved Hide resolved

flip1995 reviewed Feb 16, 2021

View reviewed changes

clippy_lints/src/loops/for_loop_arg.rs Outdated Show resolved Hide resolved

Y-Nak mentioned this pull request Feb 16, 2021

Refactor types lints #6725

Merged

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 Feb 21, 2021

nahuakang requested a review from flip1995 February 21, 2021 18:42

nahuakang added 9 commits February 22, 2021 16:03

create loops dir; arrange manual_flatten lint and utils

eeebe75

Refactor check_for_loop_arg; rename manual_flatten's lint back to che…

e521e52

…ck_manual_flatten

Refactor check_for_loop_over_map_kv to its own module

07dd4f7

Refactor check_for_mut_range_bound to its own module

e85f3c1

Refactor check_for_loop_range into its module

d37a042

Refactor check_for_loop_explicit_counter to its own module

abdd5f4

Refactor check_for_single_element_loop to its own module

51d4678

Add check_infinite_loop to its own module

2050b41

Add check_needless_collect to its own module

6fa5147

nahuakang added 10 commits February 22, 2021 16:06

Add detect_same_item_push to its own module

2ad6cc8

Move detect_manual_memcpy to its module; split up utils structs

7d02cd9

Refactor never loop to its own module

f06f9bc

Refactor while let on iterator lint to its module; rename for loop ex…

19c4de5

…plicit counter to explicit counter loop

Refactor empty loop to its own module

d07539a

Refactor while let loop to its own module

d08d3e2

Move MinifyingSugg into manual_memcpy

7d5dfcc

Move check_for_loop_arg back to mod; split into 4 lint files

2e9f90b

Clean up: Rename some files to be consistent with lint names; import …

d8eeb97

…lints to each file

Include loops.rs changes from PR#6698

fee0c89

nahuakang force-pushed the refactor_loops_module branch from 85826ea to fee0c89 Compare February 22, 2021 15:24

magurotuna approved these changes Feb 25, 2021

View reviewed changes

Y-Nak reviewed Feb 25, 2021

View reviewed changes

Y-Nak mentioned this pull request Mar 2, 2021

Refactor: organize loops file into loops module (Delegated) #6824

Merged

flip1995 closed this Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: organize loops file into loops module #6693

Refactor: organize loops file into loops module #6693

nahuakang commented Feb 7, 2021 •

edited

Loading

rust-highfive commented Feb 7, 2021

nahuakang commented Feb 9, 2021

bors commented Feb 10, 2021

Y-Nak left a comment

flip1995 commented Feb 21, 2021

nahuakang commented Feb 21, 2021

Y-Nak commented Feb 22, 2021

bors commented Feb 25, 2021

magurotuna left a comment

Y-Nak Feb 25, 2021

Y-Nak Feb 25, 2021 •

edited

Loading

Y-Nak Feb 25, 2021 •

edited

Loading

nahuakang commented Feb 25, 2021

flip1995 commented Mar 2, 2021

Refactor: organize loops file into loops module #6693

Refactor: organize loops file into loops module #6693

Conversation

nahuakang commented Feb 7, 2021 • edited Loading

rust-highfive commented Feb 7, 2021

nahuakang commented Feb 9, 2021

bors commented Feb 10, 2021

Y-Nak left a comment

Choose a reason for hiding this comment

flip1995 commented Feb 21, 2021

nahuakang commented Feb 21, 2021

Y-Nak commented Feb 22, 2021

bors commented Feb 25, 2021

magurotuna left a comment

Choose a reason for hiding this comment

Y-Nak Feb 25, 2021

Choose a reason for hiding this comment

Y-Nak Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Y-Nak Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

nahuakang commented Feb 25, 2021

flip1995 commented Mar 2, 2021

nahuakang commented Feb 7, 2021 •

edited

Loading

Y-Nak Feb 25, 2021 •

edited

Loading

Y-Nak Feb 25, 2021 •

edited

Loading