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

Add unnecessary_literal_unwrap lint #10358

Merged
merged 16 commits into from
Jun 12, 2023
Merged

Add unnecessary_literal_unwrap lint #10358

merged 16 commits into from
Jun 12, 2023

Conversation

pksunkara
Copy link
Contributor

@pksunkara pksunkara commented Feb 16, 2023

Add lint for more unnecessary unwraps and suggest fixes for them.

Fixes #10352

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

r? @llogiq


changelog: New lint [unnecessary_literal_unwrap]
#10358

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2023

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

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2023
@pksunkara pksunkara changed the title Add test code Add unnecessary_literal_unwrap lint Feb 16, 2023
@llogiq
Copy link
Contributor

llogiq commented Feb 16, 2023

The unwrap_or_else_default UI test is still failing.

@llogiq
Copy link
Contributor

llogiq commented Feb 16, 2023

@rustbot author

@rustbot rustbot 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 Feb 16, 2023
@samueltardieu
Copy link
Contributor

The unwrap_or_else_default UI test is still failing.

As well as many others. I count differences in 21 output files when testing locally.

@pksunkara
Copy link
Contributor Author

Only the documentation is left. But before I do that, I would like to know 2 things:

  1. How do I make this lint warn by default?
  2. What should I put in place of #[clippy::version = "1.45.0"] in declare_lint! usage?

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
tests/ui/unnecessary_literal_unwrap.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Mar 27, 2023

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

@llogiq
Copy link
Contributor

llogiq commented May 23, 2023

@pksunkara do you intend to revisit this at some point?

@pksunkara
Copy link
Contributor Author

Yup, I can take a look on Monday.

@pksunkara
Copy link
Contributor Author

I would appreciate if I can get some answers for the questions I asked earlier, #10358 (comment)

@llogiq
Copy link
Contributor

llogiq commented May 28, 2023

  1. How do I make this lint warn by default?
  2. What should I put in place of #[clippy::version = "1.45.0"] in declare_lint! usage?
  1. Do you mean in a test? That would be #[warn(clippy::unnecessary_literal_unwrap)]. Otherwise the default lint level is given by the lint group. As this lint is in complexity, that should IIRC warn by default.
  2. #[clippy::version = "1.69.0"].

@pksunkara
Copy link
Contributor Author

pksunkara commented Jun 7, 2023

I have addressed the review comments.

@rustbot reviewer

@rustbot
Copy link
Collaborator

rustbot commented Jun 7, 2023

Error: Parsing assign command in comment failed: ...'' | error: specify user to assign to at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jun 7, 2023
@blyxyas
Copy link
Member

blyxyas commented Jun 10, 2023

@pksunkara The comment to remove the L-waiting-on-author label is rustbot ready

@llogiq
Copy link
Contributor

llogiq commented Jun 10, 2023

There are still merge conflicts, and I don't have time to review right now, sorry. Perhaps I'll find a few minutes tomorrow.

@pksunkara
Copy link
Contributor Author

There are still merge conflicts

They are very minor ones because of this PR touching a lot of tests and I will fix them once the code is reviewed.

Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

Looks good, I only have a small nit regarding docs.

Also I'm not completely sure about the name: So far we have 23 needless-* and 18 unnecessary-* lints. I like the former more for both brevity and being easier to write.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@pksunkara
Copy link
Contributor Author

Addressed the conflicts and review comment.

@bors
Copy link
Collaborator

bors commented Jun 12, 2023

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

@llogiq
Copy link
Contributor

llogiq commented Jun 12, 2023

So you intend to keep the name longer. Ah, well, you've done enough rebases as is. Let's roll with it. Thanks for seeing this PR through.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 12, 2023

📌 Commit bfd5aba has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 12, 2023

⌛ Testing commit bfd5aba with merge 7c2bf28...

@bors
Copy link
Collaborator

bors commented Jun 12, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 7c2bf28 to master...

@bors bors merged commit 7c2bf28 into rust-lang:master Jun 12, 2023
@pksunkara pksunkara deleted the unnecessary-unwrap branch June 12, 2023 18:24
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.

Unnecessary direct unwraps on Option and Result are not warned/fixed
7 participants