Fully destructure slice and array constants into patterns #77390

oli-obk · 2020-10-01T07:39:18Z

There were a lot of workarounds for making such constants not opaque, now we expand all constants into patterns unless they are not structural match

… them opaque

rust-highfive · 2020-10-01T07:39:21Z

r? @varkor

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

oli-obk · 2020-10-01T07:39:30Z

@bors try @rust-timer queue

rust-timer · 2020-10-01T07:39:31Z

Awaiting bors try build completion

bors · 2020-10-01T07:39:42Z

⌛ Trying commit 14a0b10 with merge 6f61447b8e9a5d5b75684be6181567232aced705...

bors · 2020-10-01T08:25:52Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 6f61447b8e9a5d5b75684be6181567232aced705 (6f61447b8e9a5d5b75684be6181567232aced705)

rust-timer · 2020-10-01T08:25:54Z

Queued 6f61447b8e9a5d5b75684be6181567232aced705 with parent 00730fd, future comparison URL.

rust-timer · 2020-10-01T16:52:09Z

Finished benchmarking try commit (6f61447b8e9a5d5b75684be6181567232aced705): 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

oli-obk · 2020-10-02T08:11:18Z

ok, this is a perf improvement across the board. Unless people start putting include_bytes!(some_humongous_file) into constants used in patterns we're going to be fine. And if they do, we can always add an arbitrary limit after which we treat a slice/array constant as opaque

ready for review @varkor

compiler/rustc_mir_build/src/thir/pattern/_match.rs

RalfJung · 2020-10-02T09:18:31Z

compiler/rustc_mir_build/src/thir/pattern/_match.rs

-                    // (ty::ConstKind::Value(ConstValue::ByRef { .. }), ty::Slice(_)) => { ... }
-                    _ => Some(ConstantValue(value)),
-                }
+                Some(ConstantValue(value))


Is this related to exhaustiveness checking? Shouldn't opaque consts be treated as matching nothing for that as they might not be structural-match?

it is, very good catch. We still need a variant for string slices here, as string slices are not opaque, but ConstantValue is gone now and the replacement Str only handles &str constants. There are further improvements we can do here now that everything is explicit, but I'd rather not touch compare_const_vals in this PR.

I should note that I understand basically nothing of this code, if I caught anything that was an accident.^^

Also if the last 2 comments are bugfixes, is it possible to add a test?

There was no bugfix, no behaviour was changed, all I did was to move the bail-out (that was guaranteed to happen later) to an earlier site so the rest of the code that I was able to remove became dead code. This PR generally is mostly just removing dead code that became obsolete after destructuring all structural-eq constants into (non-const) patterns. This means except for &str, all PatKind::Const are now opaque.

Edit: they were behaving opaquely before already, but there was lots of code making sure we didn't run into ICEs around them by bailing out as "not contributing to exhaustiveness".

I see. Maybe add some doc comments to PatKind explaining this. :)

oli-obk · 2020-10-02T12:54:10Z

@bors try @rust-timer queue

rust-timer · 2020-10-02T12:54:12Z

Awaiting bors try build completion

bors · 2020-10-02T12:54:23Z

⌛ Trying commit 638a29f7e10dae8d154fc4c7624ed8b0ea33df8a with merge be5d6894699da44f42b10af9e2afd5a423ca67d0...

RalfJung · 2020-10-02T13:30:14Z

compiler/rustc_mir_build/src/thir/pattern/_match.rs

-    /// Literal values.
-    ConstantValue(&'tcx ty::Const<'tcx>),
+    /// String literals
+    Str(&'tcx ty::Const<'tcx>),


Okay so this type is indeed only used for exhaustiveness checking, that's why we do not need consts in here? Maybe add that to the type doccomment.

Why are strings special here?

bors · 2020-10-02T13:37:35Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: be5d6894699da44f42b10af9e2afd5a423ca67d0 (be5d6894699da44f42b10af9e2afd5a423ca67d0)

rust-timer · 2020-10-02T13:37:37Z

Queued be5d6894699da44f42b10af9e2afd5a423ca67d0 with parent 154f1f5, future comparison URL.

… just treat them as nonexhaustive

rust-timer · 2020-10-02T17:51:53Z

Finished benchmarking try commit (be5d6894699da44f42b10af9e2afd5a423ca67d0): 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

varkor · 2020-10-10T14:20:30Z

Once there's a comment explaining why strings are special, this should be good.
cc @Nadrieril, who might have comments.

RalfJung · 2020-10-10T14:33:33Z

Once there's a comment explaining why strings are special, this should be good.
cc @Nadrieril, who might have comments.

I hope Oli can also add a comment clarifying this. :)

Nadrieril · 2020-10-12T15:27:22Z

Omg yes you removed slice_pat_covered_by_const! I hated that function. I'm happy about this, const handling was a major source of complexity.
If I understand correctly, what you're doing is removing code that is now dead since #70743? I haven't read that PR in enough detail to check if all the code you're removing is inaccessible.

Nadrieril · 2020-10-12T15:30:11Z

compiler/rustc_mir_build/src/thir/pattern/_match.rs

-        ty::Bool => {
-            [true, false].iter().map(|&b| ConstantValue(ty::Const::from_bool(cx.tcx, b))).collect()
-        }
+        ty::Bool => vec![make_range(0, 1)],
        ty::Array(ref sub_ty, len) if len.try_eval_usize(cx.tcx, cx.param_env).is_some() => {


Doesn't this affect diagnostics? I'd expect this would display "pattern 0 not covered" instead of "pattern false not covered", but that would clearly be caught by the tests. I don't understand what piece of code makes it so it doesn't.

I had also wondered this, but assumed this would be caught by tests, so it must have been okay. But you're right that it would be better to understand this (and add a comment).

This is because while decisions are made on Constructors, they do not directly flow into diagnostics. Instead diagnostics are reported on Pats in https://github.com/rust-lang/rust/blob/6553d38bcb0c9193808ae93db8bd95bcf4c4824a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs

oli-obk · 2020-10-12T15:30:57Z

If I understand correctly, what you're doing is removing code that is now dead since #70743? I haven't read that PR in enough detail to check if all the code you're removing is inaccessible.

Well, I also made a small change here (&[u8] and &[u8; N] constants get converted into slice patterns, thus leaving no constants but &str or opaque constants as PatKind::Const). This allowed all of that code to become dead, but parts of it were already dead after #70743

compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs

varkor · 2020-10-12T15:40:44Z

I'm concerned by #77390 (comment). I think we should delay merging this until @Nadrieril's concerns have been addressed.

@bors r-

Nadrieril · 2020-10-12T15:44:42Z

Well, I also made a small change here (&[u8] and &[u8; N] constants get converted into slice patterns, thus leaving no constants but &str or opaque constants as PatKind::Const). This allowed all of that code to become dead, but parts of it were already dead after #70743

Wait, integer and float ranges are still PatKind::Const, right?

oli-obk · 2020-10-12T15:46:12Z

Wait, integer and float ranges are still PatKind::Const, right?

hmm yes, oops, forgot about those. Booleans and chars, too.

Nadrieril · 2020-10-12T15:48:42Z

compiler/rustc_mir_build/src/thir/pattern/mod.rs

@@ -158,6 +158,9 @@ crate enum PatKind<'tcx> {
        subpattern: Pat<'tcx>,
    },

+    /// Opaque constant. This will be treated as a single, but unknown value.
+    /// Except for `&str`, which will be handled as a string pattern and thus exhaustiveness
+    /// checking will detect if you use the same string twice in different patterns.
    Constant {


This comment should also mention int ranges etc then

RalfJung · 2020-10-12T15:50:28Z

I'm concerned by #77390 (comment). I think we should delay merging this until @Nadrieril's concerns have been addressed.

CI is already running, so you also need to
@bors retry

oli-obk · 2020-10-15T14:33:36Z

@Nadrieril can you help me with the latest commit? I have absolutely no clue how to create a Constructor::Opaque that will work. Everything I read and tested led me to believe the last commit should work, but instead it still ICEs the example I showed you and treats raw pointer patterns like wildcard patterns

Nadrieril · 2020-10-16T00:52:28Z

So, your example ICEs in stable (playground link), so that's probably unrelated to the current PR.
Then, if I remove the double ref it doesn't ICE but emits an "unreachable pattern" warning that's clearly incorrect, again on stable (playground link).
So the problems may not come from you getting the Opaque constructor wrong. I'll have a look tomorrow or this week-end and see if I understand what's happening

Nadrieril · 2020-10-17T18:59:55Z

I'm pretty sure I found how to fix one of the bugs: see #78057 . I'm not familiar enough with const-to-pat.rs to fix it myself.
I'm still going through the PR to try to make Opaque work

Nadrieril · 2020-10-17T23:55:05Z

I was struggling to realize why I couldn't make Opaque work, and I've found why: if I replace every construction of Opaque with a panic!(), it appears that it's not constructed at all for most of the cases I was testing! In fact const_to_pat expands constants to PatKind::Wild in many error cases like

rust/compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs

Lines 337 to 348 in 6af9846

    
                           let msg = format!( 
        
                               "to use a constant of type `{}` in a pattern, \ 
        
                                `{}` must be annotated with `#[derive(PartialEq, Eq)]`", 
        
                               path, path, 
        
                           ); 
        
                           self.saw_const_match_error.set(true); 
        
                           if self.include_lint_checks { 
        
                               tcx.sess.span_err(span, &msg); 
        
                           } else { 
        
                               tcx.sess.delay_span_bug(span, &msg) 
        
                           } 
        
                           PatKind::Wild

I don't like these wildcards. Wrt _match.rs, I'd like to emit PatKind::Constant instead. Would that break anything? Who else uses the output of const_to_pat?
The alternative is that I ignore those cases since they are errors anyways.

EDIT: this breaks things. This is not very important, I'll leave further discussion for #78057

Nadrieril · 2020-10-18T13:27:39Z

@oli-obk I fixed Opaque! I submitted #78072 that contains your changes plus mine.
I also filed two issues #78071 and #78057 from the discussion we had here. They are not regressions nor directly related to the current PR so I thought we'd deal with them separately.

oli-obk · 2020-10-20T09:56:43Z

closing in favour of #78072

Thanks so much!

…, r=varkor Cleanup constant matching in exhaustiveness checking This supercedes rust-lang#77390. I made the `Opaque` constructor work. I have opened two issues rust-lang#78071 and rust-lang#78057 from the discussion we had on the previous PR. They are not regressions nor directly related to the current PR so I thought we'd deal with them separately. I left a FIXME somewhere because I didn't know how to compare string constants for equality. There might even be some unicode things that need to happen there. In the meantime I preserved previous behavior. EDIT: I accidentally fixed rust-lang#78071

oli-obk added 2 commits September 29, 2020 18:28

Destructure byte array constants to array patterns instead of keeping…

002a1c2

… them opaque

Destructure byte slices and remove all the workarounds

14a0b10

rust-highfive assigned varkor Oct 1, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2020

oli-obk mentioned this pull request Oct 2, 2020

Refactorings in preparation for const value trees #77227

Merged

RalfJung reviewed Oct 2, 2020

View reviewed changes

compiler/rustc_mir_build/src/thir/pattern/_match.rs Show resolved Hide resolved

RalfJung reviewed Oct 2, 2020

View reviewed changes

Treat booleans as integers with valid range 0..=1

e0e48ac

RalfJung reviewed Oct 2, 2020

View reviewed changes

Stop passing opaque constants around in the exhaustiveness checks and…

523b752

… just treat them as nonexhaustive

oli-obk force-pushed the pat_opt branch from 638a29f to 523b752 Compare October 2, 2020 14:50

Document new (or new uses) of enum variants

6553d38

Nadrieril reviewed Oct 12, 2020

View reviewed changes

compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs Outdated Show resolved Hide resolved

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 12, 2020

Nadrieril reviewed Oct 12, 2020

View reviewed changes

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 12, 2020

oli-obk removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 12, 2020

oli added 2 commits October 15, 2020 14:30

Update comment

d0bb77d

WIP: Add opaque constructor

0915063

Nadrieril mentioned this pull request Oct 17, 2020

Incorrect "unreachable pattern" warning with opaque constant #78057

Closed

Nadrieril mentioned this pull request Oct 18, 2020

Cleanup constant matching in exhaustiveness checking #78072

Merged

oli-obk closed this Oct 20, 2020

Nadrieril mentioned this pull request Oct 27, 2020

Rollup of 12 pull requests #78334

Merged

Nadrieril mentioned this pull request Nov 14, 2020

Incorrect "unreachable pattern" warning on nightly #79048

Closed

oli-obk deleted the pat_opt branch March 16, 2021 12:14

Fully destructure slice and array constants into patterns #77390

Fully destructure slice and array constants into patterns #77390

Conversation

oli-obk commented Oct 1, 2020

rust-highfive commented Oct 1, 2020

oli-obk commented Oct 1, 2020

rust-timer commented Oct 1, 2020

bors commented Oct 1, 2020

bors commented Oct 1, 2020

rust-timer commented Oct 1, 2020

rust-timer commented Oct 1, 2020

oli-obk commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Oct 2, 2020

rust-timer commented Oct 2, 2020

bors commented Oct 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 2, 2020

rust-timer commented Oct 2, 2020

rust-timer commented Oct 2, 2020

varkor commented Oct 10, 2020

RalfJung commented Oct 10, 2020

Nadrieril commented Oct 12, 2020

Nadrieril Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Oct 12, 2020

varkor commented Oct 12, 2020

Nadrieril commented Oct 12, 2020

oli-obk commented Oct 12, 2020

Nadrieril Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Oct 12, 2020

oli-obk commented Oct 15, 2020

Nadrieril commented Oct 16, 2020

Nadrieril commented Oct 17, 2020 • edited Loading

Nadrieril commented Oct 17, 2020 • edited Loading

Nadrieril commented Oct 18, 2020

oli-obk commented Oct 20, 2020

oli-obk Oct 5, 2020 •

edited

Loading

Nadrieril Oct 12, 2020 •

edited

Loading

Nadrieril Oct 12, 2020 •

edited

Loading

Nadrieril commented Oct 17, 2020 •

edited

Loading

Nadrieril commented Oct 17, 2020 •

edited

Loading