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.

Sign up for GitHub

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

Jump to bottom

Add unneeded_try_convert lint #4759

Closed
wants to merge 1 commit into from
Closed

Add unneeded_try_convert lint #4759

wants to merge 1 commit into from

Conversation

HMPerson1
Copy link
Contributor

changelog: New lint: unneeded_try_convert checks for things like option.unwrap_or_else(|| <..>::from(..))? as the from is unneeded becase ? will do it for you.

Closes #4676

Some notes:

  • There are a few test cases I've commented out because the suggested fix doesn't compile.
  • clippy::redundant_closure catches result.map_err(|x| String::from(x))? and suggests result.map_err(String::from)?, which this then catches and suggests result?, but this also catches result.map_err(|x| String::from(x))? but instead suggests result.map_err(|x| x)?. I'm not sure whether we should add a special case specifically for that in this lint or maybe have another lint for useless map or if it's fine to just leave that as-is.

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 31, 2019
@bors
Copy link
Contributor

bors commented Nov 7, 2019

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

@bors
Copy link
Contributor

bors commented Nov 7, 2019

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

@bors
Copy link
Contributor

bors commented Nov 11, 2019

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

@bors
Copy link
Contributor

bors commented Nov 23, 2019

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

@flip1995 flip1995 added the A-lint Area: New lints label Nov 25, 2019
@phansch phansch self-requested a review April 15, 2020 19:41
phansch
phansch reviewed Apr 16, 2020
View reviewed changes
Copy link
Member

@phansch phansch left a comment

Choose a reason for hiding this comment

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

Apologies for having to wait so long for a review. I think this generally looks very good and I just found some small things to change.

clippy_lints/src/unneeded_try_convert.rs
if let ExprKind::MethodCall(call_path, _, call_args) = &expr.kind;
if call_path.ident.as_str() == "ok_or_else";
if let [receiver, closure_expr] = &**call_args;
if match_type(cx, cx.tables.expr_ty(receiver), &paths::OPTION);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if match_type(cx, cx.tables.expr_ty(receiver), &paths::OPTION);
if is_type_diagnostic_item(cx, cx.tables.expr_ty(receiver), sym!(option_type));

clippy_lints/src/unneeded_try_convert.rs
if call_path.ident.as_str() == "map_err";
if let [receiver, mapper_expr] = &**call_args;
let receiver_ty = cx.tables.expr_ty(receiver);
if match_type(cx, receiver_ty, &paths::RESULT);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if match_type(cx, receiver_ty, &paths::RESULT);
if is_type_diagnostic_item(cx, receiver_ty, sym!(result_type));

tests/ui/unneeded_try_convert.rs
let option = Some(3);
option.ok_or_else(|| String::from("foo"))?;
option.ok_or_else(|| String::from(complex_computation()))?;
// type arg not fixed
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a leftover todo?

@phansch phansch 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 Apr 16, 2020
@flip1995
Copy link
Member

Thanks for contributing to Clippy! Sadly this PR was not updated in quite some time. If you waited on input from a reviewer, we're sorry that this fell under the radar. If you want to continue to work on this, just reopen the PR and/or ping a reviewer.

@flip1995 flip1995 closed this May 25, 2020
@flip1995 flip1995 added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansch phansch phansch left review comments

Assignees
No one assigned
Labels
A-lint Area: New lints S-inactive-closed Status: Closed due to inactivity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lint simplifyable ok_or_else usage
4 participants
@HMPerson1 @bors @flip1995 @phansch