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

Extend unnecessary_unwrap to look for expect in addition to unwrap #7584

Merged
merged 2 commits into from
Sep 4, 2021

Conversation

shepmaster
Copy link
Member

@shepmaster shepmaster commented Aug 18, 2021

changelog: Extend [`unnecessary_unwrap`] to also check for Option::expect and Result::expect. Also give code suggestions in some cases.

Fixes #7581

@rust-highfive
Copy link

r? @camsteffen

(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 Aug 18, 2021
tests/ui/unnecessary_unwrap.stderr Outdated Show resolved Hide resolved
tests/ui/unnecessary_unwrap.stderr Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor

camsteffen commented Aug 18, 2021

You bumped into the stderr length limit. I think you can remove some of your tests. It isn't necessary to add an expect test for every unwrap test.

clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
@camsteffen camsteffen 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 Aug 20, 2021
@shepmaster
Copy link
Member Author

An actual code suggestion would be even better

Any tips on where to look for how to do that?

I haven't found any examples of how to suggest that a line of code be deleted.

if __option__.is_some() {
    let __var__ = __option__.unwrap();
}

Should be suggested to

if let Some(__var__) = __option__ {
}

So I'd need to:

  1. track the target variable name (if there is one)
  2. track if it's an Option or Result
  3. write the if into an if-let, preserving the body
  4. remove the redundant unwrap/expect expression / statement

@shepmaster
Copy link
Member Author

You bumped into the stderr length limit. I think you can remove some of your tests.

Looks like the help caused this to happen again — any preferred tests I should remove this round?

@camsteffen
Copy link
Contributor

camsteffen commented Aug 23, 2021

For the code suggestion, I think it's okay to keep it simple and just make a suggestion to replace the if condition like if let Some(..) = __option__. So that is leaving it to the user to fill in .. and make other code changes. The suggestion would be Applicability::Unspecified. Let me know if you need more help with that.

Looks like the help caused this to happen again — any preferred tests I should remove this round?

I'm seeing if we can increase that that limit (Zulip thread). If not, please split the test into another file.

@camsteffen
Copy link
Contributor

The stderr limit is removed! #7593

@shepmaster
Copy link
Member Author

Ok, I added a commit with a potential suggestion implementation (I was thinking you wanted auto-fixable, which is why I was a bit 😰).

If you like it, I can squash the second and third commits.

clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
tests/ui/checked_unwrap/complex_conditionals.stderr Outdated Show resolved Hide resolved
tests/ui/checked_unwrap/complex_conditionals.stderr Outdated Show resolved Hide resolved
@shepmaster
Copy link
Member Author

All right, pushed up a new version. Let me know if you like it. If so, I'll squash the last two commits.

Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

A couple more things, then go ahead and squash and I will merge.

clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
clippy_lints/src/unwrap.rs Outdated Show resolved Hide resolved
@shepmaster
Copy link
Member Author

Addressed!

@camsteffen
Copy link
Contributor

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2021

📌 Commit b477543 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Sep 4, 2021

⌛ Testing commit b477543 with merge 90e40f8...

bors added a commit that referenced this pull request Sep 4, 2021
Extend unnecessary_unwrap to look for expect in addition to unwrap

changelog:

Extended ``[`unnecessary_unwrap`]`` to also check for `Option::expect` and `Result::expect`.

Fixes #7581
@bors
Copy link
Contributor

bors commented Sep 4, 2021

💔 Test failed - checks-action_test

@camsteffen
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2021

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Sep 4, 2021

📌 Commit b477543 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Sep 4, 2021

⌛ Testing commit b477543 with merge f771927...

@bors
Copy link
Contributor

bors commented Sep 4, 2021

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

@bors bors merged commit f771927 into rust-lang:master Sep 4, 2021
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.

Extend unnecessary_unwrap to look for expect in addition to unwrap
4 participants