Rename various query cycle things.

[nnethercote]

The existing names have bugged me for a while. Changes:

• CycleError -> Cycle. Because we normally use "error" for a Diag with Level::Error, and this type is just a precursor to that. Also, many existing locals of this type are already named cycle.

• CycleError::cycle -> Cycle::frames. Because it is a sequence of frames, and we want to avoid Cycle::cycle (and cycle.cycle).

• cycle_error -> find_and_handle_cycle. Because that's what it does. The existing name is just a non-descript noun phrase.

• mk_cycle -> handle_cycle. Because it doesn't make the cycle; the cycle is already made. It handles the cycle, which involves (a) creating the error, and (b) handling the error.

• report_cycle -> create_cycle_error. Because that's what it does: creates the cycle error (i.e. the Diag).

• value_from_cycle_error -> handle_cycle_error_fn. Because most cases don't produce a value, they just emit an error and quit. And the _fn suffix is for consistency with other vtable fns.

• from_cycle_error -> handle_cycle_error. A similar story for this module.

r? @petrochenkov

[nnethercote]

cc @Zalathar @Zoxc @zetanumbers

I know this has high conflict potential so I'm happy to wait on merging this if it will cause problems for any other current work. But I did want to see what it would look like. I think it turned out well.

[zetanumbers]

No, I don't like renamings. I have to adjust every time. Thanks for notifying beforehand at least.

But about the matter. The word "handle" seems like one of those fit-everything words like "data", etc.

[Zoxc]

• CycleError::cycle -> Cycle::stack. Because it is a stack

Not for the parallel case though, it's a list of edges labeled with Span forming a cycle.

[nnethercote]

Not for the parallel case though, it's a list of edges labeled with Span forming a cycle.

Suggestion for a better name for that field? frames?

[Zoxc]

cycle does fit quite well.

[nnethercote]

But if Cycle::cycle is a field then we end up writing cycle.cycle a lot, which feels bad. cycle.frames seems better to me.

[Zoxc]

path would be a less specific variant of cycle. frames works too I guess.

[nnethercote]

The word "handle" seems like one of those fit-everything words like "data", etc.

In this case it reflects the messiness of the code. Each handle_cycle_error can do a variety of things

• emit the cycle error message or print a custom error message
• abort or return an erroneous value

The existing name from_cycle_error only describes one of these possibilities. A fully descriptive name would be something like emit_cycle_or_custom_error_and_abort_or_return_erroneous_value but that's untenable so a vague catch-all verb like "handle" is hard to beat.

[nnethercote]

@rustbot ready

[petrochenkov]

@bors r+

[rust-bors]

📌 Commit b357d7a has been approved by petrochenkov

It is now in the queue for this repository.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[nnethercote]

I rebased.

@bors r=petrochenkov

[rust-bors]

📌 Commit 90ea993 has been approved by petrochenkov

It is now in the queue for this repository.

[jyn514]

I believe this broke query cycle handling. I'm seeing the following crash in ferrocene/ferrocene#2239:

(lldb) bt 100
  ...
  * frame #19: 0x000000011d710148 librustc_driver-76b3d4856834c44c.dylib`create_cycle_error at job.rs:474:37 [opt]
    frame #20: 0x000000011d72fd58 librustc_driver-76b3d4856834c44c.dylib`handle_cycle<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>> at execution.rs:117:17 [opt]
    frame #21: 0x000000011d732eb4 librustc_driver-76b3d4856834c44c.dylib`find_and_handle_cycle<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>> at execution.rs:209:6 [opt]
    frame #22: 0x000000011b400d44 librustc_driver-76b3d4856834c44c.dylib`try_execute_query<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>, false> at execution.rs:333:25 [opt]
    frame #23: 0x000000011b571da4 librustc_driver-76b3d4856834c44c.dylib`{closure#0}<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>> at execution.rs:596:32 [opt] [inlined]
    frame #24: 0x000000011b571d80 librustc_driver-76b3d4856834c44c.dylib`maybe_grow<rustc_middle::query::erase::ErasedData<[u8; 8]>, rustc_query_impl::execution::execute_query_non_incr_inner::{closure_env#0}<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>>> at lib.rs:57:9 [opt] [inlined]
    frame #25: 0x000000011b571d64 librustc_driver-76b3d4856834c44c.dylib`ensure_sufficient_stack<rustc_middle::query::erase::ErasedData<[u8; 8]>, rustc_query_impl::execution::execute_query_non_incr_inner::{closure_env#0}<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>>> at stack.rs:21:5 [opt] [inlined]
    frame #26: 0x000000011b571d64 librustc_driver-76b3d4856834c44c.dylib`execute_query_non_incr_inner<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>> at execution.rs:596:5 [opt] [inlined]
    frame #27: 0x000000011b571d64 librustc_driver-76b3d4856834c44c.dylib`__rust_end_short_backtrace at query_impl.rs:80:30 [opt]
    frame #28: 0x000000011c46eb5c librustc_driver-76b3d4856834c44c.dylib`query_get_at<rustc_middle::query::caches::DefIdCache<rustc_middle::query::erase::ErasedData<[u8; 8]>>> at inner.rs:45:17 [opt] [inlined]
    frame #29: 0x000000011c46eaa4 librustc_driver-76b3d4856834c44c.dylib`def_span<rustc_span::def_id::DefId> at plumbing.rs:571:46 [opt] [inlined]
    frame #30: 0x000000011c46eaa4 librustc_driver-76b3d4856834c44c.dylib`def_span<rustc_span::def_id::DefId> at plumbing.rs:559:39 [opt] [inlined]
    frame #31: 0x000000011c46eaa4 librustc_driver-76b3d4856834c44c.dylib`default_span at keys.rs:141:13 [opt]
    frame #32: 0x000000011c46ea5c librustc_driver-76b3d4856834c44c.dylib`default_span at keys.rs:129:26 [opt]
    frame #33: 0x000000011c332484 librustc_driver-76b3d4856834c44c.dylib`default_span at plumbing.rs:447:29 [opt]
    frame #34: 0x000000011d710148 librustc_driver-76b3d4856834c44c.dylib`create_cycle_error at job.rs:474:37 [opt]
... several million more frames, crashes LLDB if i try to print the whole thing.

I think this is because TaggedQueryKey is no longer correctly detecting cycles within def_span itself:

rust/compiler/rustc_middle/src/query/plumbing.rs

Lines 439 to 442 in 8a70352

	if let TaggedQueryKey::def_span(..) = self {
	// The `def_span` query is used to calculate `default_span`,
	// so exit to avoid infinite recursion.
	return DUMMY_SP

[jyn514]

the TaggedQueryKey that recurses infinitely is local_def_id_to_hir_id(DefId(0:21 ~ bad_resolve[627f]::{impl#2}::bar)) (where bad_resolve is

rust/tests/ui/delegation/bad-resolve.rs

Line 33 in 8a70352

reuse <F as Trait>::bar;

)

[Zalathar]

There have been a number of cycle-handling changes recently, so it’s plausible that something broke, but this particular PR seems to be a fairly clean renaming, so the cause might be elsewhere.