Refactor lints in methods module #6896
Conversation
|
r? @llogiq (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
TaKO8Ki
changed the title
|
r? @phansch since you reviewed the last PR and know the context. |
|
☔ The latest upstream changes (presumably #6907) made this pull request unmergeable. Please resolve the merge conflicts. |
TaKO8Ki
force-pushed
the
refactor-lints-in-methods-module
branch
from
3c8152d to
08cc1f7
Compare
TaKO8Ki
changed the title
| use rustc_span::sym; | ||
|
|
||
| /// Wrapper fn for `CHARS_NEXT_CMP` and `CHARS_LAST_CMP` lints. | ||
| pub(super) fn check( |
There was a problem hiding this comment.
I wonder if chars_last_cmp::check and chars_next_cmp::check should be merged into this because they are almost the same and depend on each other. I think separating them in this way decreases readability and understandability. This also applies to chars_cmp_with_unwrap.
There was a problem hiding this comment.
I agree with you. I'm going to merge chars_last_cmp and chars_next_cmp and merge chars_next_cmp_with_unwrap and chars_last_cmp_with_unwrap.
There was a problem hiding this comment.
I'm going to do it in a new PR.
|
☔ The latest upstream changes (presumably #6918) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sorry for the merge conflicts. Resolving those should be quite easy though, since #6918 only changed some imports. |
TaKO8Ki
force-pushed
the
refactor-lints-in-methods-module
branch
from
c3d0476 to
b949d84
Compare
…wrap to their own modules
TaKO8Ki
force-pushed
the
refactor-lints-in-methods-module
branch
from
b949d84 to
602bcf3
Compare
|
@bors r+ Thanks for your patience! |
|
📌 Commit b6a2757 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Destructure args in `methods`
changelog: none
This changes the main pattern in `methods` to match and destructure the method call args at the same time as the method name, and pass individual arg `Expr`s to the lint impls.
```rust
// before
["expect", ..] => expect::check(cx, expr, arg_lists[0]);
// after
("expect", [arg]) => expect::check(cx, expr, recv, arg);
```
This makes the code safer since there is no risk of out of bounds `args[n]` everywhere. There will be no more collecting `method_names`, `arg_lists`, `method_spans` as a separate step - everything comes out of the `match`es. Chained methods are parsed in a nested `match`. This makes the code more verbose in some ways, but IMO it is much easier to follow.
~Definitely should wait for #6896. Just putting out the idea.~
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.