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

Improve ManuallyDrop suggestion #90901

Merged
merged 2 commits into from
Nov 17, 2021
Merged

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Nov 14, 2021

closes #90585

  • Fixes the recommended change to use ManuallyDrop as per the issue
  • Changes the note to a help
  • improves the span so it only points at the type.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 14, 2021
@rukai
Copy link
Contributor Author

rukai commented Nov 14, 2021

r? @estebank

@rust-highfive rust-highfive assigned estebank and unassigned jackh726 Nov 14, 2021
Comment on lines 374 to 380
let (field_span, ty_span) =
// We are currently checking the type this field came from, so it must be local.
if let Node::Field(field) = tcx.hir().get_if_local(field.did).unwrap() {
(field.span, field.ty.span)
} else {
unreachable!("mir field has to correspond to hir field");
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let (field_span, ty_span) =
// We are currently checking the type this field came from, so it must be local.
if let Node::Field(field) = tcx.hir().get_if_local(field.did).unwrap() {
(field.span, field.ty.span)
} else {
unreachable!("mir field has to correspond to hir field");
};
let (field_span, ty_span) = match tcx.hir().get_if_local(field.did) {
// We are currently checking the type this field came from, so it must be local.
Some(Node::Field(field)) => (field.span, field.ty.span),
_ => unreachable!("mir field has to correspond to hir field"),
};

.span_note(field_span, "`std::mem::ManuallyDrop` can be used to wrap the type")
.multipart_suggestion_verbose(
"wrap the type with `std::mem::ManuallyDrop` and ensure it is manually dropped",
vec![(ty_span, format!("std::mem::ManuallyDrop<{}>", field_ty))],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
vec![(ty_span, format!("std::mem::ManuallyDrop<{}>", field_ty))],
vec![
(ty_span.shrink_to_lo(), "std::mem::ManuallyDrop<".to_string()),
(ty_span.shrink_to_hi(), ">".to_string()),
],

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh thats clever!

@rukai
Copy link
Contributor Author

rukai commented Nov 17, 2021

This is ready for rereview

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 17, 2021

📌 Commit 62acf7f has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 17, 2021
…estebank

Improve ManuallyDrop suggestion

closes rust-lang#90585
* Fixes the recommended change to use ManuallyDrop as per the issue
* Changes the note to a help
* improves the span so it only points at the type.
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 17, 2021
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#89610 (warn on must_use use on async fn's)
 - rust-lang#90667 (Improve diagnostics when a static lifetime is expected)
 - rust-lang#90687 (Permit const panics in stable const contexts in stdlib)
 - rust-lang#90772 (Add Vec::retain_mut)
 - rust-lang#90861 (Print escaped string if char literal has multiple characters, but only one printable character)
 - rust-lang#90884 (Fix span for non-satisfied trivial trait bounds)
 - rust-lang#90900 (Remove workaround for the forward progress handling in LLVM)
 - rust-lang#90901 (Improve ManuallyDrop suggestion)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 469faa2 into rust-lang:master Nov 17, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion for E0740 (union contains a field that needs dropping) doesn't change anything
6 participants