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

Do for loops still need to be a terminating scope? #60253

Closed
Centril opened this issue Apr 25, 2019 · 4 comments · Fixed by #63432
Closed

Do for loops still need to be a terminating scope? #60253

Centril opened this issue Apr 25, 2019 · 4 comments · Fixed by #63432
Assignees
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Apr 25, 2019

See: #60225

@pnkfelix Can you comment on whether this construct is actually relevant today? I haven't been able to encode a program where the difference can be observed with and without the construct when using the desugaring of for-loops. If it isn't relevant, then let's perhaps make an issue and remove it in a follow up PR.

@Centril Centril added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 25, 2019
@pnkfelix
Copy link
Member

pnkfelix commented May 2, 2019

for future reference: @Centril is referencing my PR #21984 which changed the r-value temporary rules with respect to the temporaries in the ITER_EXPR of for PAT in ITER_EXPR { ... }

@petrochenkov
Copy link
Contributor

Even beside the { let _result = ...; _result } part the for loop desugaring is so complex.
It would be nice to document what every employed trick changes compared to the naive desugaring.

@pnkfelix
Copy link
Member

pnkfelix commented May 2, 2019

PR #21984 itself was motivated (or at least spawned off of) PR #21972. I'm actually trying to find out what the actual motivation for the described change was.

The change, as described, was that the temporaries within ITER_EXPR would previously live until the end of the enclosing block.

So if you consider the following code (play):

#![allow(unused_variables, unused_mut)]

#[derive(Debug)]
struct D(&'static str);

impl Drop for D {
    fn drop(&mut self) {
        println!("dropping D(\"{}\")", self.0);
    }
}

fn main() {
    let mut foo: Vec<&D> = Vec::new();
    {
        let block_temp = D("a block temp");
        println!("start of enclosing block");
        for (j, i) in (&vec![D("1"), D("2"), D("3")]).into_iter().enumerate() {
            println!("iter: {}", j);
            foo.push(i);
        }
        println!("end of enclosing block");
    }
}

which, when it is accepted by the compiler (as written it requires NLL), it should print this:

start of enclosing block
iter: 0
iter: 1
iter: 2
dropping D("1")
dropping D("2")
dropping D("3")
end of enclosing block
dropping D("a block temp")

notably, we drop all of {D("1"), D("2"), D("3")} immediately after the for-statement, but before the println! at the end of the block.

The breakage I would be concerned about, if we made for-loops no longer a terminating scope for their ITER_EXPR, is that all of the temps would live until the end of the block, and thus the drops of {D("1"), D("2"), D("3")} would happen after the "end of enclosing block", which would be an observable change.


@Centril has relayed to me in private conversation that they tried examples like this under variants of their desugaring and did not witness any change in behavior. Our best guess as to why this is, is because the desugaring is, in the end, turning into a match which is itself a terminating scope for the temporaries in the match-input, and so you don't need to recreate such an effect by other means.

@Centril Centril self-assigned this May 2, 2019
@pnkfelix
Copy link
Member

pnkfelix commented May 3, 2019

Note: it is nonetheless possible that the distinction here is more subtle and my above example isn't quite right. in particular, I'm a little worried that there may have been a reason that the example from PR #21984 puts the for-expression in the tail expression of a block, and that examples that don't take similar care might fail to pick up on a relevant distinction here...

Centril added a commit to Centril/rust that referenced this issue Aug 10, 2019
Cleanup & Simplify stuff in lowering

Closes rust-lang#60253 as a byproduct.

It turns out that it is in fact necessary to have a `DropTemps(...)` around the `match_expr` and there is a test (https://github.com/rust-lang/rust/blob/master/src/test/ui/issues/issue-13304.rs) which fails without that.

r? @eddyb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants