Skip to content

Fix: avoid changing drop order #11603

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

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Fix: avoid changing drop order #11603

merged 2 commits into from
Oct 3, 2023

Conversation

koka831
Copy link
Contributor

@koka831 koka831 commented Oct 3, 2023

Fixes #11599

changelog: [redundant_locals] No longer lints which implements Drop trait to avoid reordering

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 3, 2023
// the local does not impl Drop trait. see #11599
let local_ty = cx.typeck_results().node_type(local.hir_id);
if let Some(drop_trait_id) = cx.tcx.lang_items().drop_trait();
if !cx.tcx.infer_ctxt().build().type_implements_trait(
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it won't work if the type is composed of other types implementing Drop.

struct WithDrop(usize);
impl Drop for WithDrop {..}

struct V(WithDrop);

let first = V(WithDrop(1));
let second = WithDrop(2);
let first = first;

clippy_utils::ty::needs_ordered_drop would be better.

But also, the lint already has a bit of logic for checking if it affects drop behavior further down and it already checks if the type has a significant dtor that affects drop ordering:

/// Check if a rebinding of a local affects the code's drop behavior.
fn affects_drop_behavior<'tcx>(cx: &LateContext<'tcx>, bind: HirId, rebind: HirId, rebind_expr: &Expr<'tcx>) -> bool {
let hir = cx.tcx.hir();
// the rebinding is in a different scope than the original binding
// and the type of the binding cares about drop order
hir.get_enclosing_scope(bind) != hir.get_enclosing_scope(rebind)
&& needs_ordered_drop(cx, cx.typeck_results().expr_ty(rebind_expr))
}

We should probably just remove the get_enclosing_scope(bind) != get_enclosing_scope(rebind) check in there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I couldn't figure out why affects_drop_behavior wasn't working. 😅

We should probably just remove the get_enclosing_scope(bind) != get_enclosing_scope(rebind) check from there.

Yes, it works perfectly.

@koka831
Copy link
Contributor Author

koka831 commented Oct 3, 2023

I've addressed them. could you r? @y21

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2023

Failed to set assignee to y21: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@Alexendoo
Copy link
Member

Thanks! @bors r=y21

@bors
Copy link
Contributor

bors commented Oct 3, 2023

📌 Commit c715267 has been approved by y21

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 3, 2023

⌛ Testing commit c715267 with merge b437069...

@bors
Copy link
Contributor

bors commented Oct 3, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: y21
Pushing b437069 to master...

@bors bors merged commit b437069 into rust-lang:master Oct 3, 2023
@koka831 koka831 deleted the fix/11599 branch October 4, 2023 01:44
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.

Incorrect redundant_locals when rebinding to change drop order
6 participants