Skip to content

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented Dec 17, 2020

Primarily, this fixes #56379. This also fixes incorrect interactions between or-patterns and slice patterns that I discovered while working on #56379. Those two examples show the incorrect diagnostics:

match &[][..] {
    [true] => {}
    [true // detected as unreachable but that's not true
        | false, ..] => {}
    _ => {}
}
match (true, None) {
    (true, Some(_)) => {}
    (false, Some(true)) => {}
    (true | false, None | Some(true // should be detected as unreachable
                               | false)) => {}
}

I did not measure any perf impact. However, I suspect that 616ba9f should have a negative impact on large or-patterns. I'll see what the perf run says; I have optimization ideas up my sleeve if needed.

EDIT: I initially had a noticeable perf impact that I thought unavoidable. I then proceeded to avoid it x)

r? @varkor
@rustbot label +A-exhaustiveness-checking

@rustbot rustbot added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 17, 2020
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 17, 2020
@Nadrieril Nadrieril force-pushed the usefulness-merging branch 3 times, most recently from d606216 to 616ba9f Compare December 17, 2020 05:29
@Nadrieril
Copy link
Member Author

Nadrieril commented Dec 17, 2020

Oh well, that's why I shouldn't push a PR just before sleep ^^. I thought a perf impact was unavoidable but it wasn't. I've edited the OP to reflect that.

@lqd
Copy link
Member

lqd commented Dec 17, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@bors
Copy link
Collaborator

bors commented Dec 17, 2020

⌛ Trying commit 616ba9f9f7f5845777a36e1a41a515e6c33a8776 with merge 14cfbb5782b26a8629319961f978f42d37deff26...

@bors
Copy link
Collaborator

bors commented Dec 17, 2020

☀️ Try build successful - checks-actions
Build commit: 14cfbb5782b26a8629319961f978f42d37deff26 (14cfbb5782b26a8629319961f978f42d37deff26)

@rust-timer
Copy link
Collaborator

Queued 14cfbb5782b26a8629319961f978f42d37deff26 with parent bdd0a78, future comparison URL.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 17, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (14cfbb5782b26a8629319961f978f42d37deff26): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 17, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 17, 2020

.6% regression on instructions for match-stress-enum-check (in check_match, as expected). Personally I think the diagnostic improvements are worth it.

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 18, 2020
`SpanSet` is heavily inspired from `DefIdForest`.
This is elegant but a bit of a perf gamble. That said, or-patterns
rarely have many branches and it's easy to optimize or revert if we ever
need to. In the meantime simpler code is worth it.
@varkor
Copy link
Contributor

varkor commented Dec 19, 2020

Looks good, thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 19, 2020

📌 Commit cefcadb has been approved by varkor

@bors 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 19, 2020
@bors
Copy link
Collaborator

bors commented Dec 19, 2020

⌛ Testing commit cefcadb with merge 30bf3f1f34da6ffc50c4df5abf6dd8d2aa5a3e73...

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[TIMING] StdLink { compiler: Compiler { stage: 0, host: TargetSelection { triple: "x86_64-unknown-linux-gnu", file: None } }, target_compiler: Compiler { stage: 0, host: TargetSelection { triple: "x86_64-unknown-linux-gnu", file: None } }, target: TargetSelection { triple: "x86_64-unknown-linux-gnu", file: None } } -- 0.001
[TIMING] Std { target: TargetSelection { triple: "x86_64-unknown-linux-gnu", file: None }, compiler: Compiler { stage: 0, host: TargetSelection { triple: "x86_64-unknown-linux-gnu", file: None } } } -- 73.325
Building LLVM for x86_64-unknown-linux-gnu
running: "cmake" "/checkout/src/llvm-project/llvm" "-G" "Ninja" "-DLLVM_ENABLE_ASSERTIONS=ON" "-DLLVM_TARGETS_TO_BUILD=AArch64;ARM;Hexagon;MSP430;Mips;NVPTX;PowerPC;RISCV;Sparc;SystemZ;WebAssembly;X86" "-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=AVR" "-DLLVM_INCLUDE_EXAMPLES=OFF" "-DLLVM_INCLUDE_TESTS=OFF" "-DLLVM_INCLUDE_DOCS=OFF" "-DLLVM_INCLUDE_BENCHMARKS=OFF" "-DLLVM_ENABLE_TERMINFO=OFF" "-DLLVM_ENABLE_LIBEDIT=OFF" "-DLLVM_ENABLE_BINDINGS=OFF" "-DLLVM_ENABLE_Z3_SOLVER=OFF" "-DLLVM_PARALLEL_COMPILE_JOBS=16" "-DLLVM_TARGET_ARCH=x86_64" "-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-linux-gnu" "-DLLVM_ENABLE_ZLIB=ON" "-DLLVM_ENABLE_LIBXML2=OFF" "-DLLVM_VERSION_SUFFIX=-rust-1.50.0-nightly" "-DCMAKE_INSTALL_MESSAGE=LAZY" "-DCMAKE_C_COMPILER_LAUNCHER=sccache" "-DCMAKE_CXX_COMPILER_LAUNCHER=sccache" "-DCMAKE_C_COMPILER=cc" "-DCMAKE_CXX_COMPILER=c++" "-DCMAKE_ASM_COMPILER=cc" "-DCMAKE_C_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_CXX_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_INSTALL_PREFIX=/checkout/obj/build/x86_64-unknown-linux-gnu/llvm" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64" "-DCMAKE_BUILD_TYPE=Release"
CMake Error: The source directory "/checkout/src/llvm-project/llvm" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
command did not execute successfully, got: exit code: 1


build script failed, must exit now', /cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.44/src/lib.rs:885:5
 finished in 0.009 seconds
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --stage 2 src/tools/cargo src/tools/cargotest
Build completed unsuccessfully in 0:01:13
Build completed unsuccessfully in 0:01:13
make: *** [check-aux] Error 1
Makefile:44: recipe for target 'check-aux' failed

@bors
Copy link
Collaborator

bors commented Dec 19, 2020

💔 Test failed - checks-actions

@bors 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 19, 2020
@varkor
Copy link
Contributor

varkor commented Dec 19, 2020

Looks spurious to me.

@bors retry

@bors 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 19, 2020
@bors
Copy link
Collaborator

bors commented Dec 19, 2020

⌛ Testing commit cefcadb with merge 1f5bc17...

@bors
Copy link
Collaborator

bors commented Dec 19, 2020

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 1f5bc17 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 19, 2020
@bors bors merged commit 1f5bc17 into rust-lang:master Dec 19, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 19, 2020
@Nadrieril Nadrieril deleted the usefulness-merging branch December 19, 2020 22:39
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2021
Identify unreachable subpatterns more reliably

In rust-lang#80104 I used `Span`s to identify unreachable sub-patterns in the presence of or-patterns during exhaustiveness checking. In rust-lang#80501 it was revealed that `Span`s are complicated and that this was not a good idea.
Instead, this PR identifies subpatterns logically: as a path in the tree of subpatterns of a given pattern. I made a struct that captures a set of such subpatterns. This is a bit complex, but thankfully self-contained; the rest of the code does not need to know anything about it.
Fixes rust-lang#80501. I think I managed to keep the perf neutral.

r? `@varkor`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

E0005; should all constructors be listed?
9 participants