Skip to content

drop-checking is more permissive when let statements have an else block #142056

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

Open
dianne opened this issue Jun 5, 2025 · 1 comment
Open
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team

Comments

@dianne
Copy link
Contributor

dianne commented Jun 5, 2025

When pattern bindings are lowered to MIR, they're dropped in reverse order of declaration1. Normally, for each binding, this means running any necessary drop glue, then dropping its storage. However, with let-else, the drop glue is run for all bindings first, then all their storages are dropped. As a result of not interleaving the drops and the StorageDeads, bindings without meaningful drops live slightly longer than bindings with meaningful drops declared at the same time, regardless of declaration order. Example (playground link):

struct Struct<T>(T);
impl<T> Drop for Struct<T> {
    fn drop(&mut self) {}
}

fn main() {
    {
        // This is an error: `short1` is dead before `long1` is dropped.
        let (mut long1, short1) = (Struct(&0), 1);
        long1.0 = &short1;
    }
    {
        // This is OK: `short2`'s storage is live until after `long2`'s drop runs.
        #[expect(irrefutable_let_patterns)]
        let (mut long2, short2) = (Struct(&0), 1) else { unreachable!() };
        long2.0 = &short2;
    }
    {
        // Sanity check: `short3`'s drop is significant; it's dropped before `long3`:
        let tmp = Box::new(0);
        #[expect(irrefutable_let_patterns)]
        let (mut long3, short3) = (Struct(&tmp), Box::new(1)) else { unreachable!() };
        long3.0 = &short3;
    }
}

Implementation-wise, this arises because storage for a let-else's bindings is initialized all at once before match lowering (at which point the StorageDeads are scheduled), but their drops are scheduled afterwards by match lowering. Also of note: the order in which match lowering schedules drops isn't the order in which patterns are normally visited1, so if special treatment is still needed for let-else, care needs to be taken to make sure drop order doesn't change.

Zulip stream with details on implementation history: https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/dropck.20inconsistency.20between.20.60let.60.20and.20.60let.60-.60else.60/with/522465186

Related: #142057

cc @rust-lang/lang since fixing this in any direction would change what programs are valid. I imagine that needs a T-lang decision?
cc @dingxiangfei2009, though I'd be happy to try implementing a fix myself once a decision is made on what the proper behavior should be.

@rustbot label: +T-compiler +T-lang +A-MIR

Footnotes

  1. Match lowering treats bindings in or-patterns as occurring after bindings outside of or-patterns and differs between let and match (example). Additionally, it treats bindings in a var @ subpattern's subpattern as occurring before the var (Unnecessary 'cannot bind by-move with sub-bindings' with bindings_after_at #69971). 2

@dianne dianne added the C-bug Category: This is a bug. label Jun 5, 2025
@rustbot rustbot added needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team labels Jun 5, 2025
@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 8, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 9, 2025
…e-tests, r=Nadrieril

add tests for pattern binding drop order edge cases

This adds tests for rust-lang#142163, rust-lang#142057, and rust-lang#142056. I'm using these tests to help make sure I don't commit breaking changes when implementing match lowering for guard patterns, but I think it makes sense to add them separately. They don't directly have anything to do with guard patterns.

r? `@Nadrieril` or reassign
rust-timer added a commit that referenced this issue Jun 9, 2025
Rollup merge of #142193 - dianne:binding-drop-order-edge-case-tests, r=Nadrieril

add tests for pattern binding drop order edge cases

This adds tests for #142163, #142057, and #142056. I'm using these tests to help make sure I don't commit breaking changes when implementing match lowering for guard patterns, but I think it makes sense to add them separately. They don't directly have anything to do with guard patterns.

r? `@Nadrieril` or reassign
@traviscross traviscross added I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels Jun 14, 2025
@traviscross
Copy link
Contributor

Great find. Thanks @dianne. We'll discuss. For my part, I would indeed expect, setting aside the behavior of temporaries, that these three would be equivalent:

{
    let (mut long, short) = (W(&A), A) else { unreachable!() };
    long.0 = &short; //~ OK?
}
{
    if let (mut long, short) = (W(&A), A) {
        long.0 = &short; //~ ERROR does not live long enough
    } else {
        unreachable!()
    }
}
{
    match (W(&A), A) {
        (mut long, short) => long.0 = &short, //~ ERROR does not live long enough
        _ => unreachable!(),
    }
}

Playground link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team
Projects
None yet
Development

No branches or pull requests

4 participants