Skip to content
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.

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

Destructure args in methods #6913

Merged
merged 1 commit into from
Mar 31, 2021
Merged

Destructure args in methods #6913

merged 1 commit into from
Mar 31, 2021

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Mar 15, 2021

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 Exprs to the lint impls.

// 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 matches. 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.

@rust-highfive
Copy link

r? @flip1995

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 15, 2021
@bors
Copy link
Collaborator

bors commented Mar 22, 2021

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

@flip1995 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 Mar 23, 2021
@camsteffen camsteffen changed the title WIP: Destructure args in methods Destructure args in methods Mar 23, 2021
@camsteffen camsteffen marked this pull request as ready for review March 23, 2021 16:46
@camsteffen camsteffen added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 23, 2021
@bors
Copy link
Collaborator

bors commented Mar 24, 2021

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

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda disliked the first version of this PR, because I thought it made the logic harder. But I really like this version now. Great work!

clippy_lints/src/methods/mod.rs Show resolved Hide resolved
@flip1995 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 Mar 31, 2021
@bors
Copy link
Collaborator

bors commented Mar 31, 2021

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

@camsteffen
Copy link
Contributor Author

@bors r=flip1995

@bors
Copy link
Collaborator

bors commented Mar 31, 2021

📌 Commit 2108387 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Mar 31, 2021

⌛ Testing commit 2108387 with merge 487c2e8...

@bors
Copy link
Collaborator

bors commented Mar 31, 2021

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

@bors bors merged commit 487c2e8 into rust-lang:master Mar 31, 2021
@camsteffen camsteffen deleted the method-chain branch July 8, 2021 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants