Don't depend on `Allocation` sizes for pattern length #56540

oli-obk · 2018-12-05T17:35:33Z

And generally be more explicit about shortcomings of the implementation

cc @RalfJung

rust-highfive · 2018-12-05T17:35:37Z

r? @cramertj

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

cramertj · 2018-12-05T18:24:35Z

r? @RalfJung

src/librustc_mir/hair/pattern/_match.rs

RalfJung · 2018-12-07T16:44:13Z

Is there a "please select another random reviewer"? I'm afraid I don't think I can review this, pattern matching is complicated and I have zero familiarity with it. In fact I actively try to avoid it, pattern matching is one of the main reasons I only work on MIR and below ;)

oli-obk · 2018-12-07T17:27:37Z

r? @varkor you've done a lot of pattern work recently

rust-highfive · 2018-12-07T19:01:40Z

The job x86_64-gnu-llvm-5.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:152e3794:start=1544206089533048773,finish=1544206090621814294,duration=1088765521
$ 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
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:47:39] .................................................................................i.................. 200/2926
[00:47:47] .................................................................................................... 300/2926
[00:47:56] .................................................................................................... 400/2926
[00:48:05] .................................................................................................... 500/2926
[00:48:16] ............................................F....................................................... 600/2926
[00:48:40] .................................................................................................... 800/2926
[00:48:49] .................................................................................................... 900/2926
[00:49:03] .................................................................................................... 1000/2926
[00:49:15] .................................................................................................... 1100/2926
---
[00:52:43] .................................................................................................... 2600/2926
[00:52:51] .................................................................................................... 2700/2926
[00:53:01] .................................................................................................... 2800/2926
[00:53:12] .................................................................................................... 2900/2926
pers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/ctfe/references/auxiliary"
[00:53:16] ------------------------------------------
[00:53:16] 
[00:53:16] ------------------------------------------
[00:53:16] stderr:
[00:53:16] stderr:
[00:53:16] ------------------------------------------
[00:53:16] {"message":"unreachable pattern","code":{"code":"unreachable_patterns","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/ctfe/references.rs","byte_start":868,"byte_end":871,"line_start":33,"line_end":33,"column_start":9,"column_end":12,"is_primary":true,"text":[{"text":"        BOO => panic!(),","highlight_start":9,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unreachable_patterns)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"warning: unreachable pattern\n  --> /checkout/src/test/run-pass/ctfe/references.rs:33:9\n   |\nLL |         BOO => panic!(),\n   |         ^^^\n   |\n   = note: #[warn(unreachable_patterns)] on by default\n\n"}
[00:53:16] ------------------------------------------
[00:53:16] 
[00:53:16] thread '[run-pass] run-pass/ctfe/references.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3284:9
[00:53:16] note: Run with `RUST_BACKTRACE=1` for a backtrace.

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)

bors · 2018-12-10T03:44:10Z

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

rust-highfive · 2018-12-10T16:32:33Z

The job x86_64-gnu-llvm-5.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:08f93334:start=1544456192608146835,finish=1544456193698296910,duration=1090150075
$ 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
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:49:58] .................................................................................i.................. 200/2926
[00:50:06] .................................................................................................... 300/2926
[00:50:16] .................................................................................................... 400/2926
[00:50:25] .................................................................................................... 500/2926
[00:50:36] ............................................F....................................................... 600/2926
[00:51:01] .................................................................................................... 800/2926
[00:51:10] .................................................................................................... 900/2926
[00:51:25] .................................................................................................... 1000/2926
[00:51:37] .................................................................................................... 1100/2926
---
[00:55:11] .................................................................................................... 2600/2926
[00:55:19] .................................................................................................... 2700/2926
[00:55:29] .................................................................................................... 2800/2926
[00:55:41] .................................................................................................... 2900/2926
pers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/ctfe/references/auxiliary"
[00:55:44] ------------------------------------------
[00:55:44] 
[00:55:44] ------------------------------------------
[00:55:44] stderr:
[00:55:44] stderr:
[00:55:44] ------------------------------------------
[00:55:44] {"message":"unreachable pattern","code":{"code":"unreachable_patterns","explanation":null},"level":"warning","spans":[{"file_name":"/checkout/src/test/run-pass/ctfe/references.rs","byte_start":868,"byte_end":871,"line_start":33,"line_end":33,"column_start":9,"column_end":12,"is_primary":true,"text":[{"text":"        BOO => panic!(),","highlight_start":9,"highlight_end":12}],"label":null,"suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[{"message":"#[warn(unreachable_patterns)] on by default","code":null,"level":"note","spans":[],"children":[],"rendered":null}],"rendered":"warning: unreachable pattern\n  --> /checkout/src/test/run-pass/ctfe/references.rs:33:9\n   |\nLL |         BOO => panic!(),\n   |         ^^^\n   |\n   = note: #[warn(unreachable_patterns)] on by default\n\n"}
[00:55:44] ------------------------------------------
[00:55:44] 
[00:55:44] thread '[run-pass] run-pass/ctfe/references.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:3252:9
[00:55:44] note: Run with `RUST_BACKTRACE=1` for a backtrace.

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)

oli-obk · 2018-12-11T08:06:07Z

src/test/run-pass/ctfe/references.rs

    match &43 {
        &42 => panic!(),
-        BOO => panic!(),
+        BOO => panic!(), // pattern is unreachable


This is a behavioral change of this PR. We now detect that BOO is &42. This solely emits new warnings about unreachable patterns.

varkor

My main worry is that the tests don't seem to be catching the "unreachable pattern" warnings — comments are insufficient (it's unclear whether "this should warn" means it does, or we'd simply like it to in the future).

Behaviour-wise, this should only increase the number of "unreachable pattern" warnings, right? There should be no patterns that were previously unreachable that are now reachable?

varkor · 2018-12-12T23:12:36Z

src/librustc_mir/hair/pattern/_match.rs

+            },
+            // fat pointers stay the same
+            (ConstValue::ScalarPair(..), _, _) => val,
+            // FIXME(oli-obk): this is reachable for `const FOO: &&&u32 = &&&42;` being used


What happens in this case in the existing code? A bug again?

Currently this is an ICE: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=76fe5897409eefe240b587fb5ba59f6e

It will stay an ICE, but this PR is prepping the road for making it saner, because it's an explicit choice to ICE instead of just a random assert triggering.

src/librustc_mir/hair/pattern/_match.rs

src/test/ui/pattern/slice-pattern-const.rs

src/test/run-pass/ctfe/references.rs

src/librustc_mir/hair/pattern/_match.rs

varkor · 2018-12-12T23:33:29Z

src/librustc_mir/hair/pattern/_match.rs

+    ///
+    /// `crty` and `rty` can differ because you can use array constants in the presence of slice
+    /// patterns. So the pattern may end up being a slice, but the constant is an array. We convert
+    /// the array to a slice in that case


Nit: end comments with full stops (here and elsewhere).

src/test/ui/pattern/slice-pattern-const-3.rs

varkor · 2018-12-14T16:42:36Z

@oli-obk: just to confirm, this doesn't affect the behaviour regarding the issue in #53708?

oli-obk · 2018-12-14T17:07:23Z

this doesn't affect the behaviour regarding the issue in #53708

Nope, we don't take apart constants. I was intending to do that as a follow-up to this PR in order to keep the diffs sane

varkor · 2018-12-14T17:27:29Z

@bors r+

bors · 2018-12-14T17:27:30Z

📌 Commit 1c2a29e has been approved by varkor

bors · 2018-12-14T19:40:24Z

⌛ Testing commit 1c2a29e with merge 19b52568c88514b941f5d7aef723e9c67805ed5a...

bors · 2018-12-14T20:36:07Z

💔 Test failed - status-travis

rust-highfive · 2018-12-14T20:36:09Z

The job dist-x86_64-apple-alt 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.

[00:02:23]       Memory: 8 GB
[00:02:23]       Boot ROM Version: VMW71.00V.0.B64.1704110547
[00:02:23]       Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
[00:02:23]       SMC Version (system): 2.8f0
[00:02:23]       Serial Number (system): VMCwqzKbYhNn
[00:02:23] 
[00:02:23] hw.ncpu: 4
[00:02:23] hw.byteorder: 1234
[00:02:23] hw.memsize: 8589934592
---
Building stage2 tool cargo (x86_64-apple-darwin)
[00:46:20]  Downloading crates ...
[00:46:20] warning: spurious network error (2 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[00:46:20] warning: spurious network error (1 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[00:46:20] error: failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`
[00:46:20] Caused by:
[00:46:20]   [6] Couldn't resolve host name (Could not resolve host: crates.io)
[00:46:20] command did not execute successfully: "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/stage0/bin/cargo" "build" "--target" "x86_64-apple-darwin" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/Users/travis/build/rust-lang/rust/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json"
[00:46:20] expected success, got: exit code: 101
[00:46:20] expected success, got: exit code: 101
[00:46:20] failed to run: /Users/travis/build/rust-lang/rust/build/bootstrap/debug/bootstrap build
[00:46:20] Build completed unsuccessfully in 0:42:56
[00:46:20] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0a619118
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Dec 14 20:35:32 GMT 2018
---
travis_fold:start:after_failure.2
travis_time:start:09820e60
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
total 0
drwx------+ 15 travis  staff  510 Jan 25  2018 ..
drwx------   2 travis  staff   68 Dec  6  2017 .
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:0f312d1a
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
travis_time:end:0f312d1a:start=1544819740476893000,finish=1544819740492970000,duration=16077000
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1631322c
$ 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:023a9b32
travis_time:start:023a9b32
$ 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:05e6f9fe
$ dmesg | grep -i kill
$ dmesg | grep -i kill
Unable to obtain kernel buffer: Operation not permitted
usage: sudo dmesg
travis_fold:end:after_failure.6

Done. Your build exited with 1.

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)

oli-obk · 2018-12-14T21:42:20Z

@bors retry network error

bors · 2018-12-15T04:24:23Z

⌛ Testing commit 1c2a29e with merge 91857a3...

@RalfJung

Don't depend on `Allocation` sizes for pattern length And generally be more explicit about shortcomings of the implementation cc @RalfJung

bors · 2018-12-15T06:42:22Z

☀️ Test successful - status-appveyor, status-travis
Approved by: varkor
Pushing 91857a3 to master...

rust-highfive assigned cramertj Dec 5, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 5, 2018

rust-highfive assigned RalfJung and unassigned cramertj Dec 5, 2018

RalfJung reviewed Dec 7, 2018

View reviewed changes

src/librustc_mir/hair/pattern/_match.rs Outdated Show resolved Hide resolved

rust-highfive assigned varkor and unassigned RalfJung Dec 7, 2018

oli-obk force-pushed the less_const_hackery branch from a837830 to 3b24d02 Compare December 7, 2018 18:06

oli-obk added 3 commits December 10, 2018 16:34

Don't depend on Allocation sizes for pattern length

a5a7fcb

Document and simplify pattern matching with constants as patterns

8f2ce3d

Add a few more tests of edge cases

8a9414a

oli-obk force-pushed the less_const_hackery branch from 3b24d02 to 8a9414a Compare December 10, 2018 15:35

Diagnostics improvement!

7d1b19c

oli-obk commented Dec 11, 2018

View reviewed changes

varkor reviewed Dec 12, 2018

View reviewed changes

oli-obk added 4 commits December 13, 2018 12:23

Doc comment format

46c1da6

Fix indent

25a4f76

Update tests to show diagnostics

5df6af4

Expand on a few comments

1c2a29e

varkor reviewed Dec 14, 2018

View reviewed changes

src/test/ui/pattern/slice-pattern-const-3.rs Show resolved Hide resolved

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 Dec 14, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2018

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 Dec 14, 2018

bors added a commit that referenced this pull request Dec 15, 2018

Auto merge of #56540 - oli-obk:less_const_hackery, r=varkor

91857a3

Don't depend on `Allocation` sizes for pattern length And generally be more explicit about shortcomings of the implementation cc @RalfJung

bors merged commit 1c2a29e into rust-lang:master Dec 15, 2018

bors mentioned this pull request Dec 15, 2018

Don't emit Unevaluated from const_eval #56723

Merged

oli-obk deleted the less_const_hackery branch June 15, 2020 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't depend on `Allocation` sizes for pattern length #56540

Don't depend on `Allocation` sizes for pattern length #56540

oli-obk commented Dec 5, 2018

rust-highfive commented Dec 5, 2018

cramertj commented Dec 5, 2018

RalfJung commented Dec 7, 2018

oli-obk commented Dec 7, 2018

rust-highfive commented Dec 7, 2018

bors commented Dec 10, 2018

rust-highfive commented Dec 10, 2018

oli-obk Dec 11, 2018

varkor left a comment

varkor Dec 12, 2018

oli-obk Dec 13, 2018

varkor Dec 12, 2018

varkor commented Dec 14, 2018

oli-obk commented Dec 14, 2018

varkor commented Dec 14, 2018

bors commented Dec 14, 2018

bors commented Dec 14, 2018

bors commented Dec 14, 2018

rust-highfive commented Dec 14, 2018

oli-obk commented Dec 14, 2018

bors commented Dec 15, 2018

bors commented Dec 15, 2018

Don't depend on Allocation sizes for pattern length #56540

Don't depend on Allocation sizes for pattern length #56540

Conversation

oli-obk commented Dec 5, 2018

rust-highfive commented Dec 5, 2018

cramertj commented Dec 5, 2018

RalfJung commented Dec 7, 2018

oli-obk commented Dec 7, 2018

rust-highfive commented Dec 7, 2018

bors commented Dec 10, 2018

rust-highfive commented Dec 10, 2018

oli-obk Dec 11, 2018

Choose a reason for hiding this comment

varkor left a comment

Choose a reason for hiding this comment

varkor Dec 12, 2018

Choose a reason for hiding this comment

oli-obk Dec 13, 2018

Choose a reason for hiding this comment

varkor Dec 12, 2018

Choose a reason for hiding this comment

varkor commented Dec 14, 2018

oli-obk commented Dec 14, 2018

varkor commented Dec 14, 2018

bors commented Dec 14, 2018

bors commented Dec 14, 2018

bors commented Dec 14, 2018

rust-highfive commented Dec 14, 2018

oli-obk commented Dec 14, 2018

bors commented Dec 15, 2018

bors commented Dec 15, 2018

Don't depend on `Allocation` sizes for pattern length #56540

Don't depend on `Allocation` sizes for pattern length #56540