Suggest using std::mem::drop function instead of explicit destructor call #72383
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
stanislav-tkach
force-pushed
the
issue-72322
branch
2 times, most recently
from
488f62b
to
23b34b6
Compare
src/librustc_typeck/check/callee.rs
Outdated
| let (suggestion, applicability) = if snippet.is_empty() { | ||
| (snippet, Applicability::Unspecified) | ||
| } else { | ||
| (format!("drop({})", snippet), Applicability::MachineApplicable) |
There was a problem hiding this comment.
I'm not sure if Applicability::MachineApplicable should be used here. The suggestion is valid, but I'm not aware how such applications work. As far as I can see, the suggestion is emitted for the Drop::drop span, not the full Drop::drop(x) expression.
There was a problem hiding this comment.
This should probably be MaybeIncorrect since x could be a mutable reference and then drop(x) is probably not what the user wants.
stanislav-tkach
force-pushed
the
issue-72322
branch
from
23b34b6
to
d2e8cbc
Compare
stanislav-tkach
force-pushed
the
issue-72322
branch
from
d2e8cbc
to
34b5118
Compare
src/librustc_typeck/check/callee.rs
Outdated
| (format!("drop({})", snippet), Applicability::MachineApplicable) | ||
| }; | ||
|
|
||
| err.span_suggestion(span, "consider using `drop` function", suggestion, applicability); |
There was a problem hiding this comment.
If you're suggesting drop(x) then the span for the suggestion will need to be the one for the whole method call.
There was a problem hiding this comment.
I have tried to merge spans, but I'm suppose it still won't do because the resulting span doesn't include round brackets: x.drop instead of x.drop(). Is there an easy way to achieve that?
There was a problem hiding this comment.
Perhaps it isn't worth the fuss and because applicability still would be MaybeIncorrect at best, I can simply print a message leaving the suggestion empty. Something like that:
let message = format!("consider using `drop` function{}", snippet);
err.span_suggestion(
span,
&message,
String::new(),
Applicability::Unspecified,
);What do you think?
There was a problem hiding this comment.
@matthewjasper Sorry for bothering you, but I'm not sure if I should try to extend the span somehow to include the whole method call or use a simpler solution?
There was a problem hiding this comment.
I have updated the pull request accordingly.
src/librustc_typeck/check/callee.rs
Outdated
| let (suggestion, applicability) = if snippet.is_empty() { | ||
| (snippet, Applicability::Unspecified) | ||
| } else { | ||
| (format!("drop({})", snippet), Applicability::MachineApplicable) |
There was a problem hiding this comment.
This should probably be MaybeIncorrect since x could be a mutable reference and then drop(x) is probably not what the user wants.
src/librustc_typeck/check/callee.rs
Outdated
| if snippet.is_empty() { "drop".to_string() } else { format!("drop({})", snippet) }; | ||
|
|
||
| let suggestion_span = | ||
| receiver.and_then(|s| tcx.sess.source_map().merge_spans(s, span)).unwrap_or(span); |
There was a problem hiding this comment.
This should use the whole call expr span if this is a method, so that the span include the parentheses of the method call.
|
r=me once comment is addressed |
|
@bors r+ |
|
📌 Commit 6ddbef1 has been approved by |
bors
added
S-waiting-on-bors
Suggest using std::mem::drop function instead of explicit destructor call I would prefer to give a better suggestion that includes code example, but I'm currently stuck on getting the correct span for that. Closes rust-lang#72322.
Rollup of 9 pull requests Successful merges: - rust-lang#72310 (Add Peekable::next_if) - rust-lang#72383 (Suggest using std::mem::drop function instead of explicit destructor call) - rust-lang#72398 (SocketAddr and friends now correctly pad its content) - rust-lang#72465 (Warn about unused captured variables) - rust-lang#72568 (Implement total_cmp for f32, f64) - rust-lang#72572 (Add some regression tests) - rust-lang#72591 (librustc_middle: Rename upvar_list to closure_captures) - rust-lang#72701 (Fix grammar in liballoc raw_vec) - rust-lang#72731 (Add missing empty line in E0619 explanation) Failed merges: r? @ghost
I would prefer to give a better suggestion that includes code example, but I'm currently stuck on getting the correct span for that.
Closes #72322.