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

[let_chains, 3/6] And then there was only Loop #61988

Merged
merged 12 commits into from Jul 6, 2019

Conversation

Projects
None yet
7 participants
@Centril
Copy link
Member

commented Jun 20, 2019

Here we remove hir::ExprKind::While.
Instead, we desugar: 'label: while $cond $body into:

'label: loop {
    match DropTemps($cond) {
        true => $body,
        _ => break,
    }
}

Per #53667 (comment).
This is a follow up to #59288 which did the same for if expressions.

r? @matthewjasper

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Jun 20, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:02dd9cb0:start=1561033897619674660,finish=1561033899922250212,duration=2302575552
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:04:44] tidy error: /checkout/src/librustc/hir/mod.rs:1686: trailing whitespace
[00:04:48] some tidy checks failed
[00:04:48] 
[00:04:48] 
[00:04:48] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:48] 
[00:04:48] 
[00:04:48] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:48] Build completed unsuccessfully in 0:01:14
---
travis_time:end:16211a70:start=1561034200960397923,finish=1561034200965500943,duration=5103020
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:150830a3
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:049c072b
travis_time:start:049c072b
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:00241008
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril Centril force-pushed the Centril:there-is-only-loop branch from b0b569a to dad7845 Jun 20, 2019

@@ -191,7 +191,7 @@ pub fn change_continue_label() {
}

#[cfg(not(cfail1))]
#[rustc_clean(cfg="cfail2", except="HirBody, mir_built")]
#[rustc_clean(cfg="cfail2", except="typeck_tables_of, HirBody, mir_built, optimized_mir")]

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Jun 20, 2019

Contributor

Hmm, while true isn't being optimized to be the same as loop here. This is probably not blocking since we lint against while true. cc @oli-obk @wesleywiser

This comment has been minimized.

Copy link
@Centril

Centril Jun 20, 2019

Author Member

So this would be (in HIR terms):

loop {
    match DropTemps(true) {
        true => $body,
        _ => break,
    }
}

which feels like something const-prop should reduce to (ofc it operates on MIR... not HIR..):

loop {
    match true {
        true => $body,
        _ => break,
    }
}

==>

loop {
    $body
}

This comment has been minimized.

Copy link
@Centril

Centril Jun 24, 2019

Author Member

The typeck_tables_of bits are fixed now at least. The incremental behavior is the same for while and while let loops now which feels at least consistent.

I also agree that it shouldn't be blocking.

Show resolved Hide resolved src/librustc_lint/builtin.rs Outdated
@@ -4,7 +4,6 @@
// See https://github.com/rust-lang/rust/issues/51350 for more information.

const CRASH: () = 'a: while break 'a {};
//~^ ERROR constant contains unimplemented expression type

This comment has been minimized.

Copy link
@oli-obk

oli-obk Jun 21, 2019

Contributor

ugh, that's a breaking change.

@eddyb what if we did const prop and branch cleanup before const qualif?

Actually, just branch cleanup may suffice here. @Centril what's the MIR of this constant after your changes?

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Jun 21, 2019

Contributor

The issue is probably that we now generate a FalseUnwind for while loops now. We could ignore them in const qualification and allow all "loops that can't ever loop" in consts but they have to stay in the MIR until borrow checking.

This comment has been minimized.

Copy link
@Centril

Centril Jun 21, 2019

Author Member

ugh, that's a breaking change.

That's... debatable. :)

From my perspective this is a bug-fix that fixes the unjustified semantic divergence between loop- and while-expressions. Moreover, before 1.33.0 this was an ICE or error all the way back to 1.0.0. Also, you have to literally write 'label: while break 'label {}. Other forms you might think should behave similarly such as while break {} do not work. I also find it highly unlikely that anyone would ever write 'label: while break 'label {} because it is useless.

I r+ed this test in #60360 because it documented the behavior of the compiler. But now that the behavior is fixed so too should the test.

@eddyb what if we did const prop and branch cleanup before const qualif?

That would make const-prop a part of our stability guarantees. I think that's a non-starter from a T-Lang POV.

We could ignore them in const qualification and allow all "loops that can't ever loop" in consts but they have to stay in the MIR until borrow checking.

We could... but that would allow more behavior that is disallowed now. Also, I think it is not worth any of y'all's time to make 'label: while break 'label {} continue to work.

This comment has been minimized.

Copy link
@eddyb

eddyb Jun 25, 2019

Member

Wait, so what's the current behavior, ignoring constants, for 'label: while break 'label {}?
Is it a noop, or an error?

This comment has been minimized.

Copy link
@Centril

Centril Jun 25, 2019

Author Member

This comment has been minimized.

Copy link
@eddyb

eddyb Jul 1, 2019

Member

@matthewjasper I looked into FalseUnwind and indeed it should've been ignored in const-checking, see #62272.

(Probably doesn't need to hold up this PR, it can be fixed independently).

@Centril Centril force-pushed the Centril:there-is-only-loop branch from dad7845 to cab21f9 Jun 21, 2019

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 22, 2019

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

@Centril Centril force-pushed the Centril:there-is-only-loop branch from cab21f9 to 74f6fa3 Jun 22, 2019

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 23, 2019

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

@Centril Centril force-pushed the Centril:there-is-only-loop branch from 74f6fa3 to 0aeb1d9 Jun 23, 2019

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2019

Rebased. Also cleaned up lowering after #60861 so that it is more in line with how If is lowered and so that there's less duplication..

This should be good to go aside from question in #61988 (comment).

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Jun 23, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0cb87e51:start=1561309219599806964,finish=1561309222495434599,duration=2895627635
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
travis_time:start:test_assembly
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:06:27] 
[01:06:27] running 9 tests
[01:06:27] iiiiiiiii
[01:06:27] 
[01:06:27]  finished in 0.154
[01:06:27] travis_fold:end:test_assembly


[01:06:27] travis_time:end:test_assembly:start=1561313220616724513,finish=1561313220771425133,duration=154700620

[01:06:27] travis_fold:start:test_incremental
travis_time:start:test_incremental
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:06:27] 
[01:06:27] running 104 tests
[01:06:42] ............................................FF...................................................... 100/104
[01:06:43] failures:
[01:06:43] 
[01:06:43] ---- [incremental] incremental/hashes/while_let_loops.rs stdout ----
[01:06:43] 
[01:06:43] 
[01:06:43] error in revision `cfail2`: test compilation failed although it shouldn't!
[01:06:43] status: exit code: 1
[01:06:43] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/while_let_loops.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/while_let_loops/while_let_loops.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/while_let_loops" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/while_let_loops/auxiliary"
[01:06:43] ------------------------------------------
[01:06:43] 
[01:06:43] ------------------------------------------
[01:06:43] stderr:
[01:06:43] stderr:
[01:06:43] ------------------------------------------
[01:06:43] error: `typeck_tables_of(change_break_label)` should be dirty but is not
[01:06:43]    |
[01:06:43]    |
[01:06:43] LL | / pub fn change_break_label() {
[01:06:43] LL | |     let mut _x = 0;
[01:06:43] LL | |     'outer: while let Some(0u32) = None {
[01:06:43] LL | |         'inner: while let Some(0u32) = None {
[01:06:43] LL | |     }
[01:06:43] LL | | }
[01:06:43]    | |_^
[01:06:43] 
[01:06:43] 
[01:06:43] error: `typeck_tables_of(change_continue_label)` should be dirty but is not
[01:06:43]    |
[01:06:43] LL | / pub fn change_continue_label() {
[01:06:43] LL | |     let mut _x = 0;
[01:06:43] LL | |     let mut _x = 0;
[01:06:43] LL | |     'outer: while let Some(0u32) = None {
[01:06:43] LL | |         'inner: while let Some(0u32) = None {
[01:06:43] LL | |     }
[01:06:43] LL | | }
[01:06:43]    | |_^
[01:06:43] 
---
[01:06:43] 
[01:06:43] 
[01:06:43] ---- [incremental] incremental/hashes/while_loops.rs stdout ----
[01:06:43] 
[01:06:43] error in revision `cfail2`: test compilation failed although it shouldn't!
[01:06:43] status: exit code: 1
[01:06:43] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/while_loops.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/while_loops/while_loops.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/while_loops" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/while_loops/auxiliary"
[01:06:43] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:521:22
[01:06:43] ------------------------------------------
[01:06:43] 
[01:06:43] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:06:43] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[01:06:43] ------------------------------------------
[01:06:43] stderr:
[01:06:43] ------------------------------------------
[01:06:43] error: `typeck_tables_of(change_break_label)` should be dirty but is not
[01:06:43]    |
[01:06:43]    |
[01:06:43] LL | / pub fn change_break_label() {
[01:06:43] LL | |     let mut _x = 0;
[01:06:43] LL | |     'outer: while true {
[01:06:43] LL | |         'inner: while true {
[01:06:43] LL | |     }
[01:06:43] LL | | }
[01:06:43]    | |_^
[01:06:43] 
[01:06:43] 
[01:06:43] error: `typeck_tables_of(change_continue_label)` should be dirty but is not
[01:06:43]    |
[01:06:43] LL | / pub fn change_continue_label() {
[01:06:43] LL | |     let mut _x = 0;
[01:06:43] LL | |     let mut _x = 0;
[01:06:43] LL | |     'outer: while true {
[01:06:43] LL | |         'inner: while true {
[01:06:43] LL | |     }
[01:06:43] LL | | }
[01:06:43]    | |_^
[01:06:43] 
---
[01:06:43] test result: FAILED. 102 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out
[01:06:43] 
[01:06:43] 
[01:06:43] 
[01:06:43] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/incremental" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "incremental" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[01:06:43] 
[01:06:43] 
[01:06:43] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:06:43] Build completed unsuccessfully in 1:01:50

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Fixed the incremental test failure.

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

r=me with the liveness comment updated

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 26, 2019

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

@Centril Centril force-pushed the Centril:there-is-only-loop branch from a5acea6 to 63dc9a4 Jun 26, 2019

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

Rebased, updated the liveness comment, and also "blessed" the mir-opt test to the actual MIR output.

@bors r=matthewjasper rollup=never

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

📌 Commit 63dc9a4 has been approved by matthewjasper

@bors

This comment was marked as resolved.

Copy link
Contributor

commented Jun 26, 2019

🌲 The tree is currently closed for pull requests below priority 999, this pull request will be tested once the tree is reopened

@Centril

This comment was marked as outdated.

Copy link
Member Author

commented Jun 30, 2019

@bors p=-1 due to toolstate window.

@Centril Centril force-pushed the Centril:there-is-only-loop branch from e724761 to 9b1d513 Jul 6, 2019

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2019

Rebased; @bors r=matthewjasper rollup=never

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

📌 Commit 9b1d513 has been approved by matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

⌛️ Testing commit 9b1d513 with merge 254f201...

bors added a commit that referenced this pull request Jul 6, 2019

Auto merge of #61988 - Centril:there-is-only-loop, r=matthewjasper
[let_chains, 3/6] And then there was only Loop

Here we remove `hir::ExprKind::While`.
Instead, we desugar: `'label: while $cond $body` into:

```rust
'label: loop {
    match DropTemps($cond) {
        true => $body,
        _ => break,
    }
}
```

Per #53667 (comment).
This is a follow up to #59288 which did the same for `if` expressions.

r? @matthewjasper

@matthiaskrgr matthiaskrgr referenced this pull request Jul 6, 2019

Merged

Update clippy #62434

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2019

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: matthewjasper
Pushing 254f201 to master...

@bors bors added the merged-by-bors label Jul 6, 2019

@bors bors merged commit 9b1d513 into rust-lang:master Jul 6, 2019

4 checks passed

homu Test successful
Details
pr Build #20190706.11 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details

@Centril Centril deleted the Centril:there-is-only-loop branch Jul 6, 2019

mikerite added a commit to mikerite/rust-clippy that referenced this pull request Jul 6, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 6, 2019

bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.