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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

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