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

Do not trigger redundant_closure for non-function types #4008

Merged
merged 2 commits into from Apr 25, 2019

Conversation

Projects
None yet
4 participants
@g-bartoszek
Copy link
Contributor

commented Apr 19, 2019

fixes #3898

Added a check for the entity being called in the closure body to be a FnDef. This way lint does not trigger for ADTs (Box) but I'm not sure if it's correct and not too restrictive.

changelog: Fix false positive in redundant_closure pertaining to non-function types

@Manishearth Manishearth changed the title redundant closure for functions restricted to FnDefs Do not trigger redundant_closure for non-function types Apr 19, 2019

@@ -65,6 +65,9 @@ fn check_closure(cx: &LateContext<'_, '_>, expr: &Expr) {
if !(is_adjusted(cx, ex) || args.iter().any(|arg| is_adjusted(cx, arg)));

let fn_ty = cx.tables.expr_ty(caller);

if let ty::FnDef(_, _) = fn_ty.sty;

This comment has been minimized.

Copy link
@Manishearth

Manishearth Apr 19, 2019

Member

This should include FnPtr and Closure.

Use matches! if it works here, otherwise you can move this check below to the then block as a match

@phansch

This comment has been minimized.

Copy link
Collaborator

commented Apr 21, 2019

Does this also fix #1439? (No problem if it doesn't, just making sure)

@g-bartoszek

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

@phansch The lint does not trigger for the original example (#1439 (comment)). It does trigger for the "minimal repro" (#1439 (comment)) but the suggestion is correct in this case (there is no Box involved, just plain closure) so it seems that the answer is yes.

@phansch

This comment has been minimized.

Copy link
Collaborator

commented Apr 25, 2019

@bors r+

thanks!

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

📌 Commit 4f801a2 has been approved by phansch

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

⌛️ Testing commit 4f801a2 with merge 910d538...

bors added a commit that referenced this pull request Apr 25, 2019

Auto merge of #4008 - g-bartoszek:boxed-fnmut, r=phansch
Do not trigger redundant_closure for non-function types

fixes #3898

Added a check for the entity being called in the closure body to be a FnDef. This way lint does not trigger for ADTs (Box) but I'm not sure if it's correct and not too restrictive.

<!--
Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only updates to the latest nightly, you can leave the
`changelog` entry as `none`. Otherwise, please write a short comment
explaining your change.

If your PR fixes an issue, you can add "fixes #issue_number" into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

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

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR -->

changelog: Fix false positive in `redundant_closure` pertaining to non-function types
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: phansch
Pushing 910d538 to master...

@bors bors merged commit 4f801a2 into rust-lang:master Apr 25, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

phansch added a commit to phansch/rust-clippy that referenced this pull request Apr 25, 2019

Add two more tests for redundant_closure
These two cases were fixed by rust-lang#4008.

Closes rust-lang#1439

changelog: none

phansch added a commit to phansch/rust-clippy that referenced this pull request Apr 26, 2019

Add two more tests for redundant_closure
These two cases were fixed by rust-lang#4008.

Closes rust-lang#1439

changelog: none

bors added a commit that referenced this pull request Apr 29, 2019

Auto merge of #4032 - phansch:add_tests, r=phansch
Add two more tests for redundant_closure

These two cases were fixed by #4008.

Closes #1439

changelog: none

bors added a commit that referenced this pull request Apr 29, 2019

Auto merge of #4032 - phansch:add_tests, r=phansch
Add two more tests for redundant_closure

These two cases were fixed by #4008.

Closes #1439

changelog: none

bors added a commit that referenced this pull request Apr 29, 2019

Auto merge of #4032 - phansch:add_tests, r=phansch
Add two more tests for redundant_closure

These two cases were fixed by #4008.

Closes #1439

changelog: none

@g-bartoszek g-bartoszek deleted the g-bartoszek:boxed-fnmut branch May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.