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

Deprecate redundant lint option_map_or_err_ok and take manual_ok_or out of pedantic #14027

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samueltardieu
Copy link
Contributor

While extending the option_map_or_err_ok lint (warn by default, "style") to recognize η-expanded forms of Ok, as in

    // Should suggest `opt.ok_or("foobar")`
   let _ = opt.map_or(Err("foobar"), |x| Ok(x));

I discovered that the manual_ok_or lint (allow by default, "pedantic") already covered exactly the cases handled by option_map_or_err_ok, including the one I was adding. Apparently, option_map_or_err_ok was added without realizing that the lint already existed under the manual_ok_or name. As a matter of fact, artifacts of this second lint were even present in the first lint stderr file and went unnoticed for more than a year.

This PR:

  • deprecates option_map_or_err_ok with a message saying to use manual_ok_or
  • moves manual_ok_or from "pedantic" to "style" (the category in which option_map_or_err_ok was)

In addition, I think that this lint, which is short, machine applicable, and leads to shorter and clearer code with less arguments (Ok disappears) and the removal of one level of call (Err(x) is replaced by x), is a reason by itself to be in "style".

changelog: [option_map_or_err_ok and manual_ok_or]: move manual_ok_or from "pedantic" to "style", and deprecate the redundant style lint option_map_or_err_ok.

@rustbot
Copy link
Collaborator

rustbot commented Jan 18, 2025

r? @dswij

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 18, 2025
`manual_ok_or` covers the same case that were covered by
`option_map_or_err_ok` which is not deprecated. The latter was in the
"style" category. Also, the lint is machine applicable, and leads to
shorter and more readable code, so "style" is appropriate.

The only difference is that the η-expanded form of `Result::Ok()` was
not covered by `option_map_or_err_ok` while it is by `manual_ok_or`, so
the category change may expose some new occurrences.
@samueltardieu
Copy link
Contributor Author

Rebased

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.

3 participants