Skip to content

Remove def_id_for_ty_in_cycle#153867

Merged
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Zoxc:rem-def_id_for_ty_in_cycle
Mar 15, 2026
Merged

Remove def_id_for_ty_in_cycle#153867
rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Zoxc:rem-def_id_for_ty_in_cycle

Conversation

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 14, 2026

This removes QueryKey::def_id_for_ty_in_cycle and QueryStackFrame.def_id_for_ty_in_cycle by computing it instead from TaggedQueryKey.

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 Mar 14, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2026

r? @dingxiangfei2009

rustbot has assigned @dingxiangfei2009.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler, query-system
  • compiler, query-system expanded to 69 candidates
  • Random selection from 15 candidates

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 14, 2026

cc @nnethercote @Zalathar

nnethercote
nnethercote approved these changes Mar 14, 2026
View reviewed changes
Copy link
Contributor

@nnethercote nnethercote left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

I had seen this field and thought it seemed very specific. Nice to see it removed.

View changes since this review

compiler/rustc_query_impl/src/from_cycle_error.rs
let diag = search_for_cycle_permutation(
&cycle_error.cycle,
|cycle| {
if cycle[0].frame.dep_kind == DepKind::layout_of
Copy link
Contributor

@nnethercote nnethercote Mar 14, 2026

Choose a reason for hiding this comment

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

This makes me wonder if QueryStackFrame needs dep_kind when it also has tagged_key.

Copy link
Member

@Zalathar Zalathar Mar 15, 2026

Choose a reason for hiding this comment

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

Yeah, it should be fairly straightforward to reconstruct a DepKind from a TaggedQueryKey if one is needed.

compiler/rustc_query_impl/src/from_cycle_error.rs
if info.frame.dep_kind == DepKind::check_representability_adt_ty
&& let Some(def_id) = info.frame.def_id_for_ty_in_cycle
&& let Some(def_id) = def_id.as_local()
if let TaggedQueryKey::check_representability_adt_ty(key) = info.frame.tagged_key
Copy link
Contributor

@nnethercote nnethercote Mar 14, 2026

Choose a reason for hiding this comment

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

This took me a minute to parse because of the snake_case enum variant name. Likewise for the layout_of ones below. Seems hard to avoid, though.

@nnethercote
Copy link
Contributor

nnethercote commented Mar 14, 2026

@bors r+ rollup

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 14, 2026

📌 Commit 1a30924 has been approved by nnethercote

It is now in the queue for this repository.

@rust-bors rust-bors bot 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 Mar 14, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 15, 2026

☀️ Test successful - CI
Approved by: nnethercote
Duration: 3h 25m 10s
Pushing 9f0615c to main...

@rust-bors rust-bors bot added merged-by-bors This PR was explicitly merged by bors. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 15, 2026
@rust-bors rust-bors bot merged commit 9f0615c into rust-lang:main Mar 15, 2026
12 checks passed
@rustbot rustbot added this to the 1.96.0 milestone Mar 15, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2026

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 03749d6 (parent) -> 9f0615c (this PR)

Test differences

Show 2 test diffs

2 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 9f0615c4301206eeba76468f78d34876c4a2997e --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: 1h 29m -> 1h 48m (+20.5%)
  2. dist-aarch64-llvm-mingw: 1h 46m -> 1h 25m (-19.2%)
  3. dist-aarch64-apple: 1h 43m -> 2h (+16.2%)
  4. x86_64-gnu-tools: 54m 41s -> 1h 2m (+13.8%)
  5. x86_64-gnu-gcc: 1h 1m -> 1h 9m (+12.9%)
  6. optional-x86_64-gnu-parallel-frontend: 2h 19m -> 2h 37m (+12.5%)
  7. x86_64-rust-for-linux: 44m 48s -> 50m 22s (+12.4%)
  8. aarch64-apple: 2h 56m -> 3h 18m (+12.3%)
  9. aarch64-msvc-1: 1h 53m -> 2h 6m (+12.2%)
  10. aarch64-gnu-llvm-21-1: 54m 1s -> 1h (+11.6%)
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

rust-timer commented Mar 15, 2026

Finished benchmarking commit (9f0615c): 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

Results (secondary -2.2%)

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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.2% [-2.2%, -2.2%] 1
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 482.982s -> 484.991s (0.42%)
Artifact size: 396.82 MiB -> 396.79 MiB (-0.01%)

@Zalathar Zalathar mentioned this pull request Mar 15, 2026
Merged
@Zoxc Zoxc deleted the rem-def_id_for_ty_in_cycle branch March 15, 2026 07:42
Zalathar added a commit to Zalathar/rust that referenced this pull request Mar 17, 2026
@Zalathar
Rollup merge of rust-lang#153907 - Zalathar:stack-frame, r=nnethercote
deb6627 
Remove redundant fields from `QueryStackFrame`

- Follow-up to rust-lang#153867 (comment)
---

Now that QueryStackFrame holds a TaggedQueryKey, it turns out that all of its other fields are redundant and can be removed:

- An explicit dep-kind is redundant here. If we want to find the query name, or check for specific queries, we can inspect the `TaggedQueryKey` instead.
- Similarly, in cases where the key is some kind of `DepId`, we can extract it directly from the `TaggedQueryKey`.

r? nnethercote
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zalathar Zalathar Zalathar left review comments

@nnethercote nnethercote nnethercote approved these changes

Assignees

@dingxiangfei2009 dingxiangfei2009

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) merged-by-bors This PR was explicitly merged by bors. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.96.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Zoxc @rustbot @nnethercote @rust-timer @Zalathar @dingxiangfei2009