Fully destructure constants into patterns #70743
Conversation
rust-highfive
added
the
S-waiting-on-review
src/test/ui/pattern/usefulness/match-byte-array-patterns-2.stderr
Outdated
Show resolved
Hide resolved
| @@ -20,8 +20,6 @@ pub fn main() { | |||
| assert_eq!(y, 2); | |||
| let z = match &() { | |||
| ZST => 9, | |||
| // FIXME: this should not be required | |||
| _ => 42, | |||
There was a problem hiding this comment.
Sigh, this should be required, we have an RFC about this.
@nikomatsakis implemented the #[structural_match] part of the RFC, but never got to the "const patterns are opaque for exhaustiveness checking" part, so #31434 is still open.
There was a problem hiding this comment.
uh... I didn't know about this RFC (edit: lol I commented on it four years ago), but as the RFC states, this is not backwards compatible, and we already break this rule for constants of struct and enum type in arbitrary depth... I don't know how we could ever do this breaking change, or even how to start linting about it.
There was a problem hiding this comment.
anyway, I'll read up on the RFC and issue and open a discussion on zulip or sth. I guess this PR is blocked until then.
There was a problem hiding this comment.
It's probably just not plausible at this juncture, and maybe never was. Honestly at this point I'd really like to settle on "some answer" regarding the semantics of constants in match patterns and not try to maintain the "structural eq limbo" we've been living in. It'd be good to collect some notes on the current status and maybe have a lang team design meeting to walk over them?
There was a problem hiding this comment.
looks to me like the PR no longer changes this test, and so it seems like this question is ... if not resolved, then at least postponed for the time being? Can I mark this as "resolved"?
There was a problem hiding this comment.
Note that
const FOO: () = ();
match () {
FOO => {},
_ => {},
}complains about the _ arm on stable and if you remove it it compiles fine. The only change this PR does (and still does) is to give the same feature to constants of reference type.
There was a problem hiding this comment.
IOW, on top of str and [u8] we also do exhaustiveness checking for constants of type ()? What about integers?
Cc rust-lang/const-eval#42 -- Currently I'd prefer not to expand the set of types where consts are subject to exhaustiveness checking, in particular not when references are involved.
There was a problem hiding this comment.
integers do exhaustiveness checks. so do structs without private fields and enums.
There was a problem hiding this comment.
Honestly at this point I'd really like to settle on "some answer" regarding the semantics of constants in match patterns and not try to maintain the "structural eq limbo" we've been living in. It'd be good to collect some notes on the current status and maybe have a lang team design meeting to walk over them?
I cannot say much about the current status (the code is hard to follow and scarcely documented) but I have given some thought recently to what structural equality means and how it affects pattern matching. See #74446 and this hackmd.
|
The job Click to expand the log.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 |
|
This probably fixes several issues, including #53708. |
|
cc @Nadrieril |
Are there known changes to semantics to be concerned about? |
I'm superstitious about our match code because I have failed to grok various consequences of my changes several times before. This PR should strictly be an improvement in diagnostics and thus only change what lints are being emitted. |
oli-obk
force-pushed
the
eager_const_to_pat_conversion
branch
from
8eb57fd
to
df031ec
Compare
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
eager_const_to_pat_conversion
branch
from
250a4e2
to
db4de75
Compare
joelpalmer
added
S-waiting-on-review
|
r? @eddyb |
crlf0710
removed
the
S-waiting-on-review
bors
added
S-waiting-on-bors
|
@bors r=eddyb |
|
📌 Commit daf976f has been approved by |
|
☀️ Test successful - checks-actions, checks-azure |
This issue was accidentally fixed recently, probably by rust-lang#70743
Always fall back to PartialEq when a constant in a pattern is not recursively structural-eq Right now we destructure the constant as far as we can, but with this PR we just don't take it apart anymore. This is preparatory work for moving to always using valtrees, as these will just do a single conversion of the constant to a valtree at the start, and if that fails, fall back to `PartialEq`. This removes a few cases where we emitted the `unreachable pattern` lint, because we stop looking into the constant deeply enough to detect that a constant is already covered by another pattern. Previous work: rust-lang#70743 This is groundwork towards fixing rust-lang#83085 and rust-lang#105047
Always fall back to PartialEq when a constant in a pattern is not recursively structural-eq Right now we destructure the constant as far as we can, but with this PR we just don't take it apart anymore. This is preparatory work for moving to always using valtrees, as these will just do a single conversion of the constant to a valtree at the start, and if that fails, fall back to `PartialEq`. This removes a few cases where we emitted the `unreachable pattern` lint, because we stop looking into the constant deeply enough to detect that a constant is already covered by another pattern. Previous work: rust-lang/rust#70743 This is groundwork towards fixing rust-lang/rust#83085 and rust-lang/rust#105047
r? @varkor
as discussed in https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/constants.20in.20patterns/near/192789924
we should probably crater it once reviewed