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

#5626: lint iterator.map(|x| x) #5694

Merged
merged 2 commits into from
Jun 23, 2020
Merged

#5626: lint iterator.map(|x| x) #5694

merged 2 commits into from
Jun 23, 2020

Conversation

theo-lw
Copy link
Contributor

@theo-lw theo-lw commented Jun 8, 2020

changelog: adds a new lint for iterator.map(|x| x) (see #5626)

The code also lints for result.map(|x| x) and option.map(|x| x). Also, I'm not sure if I'm checking for type adjustments correctly and I can't think of an example where .map(|x| x) would apply type adjustments.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthiaskrgr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 8, 2020
src/driver.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jun 8, 2020

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

@matthiaskrgr
Copy link
Member

I'm wondering if .map(std::convert::identity) (docs) should lint as well.

@bors
Copy link
Contributor

bors commented Jun 9, 2020

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

@matthiaskrgr
Copy link
Member

Could you please rebase and remove the merge commits?

@theo-lw theo-lw force-pushed the issue-5626 branch 5 times, most recently from 56d15ab to a763836 Compare June 10, 2020 01:55
@theo-lw
Copy link
Contributor Author

theo-lw commented Jun 10, 2020

Could you please rebase and remove the merge commits?

Done. There's still a merge at the end to get rid of the merge conflict though. Also just asking because I don't know git that well, is there a reason why those merge commits aren't wanted?

@matthiaskrgr
Copy link
Member

matthiaskrgr commented Jun 10, 2020

AFAIK we follow rustcs no-merge policy https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#pull-requests

edit: Alternatively I could squash-merge the entire pr as a single commit.

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.

@wangtheo If you have difficulties with rebasing, we can do this for you. In fact, I pushed a branch on my fork, where I squashed this PR in one commit: https://github.com/flip1995/rust-clippy/tree/issue-5626

clippy_lints/src/map_identity.rs Outdated 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 Jun 10, 2020
@theo-lw theo-lw force-pushed the issue-5626 branch 2 times, most recently from e386261 to 4bf66ac Compare June 13, 2020 06:07
@theo-lw
Copy link
Contributor Author

theo-lw commented Jun 13, 2020

AFAIK we follow rustcs no-merge policy https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#pull-requests

edit: Alternatively I could squash-merge the entire pr as a single commit.

Oh that's interesting, I'll keep that in mind.

@wangtheo If you have difficulties with rebasing, we can do this for you. In fact, I pushed a branch on my fork, where I squashed this PR in one commit: https://github.com/flip1995/rust-clippy/tree/issue-5626

I tried rebasing again and I think I managed to get rid of the merge commits 😃

Also it looks like the branch isn't building and I'm not too sure why.

@flip1995
Copy link
Member

flip1995 commented Jun 13, 2020

I think a squash commit would be reasonable here. (There's still one unrelated commit.)

How to squash:

git rebase -i upstream/master  # assuming upstream is the remote of the rust-lang/rust-clippy repo
# editor will open
# change pick -> reword in the first line
# change pick -> fixup in every other line
# save and close editor
# a new editor instance will open, where you can adjust the commit message

Also it looks like the branch isn't building and I'm not too sure why.

Don't worry about that, this will be fixed in another PR (you may have to rebase after that though)

@theo-lw
Copy link
Contributor Author

theo-lw commented Jun 13, 2020

I think a squash commit would be reasonable here. (There's still one unrelated commit.)

How to squash:

git rebase -i upstream/master  # assuming upstream is the remote of the rust-lang/rust-clippy repo
# editor will open
# change pick -> reword in the first line
# change pick -> fixup in every other line
# save and close editor
# a new editor instance will open, where you can adjust the commit message

Also it looks like the branch isn't building and I'm not too sure why.

Don't worry about that, this will be fixed in another PR (you may have to rebase after that though)

Followed your instructions and squashed the commit, thanks for the help 😃

@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 13, 2020
@flip1995
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 23, 2020

📌 Commit 40ee620 has been approved by flip1995

@bors
Copy link
Contributor

bors commented Jun 23, 2020

🌲 The tree is currently closed for pull requests below priority 10, this pull request will be tested once the tree is reopened

@flip1995
Copy link
Member

@bors treeclosed-

@bors
Copy link
Contributor

bors commented Jun 23, 2020

⌛ Testing commit 40ee620 with merge fda4916...

bors added a commit that referenced this pull request Jun 23, 2020
#5626: lint iterator.map(|x| x)

changelog: adds a new lint for iterator.map(|x| x) (see #5626)

The code also lints for result.map(|x| x) and option.map(|x| x). Also, I'm not sure if I'm checking for type adjustments correctly and I can't think of an example where .map(|x| x) would apply type adjustments.
@bors
Copy link
Contributor

bors commented Jun 23, 2020

💔 Test failed - checks-action_test

@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-bors Status: The marked PR was approved and is only waiting bors labels Jun 23, 2020
@matthiaskrgr
Copy link
Member

   Compiling clippy_lints v0.0.212 (/home/runner/work/rust-clippy/rust-clippy/clippy_lints)
error[E0023]: this pattern has 3 fields, but the corresponding tuple variant has 4 fields
    --> clippy_lints/src/map_identity.rs:64:16
     |
64   |         if let ExprKind::MethodCall(ref method, _, ref args) = expr.kind;
     |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 4 fields, found 3

error: aborting due to previous error

@matthiaskrgr
Copy link
Member

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 23, 2020

📌 Commit fb4f9a0 has been approved by matthiaskrgr

@bors
Copy link
Contributor

bors commented Jun 23, 2020

⌛ Testing commit fb4f9a0 with merge 583d644...

@bors
Copy link
Contributor

bors commented Jun 23, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: matthiaskrgr
Pushing 583d644 to master...

@bors bors merged commit 583d644 into rust-lang:master Jun 23, 2020
bors added a commit that referenced this pull request Oct 8, 2020
…licy, r=Manishearth

Add note that we follow a rustc no merge-commit policy

I think it would be better to add a note that we follow a rustc no merge-commit policy. For example, it was mentioned at #5694 (comment).

changelog: none
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.

5 participants