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

new unnecessary_map_on_constructor lint #11413

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

jonboh
Copy link
Contributor

@jonboh jonboh commented Aug 26, 2023

changelog: [unnecessary_map_on_constructor]: adds lint for cases in which map is not necessary. Some(4).map(myfunction) => Some(myfunction(4))

Closes #6472

Note that the case mentioned in the issue Some(..).and_then(|..| Some(..)) is fixed by a chain of lint changes. This PR completes the last part of that chain.

By bind_instead_of_maplint:
Some(4).and_then(|x| Some(foo(4))) => Some(4).map(|x| foo)

By redundant_closure lint:
Some(4).map(|x| foo) => Some(4).map(fun)

Finally by this PR unnecessary_map_on_constructor:
Some(4).map(fun) => Some(fun(4))

I'm not sure this is the desired behavior for clippy and if it should be addressed in another issue/PR. I'd be up to give it a try if that's the case.

@rustbot
Copy link
Collaborator

rustbot commented Aug 26, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 26, 2023
@jonboh jonboh force-pushed the master branch 2 times, most recently from 70ea595 to 9cf4829 Compare August 26, 2023 17:48
@bors
Copy link
Collaborator

bors commented Sep 1, 2023

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

Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, got a couple macro cases but mostly style nits

tests/ui/unnecessary_map_on_constructor.rs Show resolved Hide resolved
tests/ui/unnecessary_map_on_constructor.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_map_on_constructor.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_map_on_constructor.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_map_on_constructor.rs Outdated Show resolved Hide resolved
clippy_lints/src/unnecessary_map_on_constructor.rs Outdated Show resolved Hide resolved
@jonboh
Copy link
Contributor Author

jonboh commented Sep 2, 2023

Thanks for the comments :), I think I've addressed all issues.

From the Zulip thread I get that the new syntax for the comments in the UI tests is still in the works, I've removed the comments, but if there's already some agreed upon syntax that I can add to be aligned with future practices let me know.

Copy link
Member

@Alexendoo Alexendoo left a comment

Choose a reason for hiding this comment

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

Thanks! just a couple last things I spotted and it looks good. If you could squash the commits too that would be great then it's ready for merging

tests/ui/unnecessary_map_on_constructor.rs Show resolved Hide resolved
clippy_lints/src/unnecessary_map_on_constructor.rs Outdated Show resolved Hide resolved
@rustbot
Copy link
Collaborator

rustbot commented Sep 12, 2023

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

@Alexendoo
Copy link
Member

Great, thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 12, 2023

📌 Commit f136e16 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 12, 2023

⌛ Testing commit f136e16 with merge cb05701...

@bors
Copy link
Collaborator

bors commented Sep 12, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing cb05701 to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Sep 12, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing cb05701 to master...

@bors bors merged commit cb05701 into rust-lang:master Sep 12, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new lint: find unneccessary Some(x).and_then(|y| Some(y ..))
4 participants