Skip to content

Conversation

compiler-errors
Copy link
Member

Fixes #108721

The problem here is that when we're checking is_sized_raw during codegen on some type that has a lot of opaques in it, something emits several nested obligations that are individually ambiguous, but when processed together in a loop then apply modulo regions. Since the evaluate_predicates_recursively inner loop doesn't process predicates until they stop changing, we return EvaluatedToAmbig, which makes the sized check return false incorrectly. See:

for obligation in predicates {
let eval = self.evaluate_predicate_recursively(stack, obligation.clone())?;
if let EvaluatedToErr = eval {
// fast-path - EvaluatedToErr is the top of the lattice,
// so we don't need to look on the other predicates.
return Ok(EvaluatedToErr);
} else {
result = cmp::max(result, eval);
}
}
Ok(result)

... Compared to the analogous loop in the new solver:

self.repeat_while_none(
|_| Ok(Certainty::Maybe(MaybeCause::Overflow)),
|this| {
let mut has_changed = Err(Certainty::Yes);
for goal in goals.drain(..) {
let (changed, certainty) = match this.evaluate_goal(goal) {
Ok(result) => result,
Err(NoSolution) => return Some(Err(NoSolution)),
};
if changed {
has_changed = Ok(());
}
match certainty {
Certainty::Yes => {}
Certainty::Maybe(_) => {
new_goals.push(goal);
has_changed = has_changed.map_err(|c| c.unify_and(certainty));
}
}
}
match has_changed {
Ok(()) => {
mem::swap(&mut new_goals, &mut goals);
None
}
Err(certainty) => Some(Ok(certainty)),
}
},
)

To fix this, if we get ambiguous during pred_known_to_hold_modulo_regions, just retry the obligation in a fulfillment context.

--

Unfortunately... I don't have a test for this. I've only tested this locally. Pending minimization :/

r? types

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 4, 2023
@Noratrieb
Copy link
Member

I tried to minimize it but it's basically impossible. Unless someone comes up with a reproduction bottom-up, we should just merge this without a test. It fixes a P-critical.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 7, 2023

📌 Commit 118afdf has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2023
@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 7, 2023
@compiler-errors
Copy link
Member Author

Beta nominating since it fixes a P-critical issue. No minimization, which is a shame, but I think the fix is pretty straightforward.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 8, 2023
Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous

Fixes rust-lang#108721

The problem here is that when we're checking `is_sized_raw` during codegen on some type that has a lot of opaques in it, something emits several nested obligations that are individually ambiguous, but when processed together in a loop then apply modulo regions. Since the `evaluate_predicates_recursively` inner loop doesn't process predicates until they stop changing, we return `EvaluatedToAmbig`, which makes the sized check return false incorrectly. See:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/traits/select/mod.rs#L596-L606

... Compared to the analogous loop in the new solver:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/solve/mod.rs#L481-L512

To fix this, if we get ambiguous during `pred_known_to_hold_modulo_regions`, just retry the obligation in a fulfillment context.

--

Unfortunately... I don't have a test for this. I've only tested this locally. Pending minimization :/

r? types
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous

Fixes rust-lang#108721

The problem here is that when we're checking `is_sized_raw` during codegen on some type that has a lot of opaques in it, something emits several nested obligations that are individually ambiguous, but when processed together in a loop then apply modulo regions. Since the `evaluate_predicates_recursively` inner loop doesn't process predicates until they stop changing, we return `EvaluatedToAmbig`, which makes the sized check return false incorrectly. See:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/traits/select/mod.rs#L596-L606

... Compared to the analogous loop in the new solver:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/solve/mod.rs#L481-L512

To fix this, if we get ambiguous during `pred_known_to_hold_modulo_regions`, just retry the obligation in a fulfillment context.

--

Unfortunately... I don't have a test for this. I've only tested this locally. Pending minimization :/

r? types
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 8, 2023
Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous

Fixes rust-lang#108721

The problem here is that when we're checking `is_sized_raw` during codegen on some type that has a lot of opaques in it, something emits several nested obligations that are individually ambiguous, but when processed together in a loop then apply modulo regions. Since the `evaluate_predicates_recursively` inner loop doesn't process predicates until they stop changing, we return `EvaluatedToAmbig`, which makes the sized check return false incorrectly. See:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/traits/select/mod.rs#L596-L606

... Compared to the analogous loop in the new solver:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/solve/mod.rs#L481-L512

To fix this, if we get ambiguous during `pred_known_to_hold_modulo_regions`, just retry the obligation in a fulfillment context.

--

Unfortunately... I don't have a test for this. I've only tested this locally. Pending minimization :/

r? types
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2023
Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous

Fixes rust-lang#108721

The problem here is that when we're checking `is_sized_raw` during codegen on some type that has a lot of opaques in it, something emits several nested obligations that are individually ambiguous, but when processed together in a loop then apply modulo regions. Since the `evaluate_predicates_recursively` inner loop doesn't process predicates until they stop changing, we return `EvaluatedToAmbig`, which makes the sized check return false incorrectly. See:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/traits/select/mod.rs#L596-L606

... Compared to the analogous loop in the new solver:

https://github.com/rust-lang/rust/blob/f15f0ea73972786e426732c5b92ba9a904b866c4/compiler/rustc_trait_selection/src/solve/mod.rs#L481-L512

To fix this, if we get ambiguous during `pred_known_to_hold_modulo_regions`, just retry the obligation in a fulfillment context.

--

Unfortunately... I don't have a test for this. I've only tested this locally. Pending minimization :/

r? types
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 9, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108754 (Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous)
 - rust-lang#108759 (1.41.1 supported 32-bit Apple targets)
 - rust-lang#108839 (Canonicalize root var when making response from new solver)
 - rust-lang#108856 (Remove DropAndReplace terminator)
 - rust-lang#108882 (Tweak E0740)
 - rust-lang#108898 (Set `LIBC_CHECK_CFG=1` when building Rust code in bootstrap)
 - rust-lang#108911 (Improve rustdoc-gui/tester.js code a bit)
 - rust-lang#108916 (Remove an unused return value in `rustc_hir_typeck`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9408af9 into rust-lang:master Mar 9, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 9, 2023
@apiraino
Copy link
Contributor

apiraino commented Mar 9, 2023

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 9, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 10, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108754 (Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous)
 - rust-lang#108759 (1.41.1 supported 32-bit Apple targets)
 - rust-lang#108839 (Canonicalize root var when making response from new solver)
 - rust-lang#108856 (Remove DropAndReplace terminator)
 - rust-lang#108882 (Tweak E0740)
 - rust-lang#108898 (Set `LIBC_CHECK_CFG=1` when building Rust code in bootstrap)
 - rust-lang#108911 (Improve rustdoc-gui/tester.js code a bit)
 - rust-lang#108916 (Remove an unused return value in `rustc_hir_typeck`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.70.0 milestone Mar 11, 2023
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 11, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.69.0 milestone Mar 11, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2023
…k-Simulacrum

[beta] backport

This PR backports:

- rust-lang#108901: fix: evaluate with wrong obligation stack
- rust-lang#108754: Retry `pred_known_to_hold_modulo_regions` with fulfillment if ambiguous
- rust-lang#108691: fix multiple issues when promoting type-test subject

It also bumps to the released stable.

r? `@Mark-Simulacrum`
@compiler-errors compiler-errors deleted the retry branch August 11, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: panic adding warp::trace::request() combinator
7 participants