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

make for PAT in ITER_EXPR { ... } a terminating-scope for ITER_EXPR. #21984

Merged
merged 1 commit into from
Feb 7, 2015

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Feb 5, 2015

make for PAT in ITER_EXPR { ... } a terminating-scope for ITER_EXPR.

In effect, temporary anonymous values created during the evaluation of ITER_EXPR no longer not live for the entirety of the block surrounding the for-loop; instead they only live for the extent of the for-loop itself, and no longer.


There is one case I know of that this breaks, demonstrated to me by @nikomatsakis (but it is also a corner-case that is useless in practice). Here is that case:

fn main() {
    let mut foo: Vec<&i8> = Vec::new();
    for i in &[1, 2, 3] { foo.push(i) }
}

Note that if you add any code following the for-loop above, or even a semicolon to the end of it, then the code will stop compiling (i.e., it gathers a vector of references but the gathered vector cannot actually be used.)

(The above code, despite being useless, did occur in one run-pass test by accident; that test is updated here to accommodate the new striction.)


So, technically this is a:

[breaking-change]

In effect, temporary anonymous values created during the evaluation of
ITER_EXPR no longer not live for the entirety of the block surrounding
the for-loop; instead they only live for the extent of the for-loop
itself, and no longer.

----

There is one case I know of that this breaks, demonstrated to me by
niko (but it is also a corner-case that is useless in practice).  Here
is that case:

```
fn main() {
    let mut foo: Vec<&i8> = Vec::new();
    for i in &[1, 2, 3] { foo.push(i) }
}
```

Note that if you add any code following the for-loop above, or even a
semicolon to the end of it, then the code will stop compiling (i.e.,
it gathers a vector of references but the gathered vector cannot
actually be used.)

(The above code, despite being useless, did occur in one run-pass test
by accident; that test is updated here to accommodate the new
striction.)

----

So, technically this is a:

[breaking-change]
@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 5, 2015

r? @nikomatsakis

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 5, 2015

Note that this landing this is blocked on PR #21980.

That is, on my machine with my other commits, if you do not have a robust codemap, then you end up ICE'ing (for me, its while compiling librand with --test), and as far as I could tell it was due to this change to the handling of for loops.

(It is possible that some different way of manipulating the spans than what I did here would resolve the above problem. But I think its more important that we first land this change to the terminating scopes before we worry too much about span handling.)

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 5, 2015

(oh, and this was spawned off of #21972)

@nikomatsakis
Copy link
Contributor

r+ -- but I'm not informing poor ol' @bors since #21980 has not landed.

@bors
Copy link
Contributor

bors commented Feb 6, 2015

🙀 You have the wrong number! Please try again with b445bf2.

@nikomatsakis
Copy link
Contributor

@bors don't be so nosy

@pnkfelix
Copy link
Member Author

pnkfelix commented Feb 7, 2015

@bors r=nikomatsakis b445bf2

bors added a commit that referenced this pull request Feb 7, 2015
…=nikomatsakis

make `for PAT in ITER_EXPR { ... }` a terminating-scope for ITER_EXPR.

In effect, temporary anonymous values created during the evaluation of ITER_EXPR no longer not live for the entirety of the block surrounding the for-loop; instead they only live for the extent of the for-loop itself, and no longer.

----

There is one case I know of that this breaks, demonstrated to me by @nikomatsakis  (but it is also a corner-case that is useless in practice).  Here is that case:

```
fn main() {
    let mut foo: Vec<&i8> = Vec::new();
    for i in &[1, 2, 3] { foo.push(i) }
}
```

Note that if you add any code following the for-loop above, or even a semicolon to the end of it, then the code will stop compiling (i.e., it gathers a vector of references but the gathered vector cannot actually be used.)

(The above code, despite being useless, did occur in one run-pass test by accident; that test is updated here to accommodate the new striction.)

----

So, technically this is a:

[breaking-change]
@bors
Copy link
Contributor

bors commented Feb 7, 2015

⌛ Testing commit b445bf2 with merge 61626b3...

@bors bors merged commit b445bf2 into rust-lang:master Feb 7, 2015
@nikomatsakis nikomatsakis mentioned this pull request Feb 11, 2015
7 tasks
bors added a commit that referenced this pull request Apr 24, 2019
Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in #21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of 91b0abd from #59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Apr 25, 2019
…ps, r=oli-obk

Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in rust-lang#21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of rust-lang@91b0abd from rust-lang#59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Apr 25, 2019
…ps, r=oli-obk

Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in rust-lang#21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of rust-lang@91b0abd from rust-lang#59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk
Centril added a commit to Centril/rust that referenced this pull request Apr 26, 2019
…ps, r=oli-obk

Introduce hir::ExprKind::Use and employ in for loop desugaring.

In the `for $pat in $expr $block` desugaring we end with a `{ let _result = $match_expr; _result }` construct which makes `for` loops into a terminating scope and affects drop order. The construct was introduced in year 2015 by @pnkfelix in rust-lang#21984.

This PR replaces the construct with `hir::ExprKind::Use(P<hir::Expr>)` which is equivalent semantically but should hopefully be less costly in terms of compile time performance (to be determined).

This is extracted out of rust-lang@91b0abd from rust-lang#59288 for easier review and so that the perf implications wrt. `for`-loops can be measured.

r? @oli-obk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants