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

needless_option_take: add autofix #14042

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

samueltardieu
Copy link
Contributor

changelog: [needless_option_take]: add autofix

@rustbot
Copy link
Collaborator

rustbot commented Jan 20, 2025

r? @llogiq

rustbot has assigned @llogiq.
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 20, 2025
@llogiq
Copy link
Contributor

llogiq commented Jan 25, 2025

The implementation looks good to me, but I'm not sure if I agree with the idea of an autofix here. The problem is that the .take() shows a probable mismatch between the intent of the programmer and the actions of the code, so it is very likely that the code is wrong in other ways that we shouldn't silently "fix".

@samueltardieu
Copy link
Contributor Author

samueltardieu commented Jan 25, 2025

Your remark opens up to questions for me:

  • If you think this signals a programmer's mistake, shouldn't this lint be into suspicious rather than in complexity?
  • Should we ever apply autofixes for suspicious lints, or should we require another flag?

In the present case, I'm not sure that someone calling .take() on an option is likely to be a mistake. It might be a remnant of a refactoring, or the programmer not realizing that they own the option (compared to having a &mut) and can use it directly.

Note that if they had used swap, I would have more inclined to see that as a probable error.

@llogiq
Copy link
Contributor

llogiq commented Jan 28, 2025

To your first question, you're probably right and we may as well autofix it. To the second question, I would still decide on a lint-by-lint basis, but opt into autofixes rather than opting out (by which I mean there should be good arguments for a fix, otherwise I'd default to not fix).

Thanks for doing this improvement.

@llogiq llogiq added this pull request to the merge queue Jan 28, 2025
Merged via the queue into rust-lang:master with commit 9ede32f Jan 28, 2025
11 checks passed
@samueltardieu samueltardieu deleted the push-lyqzlrxsmvuv branch January 28, 2025 19:04
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