Skip to content

rustc_pattern_analysis: always check that deref patterns don't match on the same place as normal constructors #143472

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

Merged
merged 2 commits into from
Jul 9, 2025

Conversation

dianne
Copy link
Contributor

@dianne dianne commented Jul 5, 2025

In #140106, deref pattern validation was tied to the deref_patterns feature to temporarily avoid affecting perf. However:

  • As of remove special-casing of boxes from match exhaustiveness/usefulness analysis #143414, box patterns are represented as deref patterns in rustc_pattern_analysis. Since they can be used by enabling box_patterns instead of deref_patterns, it was possible for them to skip validation, resulting in an ICE. This fixes that and adds a regression test.
  • External tooling (e.g. rust-analyzer) will also need to validate matches containing deref patterns, which was not possible. This fixes that by making compute_match_usefulness validate deref patterns by default.

In order to avoid doing an extra pass for anything with patterns, the second commit makes RustcPatCtxt keep track of whether it encounters a deref pattern, so that it only does the check if so. This is purely for performance. If the perf impact of the first commit is negligible and the complexity cost introduced by the second commit is significant, it may be worth dropping the latter.

r? @Nadrieril

@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2025

Nadrieril is currently at their maximum review capacity.
They may take a while to respond.

@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 Jul 5, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 5, 2025

Some changes occurred in exhaustiveness checking

cc @Nadrieril

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in match checking

cc @Nadrieril

@rust-log-analyzer

This comment has been minimized.

@Nadrieril
Copy link
Member

Nadrieril commented Jul 5, 2025

Do you want to try keeping just the first commit so we can do a perf run? Mostly out of curiosity, second commit looks good to me.

@dianne
Copy link
Contributor Author

dianne commented Jul 5, 2025

I'm a little curious too; is it possible to do a try build just for the first one? But if the second commit is fine, I don't see a reason not to keep the improvement! May as well not burden the build/perf infra.

@Nadrieril
Copy link
Member

Fair, let's merge then :)

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Jul 9, 2025

📌 Commit bb64315 has been approved by Nadrieril

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2025
@bors
Copy link
Collaborator

bors commented Jul 9, 2025

⌛ Testing commit bb64315 with merge 6b3ae3f...

@bors
Copy link
Collaborator

bors commented Jul 9, 2025

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 6b3ae3f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 9, 2025
@bors bors merged commit 6b3ae3f into rust-lang:master Jul 9, 2025
12 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 9, 2025
Copy link
Contributor

github-actions bot commented Jul 9, 2025

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 558d253 (parent) -> 6b3ae3f (this PR)

Test differences

Show 12 test diffs

Stage 1

  • [ui] tests/ui/pattern/box-pattern-constructor-mismatch.rs: [missing] -> pass (J1)

Stage 2

  • [ui] tests/ui/pattern/box-pattern-constructor-mismatch.rs: [missing] -> pass (J0)

Additionally, 10 doctest diffs were found. These are ignored, as they are noisy.

Job group index

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 6b3ae3f6e45a33c2d95fa0362c9b2593e567fd34 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-aarch64-linux: 5756.8s -> 8089.7s (40.5%)
  2. dist-apple-various: 8204.4s -> 5274.3s (-35.7%)
  3. dist-x86_64-apple: 6877.3s -> 8566.4s (24.6%)
  4. aarch64-apple: 3952.3s -> 4813.4s (21.8%)
  5. x86_64-apple-2: 4570.7s -> 3665.4s (-19.8%)
  6. x86_64-apple-1: 7960.2s -> 6560.1s (-17.6%)
  7. tidy: 71.5s -> 65.6s (-8.3%)
  8. x86_64-gnu-llvm-19-1: 3310.1s -> 3545.8s (7.1%)
  9. dist-various-1: 3955.3s -> 3697.7s (-6.5%)
  10. x86_64-msvc-1: 8356.3s -> 8853.8s (6.0%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

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.

5 participants