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

Fully destructure constants into patterns #70743

Merged
merged 21 commits into from
Sep 26, 2020

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 3, 2020

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2020
@@ -20,8 +20,6 @@ pub fn main() {
assert_eq!(y, 2);
let z = match &() {
ZST => 9,
// FIXME: this should not be required
_ => 42,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@oli-obk oli-obk Apr 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

integers do exhaustiveness checks. so do structs without private fields and enums.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikomatsakis

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.

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, 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.
2020-04-03T17:26:40.5612343Z ========================== Starting Command Output ===========================
2020-04-03T17:26:40.5615526Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/23a5e3fe-dbee-463d-a56b-4f3108ad32a7.sh
2020-04-03T17:26:40.5615874Z 
2020-04-03T17:26:40.5619597Z ##[section]Finishing: Disable git automatic line ending conversion
2020-04-03T17:26:40.5642642Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70743/merge to s
2020-04-03T17:26:40.5645862Z Task         : Get sources
2020-04-03T17:26:40.5646147Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-03T17:26:40.5646632Z Version      : 1.0.0
2020-04-03T17:26:40.5646888Z Author       : Microsoft
---
2020-04-03T17:26:41.7571316Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-04-03T17:26:41.7583842Z ##[command]git config gc.auto 0
2020-04-03T17:26:41.7589116Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-04-03T17:26:41.7592591Z ##[command]git config --get-all http.proxy
2020-04-03T17:26:41.7600099Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/70743/merge:refs/remotes/pull/70743/merge
---
2020-04-03T17:28:29.1465457Z Looks like docker image is the same as before, not uploading
2020-04-03T17:28:36.7994395Z [CI_JOB_NAME=x86_64-gnu-llvm-7]
2020-04-03T17:28:36.8293756Z [CI_JOB_NAME=x86_64-gnu-llvm-7]
2020-04-03T17:28:36.8323517Z == clock drift check ==
2020-04-03T17:28:36.8346297Z   local time: Fri Apr  3 17:28:36 UTC 2020
2020-04-03T17:28:36.9088326Z   network time: Fri, 03 Apr 2020 17:28:36 GMT
2020-04-03T17:28:36.9115053Z Starting sccache server...
2020-04-03T17:28:36.9985764Z configure: processing command line
2020-04-03T17:28:36.9987615Z configure: 
2020-04-03T17:28:36.9988963Z configure: rust.dist-src        := False
---
2020-04-03T17:34:07.0323970Z    Compiling rustc_feature v0.0.0 (/checkout/src/librustc_feature)
2020-04-03T17:34:08.6971052Z    Compiling fmt_macros v0.0.0 (/checkout/src/libfmt_macros)
2020-04-03T17:34:10.4411934Z    Compiling rustc_ast_pretty v0.0.0 (/checkout/src/librustc_ast_pretty)
2020-04-03T17:34:12.5343966Z    Compiling rustc_hir v0.0.0 (/checkout/src/librustc_hir)
2020-04-03T17:34:21.5849970Z    Compiling rustc_query_system v0.0.0 (/checkout/src/librustc_query_system)
2020-04-03T17:34:24.9748819Z    Compiling rustc_hir_pretty v0.0.0 (/checkout/src/librustc_hir_pretty)
2020-04-03T17:34:29.8941193Z    Compiling rustc_attr v0.0.0 (/checkout/src/librustc_attr)
2020-04-03T17:34:34.4436961Z    Compiling rustc_parse v0.0.0 (/checkout/src/librustc_parse)
2020-04-03T17:34:44.1139123Z    Compiling rustc_ast_lowering v0.0.0 (/checkout/src/librustc_ast_lowering)
---
2020-04-03T17:56:32.4343552Z    Compiling rustc_index v0.0.0 (/checkout/src/librustc_index)
2020-04-03T17:56:37.5798749Z error: failed to run custom build command for `backtrace-sys v0.1.35`
2020-04-03T17:56:37.5799279Z 
2020-04-03T17:56:37.5799424Z Caused by:
2020-04-03T17:56:37.5819851Z   process didn't exit successfully: `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-rustc/release/build/backtrace-sys-716460570fac9848/build-script-build` (exit code: 1)
2020-04-03T17:56:37.5820559Z --- stdout
2020-04-03T17:56:37.5820918Z cargo:rustc-cfg=rbt
2020-04-03T17:56:37.5821336Z --- stderr
2020-04-03T17:56:37.5821456Z 
2020-04-03T17:56:37.5821556Z 
2020-04-03T17:56:37.5821793Z error occurred: Getting object file details failed.
---
2020-04-03T17:56:44.0871613Z expected success, got: exit code: 101
2020-04-03T17:56:44.0880211Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --exclude src/tools/tidy
2020-04-03T17:56:44.0880642Z Build completed unsuccessfully in 0:26:28
2020-04-03T17:56:44.0930853Z == clock drift check ==
2020-04-03T17:56:44.0951528Z   local time: Fri Apr  3 17:56:44 UTC 2020
2020-04-03T17:56:44.3667275Z   network time: Fri, 03 Apr 2020 17:56:44 GMT
2020-04-03T17:56:45.2622873Z 
2020-04-03T17:56:45.2622873Z 
2020-04-03T17:56:45.2700396Z ##[error]Bash exited with code '1'.
2020-04-03T17:56:45.2716997Z ##[section]Finishing: Run build
2020-04-03T17:56:45.2771464Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/70743/merge to s
2020-04-03T17:56:45.2777225Z Task         : Get sources
2020-04-03T17:56:45.2777604Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-04-03T17:56:45.2777948Z Version      : 1.0.0
2020-04-03T17:56:45.2778205Z Author       : Microsoft
2020-04-03T17:56:45.2778205Z Author       : Microsoft
2020-04-03T17:56:45.2778590Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-04-03T17:56:45.2779035Z ==============================================================================
2020-04-03T17:56:45.6434723Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-04-03T17:56:45.6484479Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/70743/merge to s
2020-04-03T17:56:45.6581172Z Cleaning up task key
2020-04-03T17:56:45.6582567Z Start cleaning up orphan processes.
2020-04-03T17:56:45.6784268Z Terminate orphan process: pid (4106) (python)
2020-04-03T17:56:45.6976762Z ##[section]Finishing: Finalize Job

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 @rust-lang/infra. (Feature Requests)

@varkor
Copy link
Member

varkor commented Apr 3, 2020

This probably fixes several issues, including #53708.

@Centril
Copy link
Contributor

Centril commented Apr 3, 2020

cc @Nadrieril

@nikomatsakis
Copy link
Contributor

@oli-obk

we should probably crater it once reviewed

Are there known changes to semantics to be concerned about?

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 6, 2020

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 oli-obk force-pushed the eager_const_to_pat_conversion branch from 8eb57fd to df031ec Compare April 6, 2020 17:10
@rust-highfive

This comment has been minimized.

@oli-obk oli-obk force-pushed the eager_const_to_pat_conversion branch from 250a4e2 to db4de75 Compare April 8, 2020 10:29
@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 16, 2020
@Dylan-DPC-zz
Copy link

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned varkor Apr 16, 2020
@crlf0710 crlf0710 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2020
@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 Sep 24, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 24, 2020

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Sep 24, 2020

📌 Commit daf976f has been approved by eddyb

@bors
Copy link
Contributor

bors commented Sep 26, 2020

⌛ Testing commit daf976f with merge fd15e61...

@bors
Copy link
Contributor

bors commented Sep 26, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing fd15e61 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 26, 2020
@bors bors merged commit fd15e61 into rust-lang:master Sep 26, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 26, 2020
@oli-obk oli-obk deleted the eager_const_to_pat_conversion branch September 26, 2020 12:15
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Oct 21, 2020
This issue was accidentally fixed recently, probably by rust-lang#70743
bors added a commit to rust-lang-ci/rust that referenced this pull request May 16, 2023
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
RalfJung pushed a commit to RalfJung/miri that referenced this pull request May 18, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-patterns Relating to patterns and pattern matching 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.