trait_solver: normalize next-gen region constraints

[Dnreikronos]

View all comments

fixes #157729

-zassumptions-on-binders can produce next-gen region constraints that mean the same thing but don't have the same shape. in this case object candidate merging stayed ambiguous and instance resolution later hit the ice. imo normalizing the constraint at the response boundary is the least weird place for this, because candidate selection shouldn't need to know which vtable looks nicer.

canonicalize and evaluate the next-gen region constraint before response canonicalization, then cover the dyn derived

supertrait case from the issue. lgtm locally with the focused test, tests/ui/traits/next-solver, and tests/ui/assumptions_on_binders. idk if there's a better home for the helper call, but ltm this keeps the fix pretty narrow.

[rustbot]

r? @camelid

rustbot has assigned @camelid.
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 73 candidates
• Random selection from 19 candidates

[camelid]

I don't have familiarity with this code, so rerolling. @rustbot reroll

[mati865]

Me neither, @rustbot reroll

[hanna-kruppe]

Me three. Let's try something more targeted: r? types

[lcnr]

r? BoxyUwU

[rustbot]

BoxyUwU is currently at their maximum review capacity.
They may take a while to respond.

[BoxyUwU]

so it makes sense to me why having a non-normal form constraint here would cause issues 🤔 what I don't get is why we have a non-normal one here. Though admittedly we don't put much effort into tracking where/when we expect stuff to be normal form lol.

I would add an assertion to this that self.delegate.get_solver_region_constraint() returns something for which evaluating it & computing a canonical form doesn't actually do anything. In theory the only way we should call compute_external_query_constraints should be if we've already called eagerly_handle_placeholders which should evaluate/convert to normal form the constraints and then set that back into the infcx/delegate 🤔

it seems to me that for whatever reason eagerly_handle_placeholders isnt actually giving back normal form constraints for whatever reason, I would expect the "right" fix would be to look into why that's happening and see if we can fix that

View changes since the review

[BoxyUwU]

this and the changes to eagerly_handle_placeholders look good to me, what's with all the other changes in this PR ? :3

View changes since the review

[Dnreikronos]

yeah, the rest is mostly fallout from making the eager normalization stick.

overwrite_solver_region_constraint used to wrap anything that wasn't an And inside an And, so the Or<And<...>> we just normalized was made non-normal again right away. imo that's pretty surprising. the rustc_infer changes let the storage keep the constraint as-is, and then push/rollback has to remember whether the previous root was already an And.

the permutations change is related too. it used to build conjunctions backwards, so calling canonical_form() again flipped their order. basically it wasn't idempotent and would trip the new assertion. btw, the last file is just the regression test for #157729.

idk if you think the storage change is too invasive, but imo preserving the value we wrote is less weird than normalizing it again when reading it back. ltm if you'd prefer that split into a separate PR/commit.

[BoxyUwU]

Ah yes okay that makes sense thanks for explaining :3

[BoxyUwU]

@rustbot author

[rust-bors]

💔 Test for 7e65599 failed: CI

[JonathanBrouwer]

@bors retry
Spurious

[jhpratt]

@bors r- #159926 (comment)

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`.
------------------------------------------

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#159926), which was unapproved.

View changes since this unapproval

[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.