query cycles could be handled more like ICEs

[matthiaskrgr]

from https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/recent-ish.20nightly.20regression.20wrt.20Freeze

Sometimes the compiler blurps out something like error[E0391]: cycle detected when XYZ

yay: the compiler saved itself from going in circles infinitely because it noticed it was trying to compute something self-referential

nay: we SHOULD have had a proper diagnostic earlier in which the compiler errors out and explains the user why the self-referential thing they are trying to do is not going to work out instead of just throwing a "oops, query cycle" in their face.

If we encounter such a query cycle, we should tell users to report it a diagnostic bug. Maybe we can also improve the spans of the E0391 a bit.

error[E0391]: cycle detected when looking up late bound vars
 --> /home/matthias/vcs/github/rust_misc_stuff/tests/ui/traits/inheritance/auxiliary/icemaker_omni/BE190CEE104A225CD4B33CFB5FA41C069766D54BE3319CDCF10B5BCBEC24AD11.rs:4:1
  |
4 | pub trait MyNum : Add<Output=Self> + MyNum<Output=Self> + Mul<Output=Self> + PartialEq + Clone {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
note: ...which requires resolving lifetimes...
 --> /home/matthias/vcs/github/rust_misc_stuff/tests/ui/traits/inheritance/auxiliary/icemaker_omni/BE190CEE104A225CD4B33CFB5FA41C069766D54BE3319CDCF10B5BCBEC24AD11.rs:4:1
  |
4 | pub trait MyNum : Add<Output=Self> + MyNum<Output=Self> + Mul<Output=Self> + PartialEq + Clone {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires computing the super traits of `MyNum` with associated type name `Output`...
 --> /home/matthias/vcs/github/rust_misc_stuff/tests/ui/traits/inheritance/auxiliary/icemaker_omni/BE190CEE104A225CD4B33CFB5FA41C069766D54BE3319CDCF10B5BCBEC24AD11.rs:4:1
  |
4 | pub trait MyNum : Add<Output=Self> + MyNum<Output=Self> + Mul<Output=Self> + PartialEq + Clone {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: ...which again requires looking up late bound vars, completing the cycle
note: cycle used when computing the super predicates of `MyNum`
 --> /home/matthias/vcs/github/rust_misc_stuff/tests/ui/traits/inheritance/auxiliary/icemaker_omni/BE190CEE104A225CD4B33CFB5FA41C069766D54BE3319CDCF10B5BCBEC24AD11.rs:4:1
  |
4 | pub trait MyNum : Add<Output=Self> + MyNum<Output=Self> + Mul<Output=Self> + PartialEq + Clone {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

error: aborting due to 3 previous errors

[matthiaskrgr]

We could also consider making query cycles an ice with debug_assertions enabled while building rustc, but maybe that's a bit too harsh for now... (but then again, I don't think it can cause any kind of regressions in compiling code, right.?)

[compiler-errors]

I dont think you need to file a bunch of issues for fuzzed query cycles.

They could be improved, but they're not ICEs--at the end of the day, they're regular (yes, verbose) errors.

Unless you're also going to write up a detailed explanation about how and if they can be fixed at all, I think it's far more noise than what is worth it.

[Noratrieb]

While query cycles are very bad diagnostics, I think it's fine and kinda unavoidable if some very weird code (fuzzed code) has cycles. If a user hits a query cycle, that should be fixed, but for fuzzed code it doesn't really matter imo.

[fmease]

They could be improved, but they're not ICEs--at the end of the day, they're regular (yes, verbose) errors.

I definitely agree with this. On the other hand, I gave this issue a :+1: because query cycles arising from innocent looking code that uses experimental features are often indicative of a bug in the implementation, let me just mention https://github.com/rust-lang/rust/labels/F-generic_const_exprs and https://github.com/rust-lang/rust/labels/F-inherent_associated_types. So much so that I wish there was I-cycle.

[matthiaskrgr]

When just checking the files inside the rustc repo (file in our testsuite), we already have 14 different kinds of query cycles showing up, so no fuzzing required ;) I can also see that some of them are explicitly caused by broken features like generic_const_exprs ( tests/ui/unsafe/issue-87414-query-cycle.rs goes from build pass -> query cycle for example)