Change desugaring of let-else to ensure temporary is dropped earlier #94012

cormacrelf · 2022-02-15T06:02:08Z

Fixes #93951. Ref #87335. Now uses DropTemps per Cam's suggestion, which is equivalent to what I was doing earlier but obviously much better and now only a 3-line change.

This new desugaring introduces a named temporary to hold the result of evaluating the generated if-let. The named temporary surrounding the whole if-let allows borrowck to see that the unnamed temporary that holds foo(&x) is dropped before x is. I think in rustc terminology you can say there is now a destruction scope between foo(&x) and the } where x is dropped.

An alternative would be to introduce even finer-grained scopes in implicit return expressions. But that seems a much bigger change, and much more difficult for me. Or, of course, go back to the original strategy of extracting the pattern's bindings via a tuple.

From here:

it's possible there's a solution without reintroducing that complexity. If you make the desugaring let retval = if let Ok(s) = foo(&x) { /* trailing stmts */ } else { return; }; retval then you no longer have a temporary that borrowck worries will be dropped after x: playground. That is, if we can't think of any other borrowck issues.

Originally posted by me in #93628 (comment)

 fn test() {
     let x = String::from("Hey");
     // input
     let Ok(s) = foo(&x) else { return };
     return;

     // desugaring
-    if let Ok(s) = foo(&x) { return; } else { return }
+    DropTemps(if let Ok(s) = foo(&x) { return; } else { return })
 }

r? @lcnr

cormacrelf · 2022-02-15T06:25:57Z

Ah, well that puts a stopper in that idea. If the trailing statements are divergent, then the ; val is unreachable. (And you get a warning on the use of val, but rustc's codebase denies warnings.)

// orig
let Some(x) = blah() else {
    return;
};
loop {}

// desugar
let val = if let Some(x) = blah() {
    loop {}
} else {
    return;
};
val // unreachable

cormacrelf · 2022-02-15T07:07:45Z

Unless it's feasible to programmatically add an #[allow(unreachable_code)] attribute on the val return expression? Edit: apparently the lower_expr_try implementation does this. Seems legit.

This prevents *any* unreachable_code warnings on DropTemps, including from its other uses.

cormacrelf · 2022-02-16T08:43:42Z

Thanks to @camsteffen this now uses DropTemps as it should.

lcnr

the fix looks good, but I still don't understand why we actually first drop locals and then temporaries in the first place. Would like to figure this out before merging this pr. currently looking into this.

src/test/ui/let-else/let-else-then-diverge.rs

src/test/ui/let-else/let-else-temp-borrowck.rs

+ remove actual unused vars from some other tests to be less distracting

cormacrelf · 2022-02-16T14:07:18Z

👍 Let us know what you find out, it is a bit of a mystery to me too.

camsteffen · 2022-02-16T15:34:12Z

Got another idea. Perhaps we can lower to StmtKind::Semi rather than StmtKind::Expr and then the DropTemps won't be needed?

cormacrelf · 2022-02-16T15:55:16Z

The generated if-let is a tail position expression that needs to function as an implicit return value. It's got to be an Expr.

camsteffen · 2022-02-16T16:01:22Z

The generated if-let is a tail position expression that needs to function as an implicit return value. It's got to be an Expr.

Why? A let-else statement is even written as a statement with a semicolon.

cormacrelf · 2022-02-16T16:08:06Z

Look for yourself with -Zunpretty=hir,typed. In brief:

{
    let Ok(s) = expr else {
        return 0;
    };
    // many statements
    5
} // end of block

// desugars into
{
    if let Ok(s) = expr {
        // many statements
        5
    } else {
        return 0;
    }
} // end of block

The result is an implicit return expression of type integer.

camsteffen · 2022-02-16T16:37:24Z

Oh yeah 🤦 don't mind me!

lcnr · 2022-02-21T09:50:19Z

here's an example that would break if we were to first drop locals and then temporaries

struct DropsA<'a>(&'a str);
impl DropsA<'_> {
    fn as_unit(&self) {}
}
impl Drop for DropsA<'_> {
    fn drop(&mut self) {}
}

fn main() {
    let mut slot = None;
    drop(slot.replace(DropsA(&String::from("Hey"))))
}

zulip thread

after #94012 (comment) r=me

lcnr · 2022-02-21T10:57:40Z

@bors r+

bors · 2022-02-21T10:57:42Z

📌 Commit d549ede has been approved by lcnr

lcnr · 2022-02-21T10:58:07Z

@bors rollup=never

probably fine, but considering how frequently let-else is used in the compiler, this may actually impact perf

cormacrelf · 2022-02-21T11:05:09Z

Thanks @lcnr, that makes sense. Also, whoa, I did not know we were up to 381 uses in compiler/!

bors · 2022-02-22T18:16:00Z

☔ The latest upstream changes (presumably #94254) made this pull request unmergeable. Please resolve the merge conflicts.

lcnr · 2022-02-25T09:39:46Z

compiler/rustc_ast_lowering/src/block.rs

@@ -159,6 +159,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
            span,
            kind: hir::ExprKind::If(let_expr, then_expr, Some(else_expr)),
        });
+        let drop_temps = self.expr_drop_temps(span, if_expr, AttrVec::new());


to be consistent with let we would have to actually drop temps for the let_expr or init_expr, wouldn't we?

Yeah, gut feeling this would fix the new problem. Good idea.

I imagine the most straightforward solution will drop temps before the else block instead of after it. Maybe that's okay, but just want to make sure that detail is considered.

My proposed fix does not fix the general discrepancy between let and let-else, e.g https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=57e814fa57019bc62930bc9d61c894da

after the last lang meeting either @nikomatsakis or @pnkfelix should take over the review here. i am not familar enough with rusts temporary rules

Should a new issue be filed? Or work it within this issue

bors · 2022-03-05T10:39:14Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout let-else-temp (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self let-else-temp --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

CONFLICT (add/add): Merge conflict in src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.rs
CONFLICT (content): Merge conflict in src/test/ui/let-else/let-else-allow-unused.rs
Auto-merging compiler/rustc_typeck/src/check/expr.rs
Auto-merging compiler/rustc_ast_lowering/src/block.rs
Automatic merge failed; fix conflicts and then commit the result.

bors · 2022-03-14T11:04:32Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout let-else-temp (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self let-else-temp --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

CONFLICT (add/add): Merge conflict in src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.rs
CONFLICT (content): Merge conflict in src/test/ui/let-else/let-else-allow-unused.rs
Auto-merging compiler/rustc_typeck/src/check/expr.rs
Auto-merging compiler/rustc_ast_lowering/src/block.rs
Automatic merge failed; fix conflicts and then commit the result.

pnkfelix · 2022-03-15T16:12:35Z

@rustbot claim

I'm taking over review here for now.

bors · 2022-03-26T14:45:48Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout let-else-temp (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self let-else-temp --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

CONFLICT (add/add): Merge conflict in src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.rs
CONFLICT (content): Merge conflict in src/test/ui/let-else/let-else-allow-unused.rs
Auto-merging compiler/rustc_typeck/src/check/expr.rs
Auto-merging compiler/rustc_ast_lowering/src/block.rs
Automatic merge failed; fix conflicts and then commit the result.

JohnCSimon · 2022-05-07T23:41:53Z

Ping from triage:
@cormacrelf - can you please address the conflicts and build failures?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

pnkfelix · 2022-05-09T14:23:42Z

@cormacrelf I am currently planning to look into approaches for fixing this more generally with @dingxiangfei2009 . Would you like to try to collaborate with us as well on that? Do you think we should close this PR, or try to adapt it?

nikomatsakis · 2022-05-12T14:30:23Z

Cross-posting:

@dingxiangfei2009, @pnkfelix, and I got together to discuss alternatives around adjusting the desugaring. We concluded that there is no way to represent the desired temporary lifetimes if we desugar let-else in the existing HIR, and that we ought to extend the HIR with a specific let-else construct. Here are our notes:

https://hackmd.io/@nikomatsakis/r1CpZqc8c

Personally, I think we desugar too much at the HIR level and make our lives harder than they have to be (e.g., I don't think we should desugar for either until MIR construction), but @pnkfelix felt a bit sad about the idea of adding let-else. =) Anyway, if people have other ideas for how to manage the desugaring, we're all ears!

In the meantime, @dingxiangfei2009 is going to start exploring what it would like to extend HIR with let-else as a native construct -- we will see how cleanly it works out.

bors · 2022-05-16T04:54:18Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout let-else-temp (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self let-else-temp --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

CONFLICT (add/add): Merge conflict in src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.stderr
Auto-merging src/test/ui/let-else/let-else-allow-unused.rs
CONFLICT (content): Merge conflict in src/test/ui/let-else/let-else-allow-unused.rs
Auto-merging compiler/rustc_typeck/src/check/expr.rs
Auto-merging compiler/rustc_ast_lowering/src/block.rs
Automatic merge failed; fix conflicts and then commit the result.

cormacrelf · 2022-05-17T17:23:29Z

Thanks for looping me in. Yeah, I didn't push for the DropTemps, not being all that confident of its prospects after the case @lcnr found in that last review comment. Should have cleared it up, I will take one last look and close. The notes are good, I think the chosen course seems about right.

I'll be happy to lend a hand, tag me if you want me to take a look at anything as I'm reasonably familiar with this code (although I'll be no use with THIR). From my experience with it, tweaking HIR is a bit of a slog getting it to compile all the way through, so commit early & regularly go outside to look at a dog or a cloud or something, is my free advice if you want it @dingxiangfei2009.

JohnCSimon · 2022-06-18T23:04:02Z

ping from triage:
@cormacrelf
Can you please address the merge conflict?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

camsteffen · 2022-06-29T14:07:05Z

Fixes #98672

(Just for the record. I think this may be closed in favor of a different approach?)

est31 · 2022-07-15T17:25:28Z

Given that #98574 has been merged, I guess this can be closed now?

Note that I have taken one of the commits of this PR and included it in my PR to add additional tests for the let-else feature: ~~#99291~~ #100443

…=Mark-Simulacrum Add two let else regression tests Adds a regression test for rust-lang#94176, as it was fixed by rust-lang#98574 but doesn't have a regression test. The PR also incorporates a commit from rust-lang#94012 which added a test for an issue discovered in that PR. Originally they have been part of rust-lang#99291, but I've moved them out in the hopes of getting them merged more quickly, as that PR is already open since a month, and so that rust-lang#99291 can focus on the drop order part of things. Closes rust-lang#94176 Closes rust-lang#96961 -- dupe of rust-lang#94176

rust-highfive assigned lcnr Feb 15, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 15, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 15, 2022

This comment has been minimized.

Sign in to view

cormacrelf closed this Feb 15, 2022

let-else: test for issue 93951 / temporary lifetimes

638d9c3

cormacrelf reopened this Feb 15, 2022

cormacrelf force-pushed the let-else-temp branch from 8ba5ee9 to 29d3914 Compare February 15, 2022 09:12

est31 mentioned this pull request Feb 15, 2022

Stabilize let else #93628

Merged

let-else: add a test for warnings on let-else with diverging tail

14283b2

cormacrelf mentioned this pull request Feb 16, 2022

let_else returns in surprising borrowck errors with impl trait #93951

Closed

let-else: use DropTemps to fix borrowck complaints

07539b8

This prevents *any* unreachable_code warnings on DropTemps, including from its other uses.

cormacrelf force-pushed the let-else-temp branch from 29d3914 to 07539b8 Compare February 16, 2022 08:42

lcnr approved these changes Feb 16, 2022

View reviewed changes

src/test/ui/let-else/let-else-then-diverge.rs Outdated Show resolved Hide resolved

src/test/ui/let-else/let-else-temp-borrowck.rs Outdated Show resolved Hide resolved

let-else: add test for linting unused variables in the first place

d549ede

+ remove actual unused vars from some other tests to be less distracting

est31 mentioned this pull request Feb 20, 2022

Missing trailing expression in block with let else yields incorrectly spanned error message #94176

Closed

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 Feb 21, 2022

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2022

lcnr reviewed Feb 25, 2022

View reviewed changes

lcnr mentioned this pull request Feb 28, 2022

let else doesn't eagerly drop with _ pattern #94351

Closed

rustbot assigned pnkfelix and unassigned lcnr Mar 15, 2022

est31 mentioned this pull request Jul 15, 2022

Add let else drop order tests #99291

Merged

est31 mentioned this pull request Aug 12, 2022

Add two let else regression tests #100443

Merged

Dylan-DPC closed this Aug 12, 2022

Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change desugaring of let-else to ensure temporary is dropped earlier #94012

Change desugaring of let-else to ensure temporary is dropped earlier #94012

cormacrelf commented Feb 15, 2022 •

edited

Loading

This comment has been minimized.

cormacrelf commented Feb 15, 2022 •

edited

Loading

cormacrelf commented Feb 15, 2022 •

edited

Loading

cormacrelf commented Feb 16, 2022

lcnr left a comment

cormacrelf commented Feb 16, 2022

camsteffen commented Feb 16, 2022

cormacrelf commented Feb 16, 2022

camsteffen commented Feb 16, 2022

cormacrelf commented Feb 16, 2022

camsteffen commented Feb 16, 2022

lcnr commented Feb 21, 2022

lcnr commented Feb 21, 2022

bors commented Feb 21, 2022

lcnr commented Feb 21, 2022

cormacrelf commented Feb 21, 2022

bors commented Feb 22, 2022

lcnr Feb 25, 2022

cormacrelf Feb 25, 2022

camsteffen Feb 28, 2022

lcnr Mar 1, 2022

ericsampson Apr 17, 2022

bors commented Mar 5, 2022

bors commented Mar 14, 2022

pnkfelix commented Mar 15, 2022

bors commented Mar 26, 2022

JohnCSimon commented May 7, 2022

pnkfelix commented May 9, 2022 •

edited

Loading

nikomatsakis commented May 12, 2022

bors commented May 16, 2022

cormacrelf commented May 17, 2022 •

edited

Loading

JohnCSimon commented Jun 18, 2022

camsteffen commented Jun 29, 2022

est31 commented Jul 15, 2022 •

edited

Loading

Change desugaring of let-else to ensure temporary is dropped earlier #94012

Change desugaring of let-else to ensure temporary is dropped earlier #94012

Conversation

cormacrelf commented Feb 15, 2022 • edited Loading

This comment has been minimized.

cormacrelf commented Feb 15, 2022 • edited Loading

cormacrelf commented Feb 15, 2022 • edited Loading

cormacrelf commented Feb 16, 2022

lcnr left a comment

Choose a reason for hiding this comment

cormacrelf commented Feb 16, 2022

camsteffen commented Feb 16, 2022

cormacrelf commented Feb 16, 2022

camsteffen commented Feb 16, 2022

cormacrelf commented Feb 16, 2022

camsteffen commented Feb 16, 2022

lcnr commented Feb 21, 2022

lcnr commented Feb 21, 2022

bors commented Feb 21, 2022

lcnr commented Feb 21, 2022

cormacrelf commented Feb 21, 2022

bors commented Feb 22, 2022

lcnr Feb 25, 2022

Choose a reason for hiding this comment

cormacrelf Feb 25, 2022

Choose a reason for hiding this comment

camsteffen Feb 28, 2022

Choose a reason for hiding this comment

lcnr Mar 1, 2022

Choose a reason for hiding this comment

ericsampson Apr 17, 2022

Choose a reason for hiding this comment

bors commented Mar 5, 2022

bors commented Mar 14, 2022

pnkfelix commented Mar 15, 2022

bors commented Mar 26, 2022

JohnCSimon commented May 7, 2022

pnkfelix commented May 9, 2022 • edited Loading

nikomatsakis commented May 12, 2022

bors commented May 16, 2022

cormacrelf commented May 17, 2022 • edited Loading

JohnCSimon commented Jun 18, 2022

camsteffen commented Jun 29, 2022

est31 commented Jul 15, 2022 • edited Loading

cormacrelf commented Feb 15, 2022 •

edited

Loading

cormacrelf commented Feb 15, 2022 •

edited

Loading

cormacrelf commented Feb 15, 2022 •

edited

Loading

pnkfelix commented May 9, 2022 •

edited

Loading

cormacrelf commented May 17, 2022 •

edited

Loading

est31 commented Jul 15, 2022 •

edited

Loading