Change for-loop desugar to not borrow the iterator during the loop #42265

Zoxc · 2017-05-27T18:26:38Z

This is enables the use of suspend points inside for-loops in movable generators. This is illegal in the current desugaring as iter is borrowed across the body.

rust-highfive · 2017-05-27T18:26:42Z

r? @eddyb

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

eddyb · 2017-05-27T18:28:37Z

src/librustc/hir/lowering.rs

@@ -2146,10 +2147,11 @@ impl<'a> LoweringContext<'a> {
                //     let result = match ::std::iter::IntoIterator::into_iter(<head>) {
                //       mut iter => {
                //         [opt_ident]: loop {
-                //           match ::std::iter::Iterator::next(&mut iter) {
-                //             ::std::option::Option::Some(<pat>) => <body>,
+                //           let <pat> = match ::std::iter::Iterator::next(&mut iter) {


This changes semantics, although I can't think of any situation in which this would fail.
FWIW when I asked about while let Some(x) = iter.next() - that would correspond to the old desugaring, not your new one, wouldn't it?

eddyb · 2017-05-27T18:40:24Z

src/librustc_const_eval/diagnostics.rs

@@ -452,12 +452,14 @@ enum Method { GET, POST }


 E0297: r##"
+#### Note: this error code is no longer emitted by the compiler.


@rust-lang/compiler This is an annoying (lack of a?) convention - is there any consensus?

This seems to be what we have done in the past. I see some instances of ## and some with ####.

Personally, I long to get rid of --explain XXX and just have a "long error mode" in which the long explanations are emitted 'inline'. That would basically make this point irrelevant.

eddyb · 2017-05-28T16:16:01Z

Started crater run.

eddyb · 2017-05-28T20:55:24Z

Crater report is pretty long, but by looking at non-root regressions I found something.

eddyb · 2017-05-28T22:56:45Z

src/test/run-pass/for-loop-has-unit-body.rs

+
+fn main() {
+    for i in 0..3 {
+        unsafe { std::mem::uninitialized() }


Not sure this makes sense? I'd rather have a compile-fail test checking there's a type mismatch when there's a body that's not ().

eddyb · 2017-05-28T22:56:55Z

src/librustc/hir/lowering.rs

@@ -2172,7 +2172,7 @@ impl<'a> LoweringContext<'a> {
                //             ::std::option::Option::Some(val) => val,
                //             ::std::option::Option::None => break
                //           };
-                //           <body>
+                //           <body>;


That's not the same, ; corresponds to StmtSemi.

Yeah, there aren't really syntax for StmtExpr?

Certain expressions (loops? not sure. maybe if too) don't require ; after them.

eddyb · 2017-05-29T12:43:24Z

src/test/run-pass/for-loop-has-unit-body.rs

+
+    // Check that the tail statement in the body can be unit
+    for _ in 0..3 {
+        ()


That's not that interesting, since it doesn't directly check for the property I care about, i.e. that non-() bodies are denied.

steveklabnik · 2017-05-30T18:39:08Z

I am not an expert on what's going on here but if we're changing the sugar, I would like to see https://doc.rust-lang.org/std/iter/#for-loops-and-intoiterator updated

eddyb · 2017-06-01T08:44:04Z

src/test/run-pass/for-loop-has-unit-body.rs

+    for _ in 0..3 {
+        ()
+    }
+}


Missing newline, here and in the other test.

eddyb · 2017-06-01T08:45:54Z

Started a second crater run.

eddyb · 2017-06-01T15:49:24Z

Second crater report is clean (modulo network errors)!

Mark-Simulacrum · 2017-06-04T01:16:45Z

I believe the documentation still needs updating (#42265 (comment)) and @eddyb wanted a few changes made (#42265 (comment)).

Zoxc · 2017-06-04T01:23:59Z

Those have both been addressed.

Mark-Simulacrum · 2017-06-04T01:31:26Z

Ah, okay. Thanks!

eddyb · 2017-06-04T01:36:02Z

@bors r+

bors · 2017-06-04T01:36:03Z

📌 Commit cfdbff7 has been approved by eddyb

bors · 2017-06-04T05:44:45Z

⌛ Testing commit cfdbff7 with merge 0b17b4c...

Change for-loop desugar to not borrow the iterator during the loop This is enables the use of suspend points inside for-loops in movable generators. This is illegal in the current desugaring as `iter` is borrowed across the body.

bors · 2017-06-04T11:17:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 0b17b4c to master...

mcarton · 2017-06-05T19:58:40Z

src/librustc/hir/lowering.rs

                //             ::std::option::Option::None => break
-                //           }
+                //           };
+                //           SemiExpr(<body>);


This comment is wrong, the statement used is a StmtExpr.

rust-highfive assigned eddyb May 27, 2017

eddyb reviewed May 27, 2017

View reviewed changes

Zoxc force-pushed the for-sugar branch from f6ecb84 to c94e44a Compare May 27, 2017 18:33

eddyb reviewed May 27, 2017

View reviewed changes

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 27, 2017

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 28, 2017

eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label May 28, 2017

eddyb reviewed May 28, 2017

View reviewed changes

eddyb suggested changes May 29, 2017

View reviewed changes

eddyb reviewed Jun 1, 2017

View reviewed changes

src/test/run-pass/for-loop-has-unit-body.rs

for _ in 0..3 {

()

}

}

Copy link

Member

eddyb Jun 1, 2017

Choose a reason for hiding this comment

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

Missing newline, here and in the other test.

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 1, 2017

eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Jun 1, 2017

Change for-loop desugar to not borrow the iterator during the loop

cfdbff7

Zoxc force-pushed the for-sugar branch from 9654e29 to cfdbff7 Compare June 1, 2017 16:34

Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2017

eddyb approved these changes Jun 4, 2017

View reviewed changes

Mark-Simulacrum added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 4, 2017

Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 4, 2017

bors merged commit cfdbff7 into rust-lang:master Jun 4, 2017

mcarton mentioned this pull request Jun 5, 2017

Rustup to *rustc 1.19.0-nightly (0418fa9d3 2017-06-04)* rust-lang/rust-clippy#1814

Merged

mcarton reviewed Jun 5, 2017

View reviewed changes

Mark-Simulacrum mentioned this pull request Jun 13, 2017

Rust 1.19 regression, minions 0.2.8, type mismatch #42618

Closed

brson mentioned this pull request Jun 15, 2017

Change the for-loop desugar so the break does not affect type inference. Fixes #42618 #42634

Merged

Mark-Simulacrum mentioned this pull request Jun 22, 2017

Rust 1.20 regression, multiqueue 0.3.2, match arms have incompatible types #42842

Closed

Zoxc mentioned this pull request Jul 13, 2017

Updated releases notes for 1.19 #42503

Closed

petrochenkov mentioned this pull request Jun 16, 2019

for loop desugaring treats the loop's trailing expression in unusual way #61902

Open

camsteffen mentioned this pull request Oct 27, 2021

Simplify for loop desugar #90352

Merged

		@@ -452,12 +452,14 @@ enum Method { GET, POST }


		E0297: r##"
		#### Note: this error code is no longer emitted by the compiler.

Change for-loop desugar to not borrow the iterator during the loop #42265

Change for-loop desugar to not borrow the iterator during the loop #42265

Uh oh!

Conversation

Zoxc commented May 27, 2017

Uh oh!

rust-highfive commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented May 28, 2017

Uh oh!

eddyb commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveklabnik commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Jun 1, 2017

Uh oh!

eddyb commented Jun 1, 2017

Uh oh!

Mark-Simulacrum commented Jun 4, 2017

Uh oh!

Zoxc commented Jun 4, 2017

Uh oh!

Mark-Simulacrum commented Jun 4, 2017

Uh oh!

eddyb commented Jun 4, 2017

Uh oh!

bors commented Jun 4, 2017

Uh oh!

bors commented Jun 4, 2017

Uh oh!

bors commented Jun 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!