allow needless_option_take to report for more cases #13684
Conversation
rustbot
added
the
S-waiting-on-review
ericwu17
force-pushed
the
generalized-needless-option-take
branch
from
a76fdbf to
9a8639e
Compare
|
@Centri3 Are you able to find some time to review this PR or assign another reviewer? Thanks! |
|
I'll be able to look the coming days 👍 |
| expr.span, | ||
| "called `Option::take()` on a temporary value", | ||
| Some(recv.span), | ||
| format!("`{function_name}` creates a temporary value"), |
There was a problem hiding this comment.
nit: I don't see this note being particularly useful. Usually it should explain why this is bad instead
There was a problem hiding this comment.
Thanks, I changed the note to the following:
`{function_name}` creates a temporary value, so calling take() has no effect.
| applicability, | ||
| ); | ||
| if !recv.is_syntactic_place_expr() && is_expr_option(cx, recv) { | ||
| if let Some(function_name) = is_creating_temporary_value(recv) { |
There was a problem hiding this comment.
nit: This function name is rather misleading. I'll suggest source_of_temporary_value
| // Returns the string of the function call that creates the temporary. | ||
| // When this function is called, we are reasonably certain that the ExprKind is either | ||
| // Call or MethodCall because we already checked that the expression is not | ||
| // is_syntactic_place_expr(). |
There was a problem hiding this comment.
nit: Please make this a doc comment
| NEEDLESS_OPTION_TAKE, | ||
| expr.span, | ||
| "called `Option::take()` on a temporary value", | ||
| Some(recv.span), |
There was a problem hiding this comment.
nit: Let's use None to allow the note to be inline.
ericwu17
force-pushed
the
generalized-needless-option-take
branch
3 times, most recently
from
2fb2f5d to
43154ae
Compare
In general, needless_option_take should report whenever take() is called on a temporary value, not just when the temporary value is created by as_ref()
ericwu17
force-pushed
the
generalized-needless-option-take
branch
from
43154ae to
b8cd513
Compare
|
Hi @Centri3, I have taken a look at your comments and made the appropriate changes. Please let me know if you need anything else from me, and thank you for the review! |
3 participants
changelog:
fixes #13671
In general, needless_option_take should report whenever take() is called on a temporary value, not just when the temporary value is created by as_ref().
Also, we stop emitting machine applicable fixes in these cases, since sometimes the intention of the user is to actually clear the original option, in cases such as
option.as_mut().take().