Skip to content

Conversation

beepster4096
Copy link
Contributor

@beepster4096 beepster4096 commented Sep 26, 2025

I noticed that drop elaboration can't, in general, handle ProjectionElem::SubType. It creates a disjoint move path that overlaps with other move paths. (Subslice does too, and I'm working on a different PR to make that special case less fragile.) If its skipped and treated as the same move path as its parent then MovePath.place has multiple possible projections. (It would probably make sense to remove all Subtype projections for the canonical place but it doesn't make sense to have this special case for a problem that doesn't actually occur in real MIR.)

The only reason this doesn't break is that Subtype is always the sole projection of the local its applied to. For the same reason, it works fine as a CastKind so I figured that makes more sense than documenting and validating this hidden invariant.

cc #112651, #133258

r? Icnr (bc you've been the main person dealing with Subtype it looks like)

@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @vakaras

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to constck

cc @fee1-dead

@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 Sep 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2025

Failed to set assignee to Icnr: cannot assign: response: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/assignees#add-assignees-to-an-issue","status":"404"}

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@beepster4096
Copy link
Contributor Author

beepster4096 commented Sep 26, 2025

r? lcnr (sorry, I got your name wrong 😅)

@rust-log-analyzer

This comment has been minimized.

@beepster4096 beepster4096 changed the title Turn ProjectionKind::Subtype into CastKind::Subtype Turn ProjectionElem::Subtype into CastKind::Subtype Sep 26, 2025
@beepster4096 beepster4096 force-pushed the subtype_is_not_a_projection branch from 37be44c to 0132f5e Compare September 26, 2025 07:29
@rust-log-analyzer

This comment has been minimized.

@beepster4096 beepster4096 force-pushed the subtype_is_not_a_projection branch from 0132f5e to aa5a214 Compare September 26, 2025 08:27
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

pls fix clippy in CI, r=me after

View changes since this review

@lcnr lcnr 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 26, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot rustbot added the T-clippy Relevant to the Clippy team. label Sep 26, 2025
@rust-log-analyzer

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

Hooray? I am a little scared because I have no idea how I actually fixed this. Maybe I accidentally turned off a check or something and this should keep ICEing..

also slightly minimized the test
@rustbot
Copy link
Collaborator

rustbot commented Sep 26, 2025

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

@beepster4096
Copy link
Contributor Author

beepster4096 commented Sep 26, 2025

Nope, it's not fixed, it just requires -Zvalidate-mir now.

@beepster4096
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2025
let f: F = 0u32;

1
}],
Copy link
Contributor

Choose a reason for hiding this comment

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

that change seems odd, why remove the async blocks here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While investigating, I saw on the issue page that the async blocks are actually not part of the bug. I can split that into a different PR if you'd like.

@lcnr
Copy link
Contributor

lcnr commented Oct 1, 2025

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Oct 1, 2025

📌 Commit 413f095 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 1, 2025
@bors
Copy link
Collaborator

bors commented Oct 2, 2025

⌛ Testing commit 413f095 with merge 42b384e...

@bors
Copy link
Collaborator

bors commented Oct 2, 2025

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 2, 2025
@bors bors merged commit 42b384e into rust-lang:master Oct 2, 2025
11 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Oct 2, 2025
Copy link
Contributor

github-actions bot commented Oct 2, 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 3369e82 (parent) -> 42b384e (this PR)

Test differences

Show 16 test diffs

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

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 42b384ec0dfcd528d99a4db0a337d9188a9eecaa --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-apple-various: 3541.1s -> 4295.3s (21.3%)
  2. aarch64-apple: 9515.5s -> 7806.2s (-18.0%)
  3. dist-aarch64-msvc: 5631.1s -> 6621.4s (17.6%)
  4. i686-gnu-2: 5432.2s -> 6358.4s (17.0%)
  5. x86_64-gnu-nopt: 6845.9s -> 7854.8s (14.7%)
  6. aarch64-gnu-debug: 3872.6s -> 4408.7s (13.8%)
  7. i686-gnu-1: 7379.3s -> 8257.9s (11.9%)
  8. dist-various-1: 4158.9s -> 3667.0s (-11.8%)
  9. aarch64-msvc-2: 5289.0s -> 4702.4s (-11.1%)
  10. x86_64-gnu-miri: 4447.4s -> 4924.7s (10.7%)
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.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (42b384e): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (secondary 2.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.7%, 2.9%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results (primary 2.8%, secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.8% [2.8%, 2.8%] 1
Regressions ❌
(secondary)
2.5% [1.2%, 3.8%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.8% [2.8%, 2.8%] 1

Binary size

Results (primary 1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 472.027s -> 471.941s (-0.02%)
Artifact size: 387.76 MiB -> 387.76 MiB (0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Oct 2, 2025
tautschnig added a commit to tautschnig/kani that referenced this pull request Oct 3, 2025
Relevant upstream PR:
- rust-lang/rust#147055 (Turn ProjectionElem::Subtype
into CastKind::Subtype)

Resolves: model-checking#4392
github-merge-queue bot pushed a commit to model-checking/kani that referenced this pull request Oct 3, 2025
Relevant upstream PR:
- rust-lang/rust#147055 (Turn
ProjectionElem::Subtype into CastKind::Subtype)

Resolves: #4392

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 and MIT licenses.
@panstromek
Copy link
Contributor

perf triage:

clap_derive bimodal noise

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Oct 6, 2025
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 6, 2025
…tion, r=lcnr

Turn ProjectionElem::Subtype into CastKind::Subtype

I noticed that drop elaboration can't, in general, handle `ProjectionElem::SubType`. It creates a disjoint move path that overlaps with other move paths. (`Subslice` does too, and I'm working on a different PR to make that special case less fragile.) If its skipped and treated as the same move path as its parent then `MovePath.place` has multiple possible projections. (It would probably make sense to remove all `Subtype` projections for the canonical place but it doesn't make sense to have this special case for a problem that doesn't actually occur in real MIR.)

The only reason this doesn't break is that `Subtype` is always the sole projection of the local its applied to. For the same reason, it works fine as a `CastKind` so I figured that makes more sense than documenting and validating this hidden invariant.

cc rust-lang#112651, rust-lang#133258

r? Icnr (bc you've been the main person dealing with `Subtype` it looks like)
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. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-clippy Relevant to the Clippy team. 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.

7 participants