rustc_trait_selection: fix trait solver hang caused by degenerate obligations

[InvalidPathException]

View all comments

Closes #157516 and #159750.

Bisection pointed to #142712 but the problem has always been present. Fulfillment can go on forever if a degenerate obligation causes additional obligations from poly_project_and_unify_term to be copies of the degenerate one.

Current fix is checking if the obligation is degenerate, making the degenerate case stall, waiting for other obligations to constrain its inference variables, so it can actually normalize or error instead of propagating this. error immediately since the code will not compile anyways.

I am not sure if this is ideal, might need to check perf due to the added heavy-ish condition check.

[rustbot]

r? @mu001999

rustbot has assigned @mu001999.
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
• compiler expanded to 74 candidates
• Random selection from 20 candidates

[mu001999]

@rustbot reroll

[mu001999]

might need to check perf due to the added heavy-ish condition check.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 41450ef (41450ef5e1890fe9aaccef25127708688bc116af)
Base parent: 165cce8 (165cce8d820b229af8f6a8226cf0b910b57600ff)

[rust-timer]

Finished benchmarking commit (41450ef): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.5%	[0.4%, 0.5%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -8.4%, secondary -2.0%)

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

Cycles

Results (primary 2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.0%	[2.0%, 2.0%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[2.0%, 2.0%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 485.902s -> 492.139s (1.28%)
Artifact size: 387.66 MiB -> 387.65 MiB (-0.00%)

[jackh726]

I'm curious if there's any code that we expect to start to compile with this change, as opposed to eventually erroring. If the answer is no, then perhaps we can just error immediately, instead of stalling.

View changes since this review

[jackh726]

This really could be expressed better. Something like

  ProjectAndUnifyResult::Holds(os) if os.is_empty() =>ProcessResult::Changed(mk_pending(obligation, os)),
  ProjectAndUnifyResult::Holds(os) => {
      let input_projection_term = infcx
          .resolve_vars_if_possible(project_obligation.predicate)
          .map_bound(|p| p.projection_term);
      let all_same_term = os.iter().all(|o| {
          let Some(proj_clause) = o.predicate.as_projection_clause() else { return false };
          infcx.resolve_vars_if_possible(proj_clause).map_bound(|p| p.projection_term) == input
      });
      if all_same_term {
          ...
      } else {
          ProcessResult::Changed(mk_pending(obligation, os))
      }
  }

[jackh726]

A concrete example in a comment here would be useful.

This is also slightly stronger than the comment, anyways: every nested obligation has the same projection term.

[InvalidPathException]

I'm curious if there's any code that we expect to start to compile with this change, as opposed to eventually erroring. If the answer is no, then perhaps we can just error immediately, instead of stalling.

I did consider this, there should not be anything that compiles because of this today, the stall path is reachable only when

1. normalization returned NoProgress (ProjectionCandidateSet::None in project)
2. equating the rigid alias with the term variable failed generalization

At that point it must error regardless of how it resolves later on (as of today). But stalling gives a (maybe) better message as we allow more info to come in, since this path is relatively rare, it seems at least not problematic to stall.

trait Bound {}
impl Bound for u32 {}

trait Driver {
    type Object<Ctx: Bound>: Bound;
}

fn make<T: Driver, C: Bound>(c: C) -> T::Object<C> {
    todo!()
}

fn any<X>() -> X {
    todo!()
}

fn f<T: Driver>() {
    let mut x = any();          // x: ?x
    x = make::<T, _>(x);        // forces <T as Driver>::Object<?x> == ?x
}

fn main() {}

Stall:

error[E0283]: type annotations needed
  --> demo.rs:17:9
   |
17 |     let mut x = any();
   |         ^^^^^
18 |     x = make::<T, _>(x);
   |                   - type must be known at this point
   |
   = note: the type must implement `Bound`
help: the trait `Bound` is implemented for `u32`
  --> demo.rs:2:1
   |
 2 | impl Bound for u32 {}
   | ^^^^^^^^^^^^^^^^^^
note: required by a bound in `make`
  --> demo.rs:8:23
   |
 8 | fn make<T: Driver, C: Bound>(_c: C) -> T::Object<C> {
   |                       ^^^^^ required by this bound in `make`
help: consider giving `x` an explicit type
   |
17 |     let mut x: u32 = any();
   |              +++++

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0283`.

Error immediately:

error[E0284]: type annotations needed for `<T as Driver>::Object<_>`
  --> demo.rs:17:9
   |
17 |     let mut x = any();
   |         ^^^^^
18 |     x = make::<T, _>(x);
   |                      - type must be known at this point
   |
   = note: cannot satisfy `<T as Driver>::Object<_>
help: consider giving `x` an explicit type, where the placeholders `_` are specified
   |
17 |     let mut x: <_ as Driver>::Object<_> = any();
   |              ++++++++++++++++++++++++++

error: aborting due to 1 previous error

For more information about this error, try `rustc --

[jackh726]

Given that the new solver is coming soon and completely overhauls normalization, I'm not inclined to potentially introduce more things that could compile, particularly if the reasoning is just better diagnostics.

Even still, the diagnostics in the error immediately case are not really bad. So, let's go that route.

[jackh726]

Sorry, can you squash? And then r=me

[InvalidPathException]

r? @jackh726

[JonathanBrouwer]

@bors try jobs=optional-x86_64-gnu-parallel-frontend

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1e67c82 (1e67c821b5dea780f4b1a46d8fa80631df419fe7)
Base parent: 5d48869 (5d4886964b04e1aa6aca158a677183a778e2950d)

[rust-log-analyzer]

The job optional-x86_64-gnu-parallel-frontend failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

diff of stderr:

20 LL | |     Self::Item: Baz,
21    | |____________________^
22    = note: ...which again requires coherence checking all impls of trait `Foo`, completing the cycle
- note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:12:1: 14:20>` is well-formed
-   --> $DIR/next-solver-region-resolution.rs:12:1
+ note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
+   --> $DIR/next-solver-region-resolution.rs:18:1
25    |
- LL | / impl<'a, T> Foo for &'a T
+ LL | / impl<'a, T> Foo for &T
27 LL | | where
- LL | |     Self::Item: 'a,
-    | |___________________^
+ LL | |     Self::Item: Baz,
+    | |____________________^
30    = note: for more information, see <https://rustc-dev-guide.rust-lang.org/overview.html#queries> and <https://rustc-dev-guide.rust-lang.org/query.html>
31 
32 error: aborting due to 1 previous error

Note: some mismatched output was normalized before being compared
- note: cycle used when checking that `<impl at /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
-   --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1
+ note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
+   --> $DIR/next-solver-region-resolution.rs:18:1
+ LL | / impl<'a, T> Foo for &T
+ LL | |     Self::Item: Baz,
+    | |____________________^

Compare output by lines enabled, diff by lines:
Expected contains these lines that are not in actual:
   | |___________________^
  --> $DIR/next-solver-region-resolution.rs:12:1
LL | / impl<'a, T> Foo for &'a T
LL | |     Self::Item: 'a,
note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:12:1: 14:20>` is well-formed
Actual contains these lines that are not in expected:
   | |____________________^
  --> $DIR/next-solver-region-resolution.rs:18:1
LL | / impl<'a, T> Foo for &T
LL | |     Self::Item: Baz,
note: cycle used when checking that `<impl at $DIR/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed

The actual stderr differed from the expected stderr
To update references, rerun the tests and pass the `--bless` flag
To only update this specific test, also pass `--test-args specialization/min_specialization/next-solver-region-resolution.rs`

error: 1 errors occurred comparing output.
status: exit status: 1
command: env -u RUSTC_LOG_COLOR RUSTC_ICE="0" RUST_BACKTRACE="short" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs" "-Zthreads=1" "-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX" "-Ztranslate-remapped-path-to-local-path=no" "-Z" "ignore-directory-in-diagnostics-source-blocks=/cargo" "-Z" "ignore-directory-in-diagnostics-source-blocks=/checkout/vendor" "--sysroot" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2" "--target=x86_64-unknown-linux-gnu" "--check-cfg" "cfg(test,FALSE)" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-Zwrite-long-types-to-disk=no" "-Cstrip=debuginfo" "-Zthreads=4" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/specialization/min_specialization/next-solver-region-resolution" "-A" "unused" "-W" "unused_attributes" "-A" "internal_features" "-A" "incomplete_features" "-A" "unused_parens" "-A" "unused_braces" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Znext-solver=globally"
stdout: none
--- stderr -------------------------------
error[E0391]: cycle detected when coherence checking all impls of trait `Foo`
##[error]  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:6:1
   |
LL | trait Foo { //~ ERROR cycle detected when coherence checking all impls of trait `Foo`
   | ^^^^^^^^^
   |
   = note: ...which requires building specialization graph of trait `Foo`...
note: ...which requires computing whether impls specialize one another...
  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:12:1
   |
LL | / impl<'a, T> Foo for &'a T
LL | | where
LL | |     Self::Item: 'a,
   | |___________________^
note: ...which requires computing normalized predicates of `<impl at /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1: 20:21>`...
  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1
   |
LL | / impl<'a, T> Foo for &T
LL | | where
LL | |     Self::Item: Baz,
   | |____________________^
   = note: ...which again requires coherence checking all impls of trait `Foo`, completing the cycle
note: cycle used when checking that `<impl at /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1: 20:21>` is well-formed
  --> /checkout/tests/ui/specialization/min_specialization/next-solver-region-resolution.rs:18:1
   |
LL | / impl<'a, T> Foo for &T
LL | | where
LL | |     Self::Item: Baz,
   | |____________________^
   = note: for more information, see <https://rustc-dev-guide.rust-lang.org/overview.html#queries> and <https://rustc-dev-guide.rust-lang.org/query.html>

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0391`.
------------------------------------------

[JonathanBrouwer]

This pr seems to cause a failure in the optional parallel frontend

@bors try jobs=x86_64-mingw
Running try job to debug an independent rollup failure

[rust-bors]

💔 Test for d3ffc12 failed: CI. Failed job:

• Calculate job matrix (web logs, enhanced plaintext logs)

[JonathanBrouwer]

@bors try jobs=x86_64-mingw

[InvalidPathException]

it looks like the output is actually "correct", the cause of the failure is a race, i.e. which of the two impls' well-formedness checks got reported as the cycle's entry point

Expected

note: cycle used when checking that `<impl at $DIR/...rs:12:1: 14:20>` is well-formed
  --> $DIR/next-solver-region-resolution.rs:12:1
   |
LL | / impl<'a, T> Foo for &'a T
LL | | where
LL | |     Self::Item: 'a,
   | |___________________^

Actual:

note: cycle used when checking that `<impl at $DIR/...rs:18:1: 20:21>` is well-formed
  --> $DIR/next-solver-region-resolution.rs:18:1
   |
LL | / impl<'a, T> Foo for &T
LL | | where
LL | |     Self::Item: Baz,
   | |____________________^

[rust-bors]

💔 Test for 0766bd9 failed: CI. Failed job:

• Calculate job matrix (web logs, enhanced plaintext logs)

[InvalidPathException]

@bors try jobs=x86_64-mingw

[rust-bors]

@InvalidPathException: 🔑 Insufficient privileges: not in try users

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling ureq v3.0.8
   Compiling citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)
    Finished `dev` profile [unoptimized] target(s) in 22.73s
     Running `target/debug/citool calculate-job-matrix`
Run type: TryJob { job_patterns: Some(["x86_64-mingw"]) }
Error: Failed to calculate job matrix

Caused by:
    Patterns `x86_64-mingw` did not match any auto jobs
##[error]Process completed with exit code 1.

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

   Compiling glob-match v0.2.1
   Compiling citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)
    Finished `dev` profile [unoptimized] target(s) in 22.38s
     Running `target/debug/citool calculate-job-matrix`
Run type: TryJob { job_patterns: Some(["x86_64-mingw"]) }
Error: Failed to calculate job matrix

Caused by:
    Patterns `x86_64-mingw` did not match any auto jobs
##[error]Process completed with exit code 1.

[JonathanBrouwer]

@bors try jobs=x86_64-mingw-1

[rust-bors]

⌛ Trying commit f2e0edf with merge f97b318…

To cancel the try build, run the command @bors try cancel.

Workflow: https://github.com/rust-lang/rust/actions/runs/30213658965