Skip to content

fix(query): Pass Query Key to value_from_cycle_error#153694

Open
TKanX wants to merge 2 commits intorust-lang:mainfrom
TKanX:bugfix/153391-cycle-error-key-param
Open

fix(query): Pass Query Key to value_from_cycle_error#153694
TKanX wants to merge 2 commits intorust-lang:mainfrom
TKanX:bugfix/153391-cycle-error-key-param

Conversation

@TKanX
Copy link
Contributor

@TKanX TKanX commented Mar 11, 2026 •
edited
Loading

Summary:

Pass the query key directly to value_from_cycle_error so that FromCycleError impls (notably FnSig) can use the recovered query's DefId instead of relying on cycle[0], which is arbitrarily rotated by the parallel deadlock handler.

As suggested in #153644 (comment).

Closes #153391

r? @nnethercote
cc @zetanumbers

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 11, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2026

Failed to set assignee to zetanumbers: invalid assignee

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.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@TKanX TKanX force-pushed the bugfix/153391-cycle-error-key-param branch from c1f26e9 to 6118290 Compare March 11, 2026 07:05
@TKanX TKanX marked this pull request as ready for review March 11, 2026 08:13
@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 Mar 11, 2026
@rustbot

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

This needs a rebase, sorry for an inconvenience. You are a half way there, but you would need to remove QueryKey::key_as_def_id call you have added and use query key directly to extract the function's arity within rustc_query_impl::from_cycle_error::fn_sig.

@zetanumbers zetanumbers mentioned this pull request Mar 11, 2026
Merged
@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

@rustbot author

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@TKanX TKanX marked this pull request as draft March 11, 2026 09:29
@TKanX TKanX force-pushed the bugfix/153391-cycle-error-key-param branch from 6118290 to 5986684 Compare March 11, 2026 09:56
@TKanX TKanX marked this pull request as ready for review March 11, 2026 09:57
@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 Mar 11, 2026
@rustbot

This comment has been minimized.

Sign in to view
@nnethercote
Copy link
Contributor

nnethercote commented Mar 11, 2026

FYI: #153707 is also changing this code, replacing the guar parameter with err. It is compatible with this change, but whichever PR merges first will cause conflicts with the other. Apologies for any confusion, I didn't realize @zetanumbers was handing out pieces of this work to other people.

@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

I had done some recruitment into query system. I thought it was clear from #153493 (comment)

@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

This PR also fixes #142064 as we no longer rely on a reported query cycle and whatever permutation it has, which was the source of that problem.

@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

LGTM

@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

@bors ready

If only I could've

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 11, 2026

Unknown command "ready". Run @bors help to see available commands.

zetanumbers added a commit to zetanumbers/rust that referenced this pull request Mar 11, 2026
@zetanumbers
Cleanup from_cycle_error based on rust-lang#153694
b597c04
@zetanumbers zetanumbers mentioned this pull request Mar 11, 2026
Open
@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

This would need a small rebase after #153639 merges.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nnethercote
Copy link
Contributor

nnethercote commented Mar 11, 2026

It's a challenging time to start working on the query system, BTW. There is a lot of work going on, by multiple people. Lots of PRs happening, and conflicts needing to be fixed. Hopefully you can work things out!

zetanumbers
zetanumbers reviewed Mar 11, 2026
View reviewed changes
@zetanumbers
Copy link
Contributor

zetanumbers commented Mar 11, 2026

@rustbot author

@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-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2026
@TKanX TKanX marked this pull request as draft March 11, 2026 17:45
@TKanX @zetanumbers
test(parallel): Add regression test for rust-lang#153391
d75b836 
Co-authored-by: Daria Sukhonina <dariasukhonina@gmail.com>
@TKanX TKanX force-pushed the bugfix/153391-cycle-error-key-param branch from 5986684 to d75b836 Compare March 11, 2026 17:48
@TKanX TKanX marked this pull request as ready for review March 11, 2026 17:48
@TKanX TKanX requested a review from zetanumbers March 11, 2026 17:48
@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 Mar 11, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2026

⚠️ Warning ⚠️

  • There are issue links (such as #123) in the commit messages of the following commits.
    Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.

nnethercote
nnethercote reviewed Mar 11, 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.

The first commit seems fine, just a couple of nits to fix below.

I'm less certain about the test because I don't know much about the state of parallel tests. I hope there won't be any problems with non-determinism? I will defer to @zetanumbers on that.

I'm happy to let this merge before #153707 because this is smaller and fixing the conflicts will probably be easier for me.

View changes since this review

compiler/rustc_query_impl/src/from_cycle_error.rs

vtables.check_representability.value_from_cycle_error =
|tcx, cycle, guar| check_representability(tcx, cycle, guar);
|tcx, _key, cycle, guar| check_representability(tcx, cycle, guar);
Copy link
Contributor

@nnethercote nnethercote Mar 11, 2026

Choose a reason for hiding this comment

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

The cases above use _ to ignore the key and other arguments. Do the same here for consistency.

compiler/rustc_query_impl/src/plumbing.rs
is_loadable_from_disk_fn: |_tcx, _key, _index| false,

value_from_cycle_error: |tcx, cycle, _| {
value_from_cycle_error: |tcx, _key, cycle, _| {
Copy link
Contributor

@nnethercote nnethercote Mar 11, 2026

Choose a reason for hiding this comment

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

ditto

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

Reviewers

@nnethercote nnethercote nnethercote left review comments

@zetanumbers zetanumbers Awaiting requested review from zetanumbers

Assignees

@nnethercote nnethercote

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ICE]: parallel: None in compiler/rustc_type_ir/src/ty_kind.rs

5 participants

@TKanX @rustbot @rust-log-analyzer @zetanumbers @nnethercote