Add a cycle handler for `check_representability` by Zoxc · Pull Request #153470 · rust-lang/rust

Zoxc · 2026-03-06T06:26:17Z

This adds a cycle handler for check_representability to avoid its use of FromCycleError.

This is an alternative to #153028. We'd only have 2 - 3 handlers (currently) so that PR might be overkill. This PR just calls the handlers directly from mk_cycle instead of and the handlers would decide if they apply to a cycle.

I have kept the handler in from_cycle_error.rs to keep changes smaller, and we could rename it if/when the FromCycleError trait ends up being removed.

This is a step towards removing query cycle recovery as the FromCycleError trait would be removed for that.

rustbot · 2026-03-06T06:26:22Z

r? @jackh726

rustbot has assigned @jackh726.
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 16 candidates

nnethercote

This is heading in a good direction, I think. The custom handler calls in mk_cycle are a bit hacky but probably better than what we currently have.

Which other queries do you think would end up using this mechanism? The tricky thing is for some queries we just return a special value, for some we just print a custom error and abort, and for some we print a custom error and also return a special value.

View changes since this review

nnethercote · 2026-03-06T07:03:40Z

compiler/rustc_query_impl/src/from_cycle_error.rs

+        .is_some();
+    if !applies {
+        return;
+    }


Why is this new check needed?

This code manually checks if it handles representability query by checking if all queries in a cycle are representability. Meanwhile currently we skip any non-representability queries in a cycle. However now representability_cycle_handler is directly called from mk_cycle and doesn't use value_from_cycle_error function pointer for specializing on Representability return type.

This is a very crude check to rely on IMO. To remove FromCycleError I would've first implemented a FallbackProvider for fallback queries to directly specialize value_from_cycle_error function pointer and not through a result type's FromCycleError trait impl. I did it in #149319 and Zoxc did something similar in #153028. In my surely very unbiased opinion my #149319 gets rid of FromCycleError (Value back then) more elegantly than Zoxc have demonstrated in #153028 so far. Per Zoxc's stated motivation:

This is a step towards removing query cycle recovery as the FromCycleError trait would be removed for that.

Also AFAIK if any query is representability then every other query in a cycle is representability too. So we could add such assertion here if I'm correct.

Oh, I didn't know about #149319. (I was CC'd, but last year, long before I started looking into the query system.) From a quick look it has a vtable function pointer (execute_fallback) but also requires specifying providers. My rough design below uses a query modifier (like cache_on_disk_if) without needing providers.

zetanumbers · 2026-03-06T08:08:58Z

compiler/rustc_query_impl/src/execution.rs

 ) -> C::Value {
+    // Try to handle the cycle error with our custom handlers.
+    // They will unwind if they handle the cycle.
+    representability_cycle_handler(tcx, &cycle_error);


So we are hard coding representability query cycle handling into query system. This code should be more general.

nnethercote · 2026-03-06T10:00:26Z

Thinking some more, here's a rough design that is general, eliminates
FromCycleError, modifies value_from_cycle_error, and preserves existing
behaviour for almost all query types.

-----------------------------------------------------------------------------
Existing behaviours for various queries
-----------------------------------------------------------------------------
default: emit cycle error, abort

erase_and_anonymize_regions_ty?, type_of?: emit cycle error, return custom value
- currently FromCycleError impl
    
type_of_opaque: stash or emit cycle error, return custom value
- currently cycle_stash + FromCycleError impl
- can remove all this custom handling, give it the default behaviour, as per
  #153247
    
variances_of, fn_sig: emit delayed_bug, return custom value
- currently cycle_delay_bug + FromCycleError impl
    
layout_of: emit custom error, return custom value
- currently cycle_delay_bug + FromCycleError impl

check_representability: emit custom error, abort 
- currently cycle_delay_bug + FromCycleError impl

-----------------------------------------------------------------------------
New design
-----------------------------------------------------------------------------
Change value_from_cycle_error to this:

  value_from_cycle_error(tcx, key?, cycle_error, error)

Remove existing `cycle_*` query modifiers, replace with:

  cycle { $block }

where $block is a function body that is implicitly passed `tcx`, `key`(?),
`cycle_error`, and `error`. (A bit like the cache_on_disk_if modifier.)
These bodies might just call on to functions defined in
rustc_query_impl/src/from_cycle_error.rs               
                                                       
`FromCycleError` is removed, but the existing `from_cycle_error` methods would
mostly remain as free functions called from `$block`s. 
                                                       
Implementations of value_from_cycle_error:             
                                                       
- default (i.e. if you don't specify the `cycle` modifier)
    let guar = error.emit();                           
    guar.raise_fatal();                                
                                                       
- type_of:                                             
    let guar = error.emit();                           
    return Ty::new_error(guar);                             
                                                       
- variances_of:                                        
    let guar = error.delay_as_bug();                   
    return ty::Variance value computed from `cycle_error`
                                                       
- layout_of:                                           
  - let guar = construct and emit new error from `cycle_error`
  - return LayoutError::Cycle(guar)                    
                                                       
- check_representability:                              
  - let guar = construct and emit new error from `cycle_error`
  - guar.raise_fatal();

zetanumbers · 2026-03-06T10:39:26Z

I don't like macros and maybe we could consider moving it out of the macro invocation for the future, but ok. That should eliminate trait specialization, transmute and Box::leak just like with #149319. That's because we would no longer rely on trait implementations which cannot be specialized on a 'tcx lifetime. Would you like me to rework #149319 like this or you would like to do this on your own instead?

zetanumbers · 2026-03-06T10:45:53Z

However cycle { ... } would be placed in rustc_middle while currently FromCycleError is in rustc_query_impl. There is a use crate::job::report_cycle; import used, so we would have to move that there or remove its use from rustc_query_impl/src/from_cycle_error.rs smh.

zetanumbers · 2026-03-06T10:52:05Z

So this cycle { ... } modifier will only be used in rustc_query_impl with define_queries!. Macro hygiene might become a problem while using report_cycle there but we'll see I guess.

zetanumbers · 2026-03-06T11:10:05Z

FromCycleError is removed, but the existing from_cycle_error methods would mostly remain as free functions called from $blocks.

But that would mean moving these functions from rustc_query_impl (query definitions) into rustc_middle (query declarations). Alternatively we can modify QueryVTables upon its creation to assign special cycle handler function pointers. That should actually be a pretty simple change I think, may I try it?

nnethercote · 2026-03-06T11:25:41Z

I don't like macros and maybe we could consider moving it out of the macro invocation for the future, but ok.

Macros have their issues, yes, but the query system relies on them and that is unlikely to change. I think it's reasonable to use slightly more macro magic to get rid of other complexities.

Would you like me to rework #149319 like this or you would like to do this on your own instead?

It's Friday night here, I'm happy to let you try it out. (I hope it works! I think it will...)

But that would mean moving these functions from rustc_query_impl (query definitions) into rustc_middle (query declarations).

Shouldn't be necessary. value_from_cycle_error is set in make_query_vtable in rustc_query_impl.

zetanumbers · 2026-03-06T13:09:09Z

Alternatively we can modify QueryVTables upon its creation to assign special cycle handler function pointers. That should actually be a pretty simple change I think, may I try it?

Just as I've said this didn't take much of my time. Done in #153493

nnethercote · 2026-03-06T23:36:50Z

I like the approach in #153493, it's a smallish change that cleans things up quite a bit and there is scope for some additional follow-up changes. It's similar in spirit to this PR, but is different enough that this PR won't be able to proceed. Sorry about that. But #153492 looks really promising and #153247 could also be really good with some changes. So there's plenty to keep working on :)

Add a cycle handler for check_representability

89a972c

rustbot assigned jackh726 Mar 6, 2026

Zoxc mentioned this pull request Mar 6, 2026

Add per query cycle handlers and use them for representability #153028

Closed

nnethercote reviewed Mar 6, 2026

View reviewed changes

zetanumbers reviewed Mar 6, 2026

View reviewed changes

nnethercote assigned nnethercote and unassigned jackh726 Mar 6, 2026

zetanumbers mentioned this pull request Mar 6, 2026

Remove FromCycleError trait #153493

Open

nnethercote 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 10, 2026

Uh oh!

Conversation

Zoxc commented Mar 6, 2026

Uh oh!

rustbot commented Mar 6, 2026

Uh oh!

nnethercote left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnethercote Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

zetanumbers Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

zetanumbers Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

nnethercote Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

zetanumbers Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

nnethercote commented Mar 6, 2026

Uh oh!

zetanumbers commented Mar 6, 2026

Uh oh!

zetanumbers commented Mar 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zetanumbers commented Mar 6, 2026

Uh oh!

zetanumbers commented Mar 6, 2026

Uh oh!

nnethercote commented Mar 6, 2026

Uh oh!

zetanumbers commented Mar 6, 2026

Uh oh!

nnethercote commented Mar 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nnethercote left a comment •

edited by rustbot

Loading

zetanumbers commented Mar 6, 2026 •

edited

Loading