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

Always fall back to PartialEq when a constant in a pattern is not recursively structural-eq #105750

Merged
merged 4 commits into from
May 16, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 15, 2022

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: #70743

This is groundwork towards fixing #83085 and #105047

@rustbot
Copy link
Collaborator

rustbot commented Dec 15, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 15, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 15, 2022

I can add the rest of the valtree work to this PR if preferred, but I thought I'd submit it in this small form first.

@pnkfelix
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 16, 2022
@bors
Copy link
Contributor

bors commented Dec 16, 2022

⌛ Trying commit ed0627422a3a653595718616513ce5b8a8aab8c6 with merge 73bd817a44e4fa3751ed828e99297caf55470e77...

@bors
Copy link
Contributor

bors commented Dec 16, 2022

☀️ Try build successful - checks-actions
Build commit: 73bd817a44e4fa3751ed828e99297caf55470e77 (73bd817a44e4fa3751ed828e99297caf55470e77)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (73bd817a44e4fa3751ed828e99297caf55470e77): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.3% [-2.3%, -2.3%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.3% [-2.3%, -2.3%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 16, 2022
@oli-obk oli-obk added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Dec 17, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 17, 2022

Blocked on making all function pointers PartialEq, even ones with higher ranked lifetimes,

@RalfJung
Copy link
Member

Blocked on making all function pointers PartialEq, even ones with higher ranked lifetimes,

So, basically #99531. It's not impossible but it seems hard.


If I understand correctly, this PR still uses the old "destructure const" machinery to build the pattern tree. Is the idea that after this PR, we can change that to use valtrees (and remove const destructuring) without further behavior changes?

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 21, 2022

Is the idea that after this PR, we can change that to use valtrees (and remove const destructuring) without further behavior changes?

Yes, I have tried this locally and it works and will permit more cleanups

@apiraino
Copy link
Contributor

Hello, checking status for this PR. Thanks for an update! cc @oli-obk @RalfJung

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 18, 2023

Still blocked on #99531

@jackh726 jackh726 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 28, 2023

@rustbot author

we got a FnPtr trait now that implements PartialEq for all function pointers

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 28, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 12, 2023
@oli-obk oli-obk closed this May 12, 2023
@oli-obk oli-obk reopened this May 12, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented May 12, 2023

@bors r-

@bors 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 May 12, 2023
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 16, 2023

@bors r=lcnr

@bors
Copy link
Contributor

bors commented May 16, 2023

📌 Commit 2282258 has been approved by lcnr

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 16, 2023
@bors
Copy link
Contributor

bors commented May 16, 2023

⌛ Testing commit 2282258 with merge 9239760...

@bors
Copy link
Contributor

bors commented May 16, 2023

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 9239760 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 16, 2023
@bors bors merged commit 9239760 into rust-lang:master May 16, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 16, 2023
@oli-obk oli-obk deleted the valtrees branch May 16, 2023 16:03
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9239760): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 641.949s -> 642.522s (0.09%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2023
thir::pattern: update some comments and error type names

Follow-up to [these comments](rust-lang#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 28, 2023
thir::pattern: update some comments and error type names

Follow-up to [these comments](rust-lang/rust#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
thir::pattern: update some comments and error type names

Follow-up to [these comments](rust-lang/rust#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
thir::pattern: update some comments and error type names

Follow-up to [these comments](rust-lang/rust#105750 (review)). Please carefully fact-check, I'm new to this area of the compiler!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.