Skip to content

Commit a04dfc3

Browse files
compiler-errorscompiler-errors
authoredAug 11, 2023
Rollup merge of rust-lang#114694 - lcnr:provisional-cache, r=compiler-errors
make the provisional cache slightly less broken It is still broken for the following cycles: ```mermaid graph LR R["R: coinductive"] --> A["A: inductive"] R --> B["B: coinductive"] A --> B B --> R ``` the `R -> A -> B -> R` cycle should be considered to not hold, as it is mixed, but because we first put `B` into the cache from the `R -> B -> R` cycle which is coinductive, it does hold. This issue will also affect our new coinduction approach. Longterm cycles are coinductive as long as one step goes through an impl where-clause, see https://github.com/rust-lang/a-mir-formality/blob/f4fc5bae36ab1a9fefddd54e5ccffc5f671467ec/crates/formality-prove/src/prove/prove_wc.rs#L51-L62. Here we would first have a fully inductive cycle `R -> B -> R` which is then entered by a cycle with a coinductive step `R -> A -coinductive-> B -> R`. I don't know how to soundly implement a provisional cache for goals not on the stack without tracking all cycles the goal was involved in and whether they were inductive or not. We could then only use goals from the cache if the *inductivity?* of every cycle remained the same. This is a mess to implement. I therefore want to rip out the provisional cache entirely, but will wait with this until I talked about it with `@nikomatsakis.` r? `@compiler-errors`
2 parents 2845bad + 02529d2 commit a04dfc3

25 files changed

+274
-56
lines changed
 

‎compiler/rustc_trait_selection/src/solve/search_graph/cache.rs

+18-14
Original file line numberDiff line numberDiff line change
@@ -19,21 +19,25 @@ rustc_index::newtype_index! {
1919

2020
#[derive(Debug, Clone)]
2121
pub(super) struct ProvisionalEntry<'tcx> {
22-
// In case we have a coinductive cycle, this is the
23-
// the currently least restrictive result of this goal.
24-
pub(super) response: QueryResult<'tcx>,
25-
// In case of a cycle, the position of deepest stack entry involved
26-
// in that cycle. This is monotonically decreasing in the stack as all
27-
// elements between the current stack element in the deepest stack entry
28-
// involved have to also be involved in that cycle.
29-
//
30-
// We can only move entries to the global cache once we're complete done
31-
// with the cycle. If this entry has not been involved in a cycle,
32-
// this is just its own depth.
22+
/// In case we have a coinductive cycle, this is the
23+
/// the current provisional result of this goal.
24+
///
25+
/// This starts out as `None` for all goals and gets to some
26+
/// when the goal gets popped from the stack or we rerun evaluation
27+
/// for this goal to reach a fixpoint.
28+
pub(super) response: Option<QueryResult<'tcx>>,
29+
/// In case of a cycle, the position of deepest stack entry involved
30+
/// in that cycle. This is monotonically decreasing in the stack as all
31+
/// elements between the current stack element in the deepest stack entry
32+
/// involved have to also be involved in that cycle.
33+
///
34+
/// We can only move entries to the global cache once we're complete done
35+
/// with the cycle. If this entry has not been involved in a cycle,
36+
/// this is just its own depth.
3337
pub(super) depth: StackDepth,
3438

35-
// The goal for this entry. Should always be equal to the corresponding goal
36-
// in the lookup table.
39+
/// The goal for this entry. Should always be equal to the corresponding goal
40+
/// in the lookup table.
3741
pub(super) input: CanonicalInput<'tcx>,
3842
}
3943

@@ -92,7 +96,7 @@ impl<'tcx> ProvisionalCache<'tcx> {
9296
self.entries[entry_index].depth
9397
}
9498

95-
pub(super) fn provisional_result(&self, entry_index: EntryIndex) -> QueryResult<'tcx> {
99+
pub(super) fn provisional_result(&self, entry_index: EntryIndex) -> Option<QueryResult<'tcx>> {
96100
self.entries[entry_index].response
97101
}
98102
}

‎compiler/rustc_trait_selection/src/solve/search_graph/mod.rs

+39-26
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use rustc_middle::traits::solve::CacheData;
1313
use rustc_middle::traits::solve::{CanonicalInput, Certainty, EvaluationCache, QueryResult};
1414
use rustc_middle::ty::TyCtxt;
1515
use rustc_session::Limit;
16-
use std::{collections::hash_map::Entry, mem};
16+
use std::collections::hash_map::Entry;
1717

1818
rustc_index::newtype_index! {
1919
pub struct StackDepth {}
@@ -216,8 +216,8 @@ impl<'tcx> SearchGraph<'tcx> {
216216
cycle_participants: Default::default(),
217217
};
218218
assert_eq!(self.stack.push(entry), depth);
219-
let response = Self::response_no_constraints(tcx, input, Certainty::Yes);
220-
let entry_index = cache.entries.push(ProvisionalEntry { response, depth, input });
219+
let entry_index =
220+
cache.entries.push(ProvisionalEntry { response: None, depth, input });
221221
v.insert(entry_index);
222222
}
223223
// We have a nested goal which relies on a goal `root` deeper in the stack.
@@ -243,23 +243,31 @@ impl<'tcx> SearchGraph<'tcx> {
243243
root.cycle_participants.insert(e.input);
244244
}
245245

246-
// NOTE: The goals on the stack aren't the only goals involved in this cycle.
247-
// We can also depend on goals which aren't part of the stack but coinductively
248-
// depend on the stack themselves. We already checked whether all the goals
249-
// between these goals and their root on the stack. This means that as long as
250-
// each goal in a cycle is checked for coinductivity by itself, simply checking
251-
// the stack is enough.
252-
if self.stack.raw[stack_depth.index()..]
253-
.iter()
254-
.all(|g| g.input.value.goal.predicate.is_coinductive(tcx))
255-
{
256-
// If we're in a coinductive cycle, we have to retry proving the current goal
257-
// until we reach a fixpoint.
258-
self.stack[stack_depth].has_been_used = true;
259-
return cache.provisional_result(entry_index);
246+
// If we're in a cycle, we have to retry proving the current goal
247+
// until we reach a fixpoint.
248+
self.stack[stack_depth].has_been_used = true;
249+
return if let Some(result) = cache.provisional_result(entry_index) {
250+
result
260251
} else {
261-
return Self::response_no_constraints(tcx, input, Certainty::OVERFLOW);
262-
}
252+
// If we don't have a provisional result yet, the goal has to
253+
// still be on the stack.
254+
let mut goal_on_stack = false;
255+
let mut is_coinductive = true;
256+
for entry in self.stack.raw[stack_depth.index()..]
257+
.iter()
258+
.skip_while(|entry| entry.input != input)
259+
{
260+
goal_on_stack = true;
261+
is_coinductive &= entry.input.value.goal.predicate.is_coinductive(tcx);
262+
}
263+
debug_assert!(goal_on_stack);
264+
265+
if is_coinductive {
266+
Self::response_no_constraints(tcx, input, Certainty::Yes)
267+
} else {
268+
Self::response_no_constraints(tcx, input, Certainty::OVERFLOW)
269+
}
270+
};
263271
}
264272
}
265273

@@ -288,15 +296,18 @@ impl<'tcx> SearchGraph<'tcx> {
288296
let provisional_entry_index =
289297
*cache.lookup_table.get(&stack_entry.input).unwrap();
290298
let provisional_entry = &mut cache.entries[provisional_entry_index];
291-
let prev_response = mem::replace(&mut provisional_entry.response, response);
292-
if stack_entry.has_been_used && prev_response != response {
293-
// If so, remove all entries whose result depends on this goal
294-
// from the provisional cache...
299+
if stack_entry.has_been_used
300+
&& provisional_entry.response.map_or(true, |r| r != response)
301+
{
302+
// If so, update the provisional result for this goal and remove
303+
// all entries whose result depends on this goal from the provisional
304+
// cache...
295305
//
296-
// That's not completely correct, as a nested goal can also
306+
// That's not completely correct, as a nested goal can also only
297307
// depend on a goal which is lower in the stack so it doesn't
298308
// actually depend on the current goal. This should be fairly
299309
// rare and is hopefully not relevant for performance.
310+
provisional_entry.response = Some(response);
300311
#[allow(rustc::potential_query_instability)]
301312
cache.lookup_table.retain(|_key, index| *index <= provisional_entry_index);
302313
cache.entries.truncate(provisional_entry_index.index() + 1);
@@ -315,8 +326,8 @@ impl<'tcx> SearchGraph<'tcx> {
315326
});
316327

317328
// We're now done with this goal. In case this goal is involved in a larger cycle
318-
// do not remove it from the provisional cache and do not add it to the global
319-
// cache.
329+
// do not remove it from the provisional cache and update its provisional result.
330+
// We only add the root of cycles to the global cache.
320331
//
321332
// It is not possible for any nested goal to depend on something deeper on the
322333
// stack, as this would have also updated the depth of the current goal.
@@ -348,6 +359,8 @@ impl<'tcx> SearchGraph<'tcx> {
348359
dep_node,
349360
result,
350361
)
362+
} else {
363+
provisional_entry.response = Some(result);
351364
}
352365

353366
result

‎tests/ui/traits/new-solver/cycles/double-cycle-inductive-coinductive.rs

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
// compile-flags: -Ztrait-solver=next
2+
#![feature(rustc_attrs)]
3+
4+
// Test that having both an inductive and a coinductive cycle
5+
// is handled correctly.
6+
7+
#[rustc_coinductive]
8+
trait Trait {}
9+
impl<T: Inductive + Coinductive> Trait for T {}
10+
11+
trait Inductive {}
12+
impl<T: Trait> Inductive for T {}
13+
#[rustc_coinductive]
14+
trait Coinductive {}
15+
impl<T: Trait> Coinductive for T {}
16+
17+
fn impls_trait<T: Trait>() {}
18+
19+
#[rustc_coinductive]
20+
trait TraitRev {}
21+
impl<T: CoinductiveRev + InductiveRev> TraitRev for T {}
22+
23+
trait InductiveRev {}
24+
impl<T: TraitRev> InductiveRev for T {}
25+
#[rustc_coinductive]
26+
trait CoinductiveRev {}
27+
impl<T: TraitRev> CoinductiveRev for T {}
28+
29+
fn impls_trait_rev<T: TraitRev>() {}
30+
31+
fn main() {
32+
impls_trait::<()>();
33+
//~^ ERROR overflow evaluating the requirement
34+
35+
impls_trait_rev::<()>();
36+
//~^ ERROR overflow evaluating the requirement
37+
}

‎tests/ui/traits/new-solver/cycles/double-cycle-inductive-coinductive.stderr

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
error[E0275]: overflow evaluating the requirement `(): Trait`
2+
--> $DIR/double-cycle-inductive-coinductive.rs:32:5
3+
|
4+
LL | impls_trait::<()>();
5+
| ^^^^^^^^^^^^^^^^^
6+
|
7+
= help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`double_cycle_inductive_coinductive`)
8+
note: required by a bound in `impls_trait`
9+
--> $DIR/double-cycle-inductive-coinductive.rs:17:19
10+
|
11+
LL | fn impls_trait<T: Trait>() {}
12+
| ^^^^^ required by this bound in `impls_trait`
13+
14+
error[E0275]: overflow evaluating the requirement `(): TraitRev`
15+
--> $DIR/double-cycle-inductive-coinductive.rs:35:5
16+
|
17+
LL | impls_trait_rev::<()>();
18+
| ^^^^^^^^^^^^^^^^^^^^^
19+
|
20+
= help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`double_cycle_inductive_coinductive`)
21+
note: required by a bound in `impls_trait_rev`
22+
--> $DIR/double-cycle-inductive-coinductive.rs:29:23
23+
|
24+
LL | fn impls_trait_rev<T: TraitRev>() {}
25+
| ^^^^^^^^ required by this bound in `impls_trait_rev`
26+
27+
error: aborting due to 2 previous errors
28+
29+
For more information about this error, try `rustc --explain E0275`.

‎tests/ui/traits/new-solver/cycles/inductive-cycle-but-err.rs

+48
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// compile-flags: -Ztrait-solver=next
2+
#![feature(trivial_bounds, marker_trait_attr)]
3+
#![allow(trivial_bounds)]
4+
// This previously triggered a bug in the provisional cache.
5+
//
6+
// This has the proof tree
7+
// - `MultipleCandidates: Trait` proven via impl-one
8+
// - `MultipleNested: Trait` via impl
9+
// - `MultipleCandidates: Trait` (inductive cycle ~> OVERFLOW)
10+
// - `DoesNotImpl: Trait` (ERR)
11+
// - `MultipleCandidates: Trait` proven via impl-two
12+
// - `MultipleNested: Trait` (in provisional cache ~> OVERFLOW)
13+
//
14+
// We previously incorrectly treated the `MultipleCandidates: Trait` as
15+
// overflow because it was in the cache and reached via an inductive cycle.
16+
// It should be `NoSolution`.
17+
18+
struct MultipleCandidates;
19+
struct MultipleNested;
20+
struct DoesNotImpl;
21+
22+
#[marker]
23+
trait Trait {}
24+
25+
// impl-one
26+
impl Trait for MultipleCandidates
27+
where
28+
MultipleNested: Trait
29+
{}
30+
31+
// impl-two
32+
impl Trait for MultipleCandidates
33+
where
34+
MultipleNested: Trait,
35+
{}
36+
37+
impl Trait for MultipleNested
38+
where
39+
MultipleCandidates: Trait,
40+
DoesNotImpl: Trait,
41+
{}
42+
43+
fn impls_trait<T: Trait>() {}
44+
45+
fn main() {
46+
impls_trait::<MultipleCandidates>();
47+
//~^ ERROR the trait bound `MultipleCandidates: Trait` is not satisfied
48+
}

‎tests/ui/traits/new-solver/cycles/inductive-cycle-but-err.stderr

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
error[E0277]: the trait bound `MultipleCandidates: Trait` is not satisfied
2+
--> $DIR/inductive-cycle-but-err.rs:46:19
3+
|
4+
LL | impls_trait::<MultipleCandidates>();
5+
| ^^^^^^^^^^^^^^^^^^ the trait `Trait` is not implemented for `MultipleCandidates`
6+
|
7+
= help: the trait `Trait` is implemented for `MultipleCandidates`
8+
note: required by a bound in `impls_trait`
9+
--> $DIR/inductive-cycle-but-err.rs:43:19
10+
|
11+
LL | fn impls_trait<T: Trait>() {}
12+
| ^^^^^ required by this bound in `impls_trait`
13+
14+
error: aborting due to previous error
15+
16+
For more information about this error, try `rustc --explain E0277`.

‎tests/ui/traits/new-solver/cycles/inductive-cycle-but-ok.rs

+44
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// compile-flags: -Ztrait-solver=next
2+
// check-pass
3+
#![feature(trivial_bounds, marker_trait_attr)]
4+
#![allow(trivial_bounds)]
5+
6+
// This previously triggered a bug in the provisional cache.
7+
//
8+
// This has the proof tree
9+
// - `Root: Trait` proven via impl
10+
// - `MultipleCandidates: Trait`
11+
// - candidate: overflow-impl
12+
// - `Root: Trait` (inductive cycle ~> OVERFLOW)
13+
// - candidate: trivial-impl ~> YES
14+
// - merge respones ~> YES
15+
// - `MultipleCandidates: Trait` (in provisional cache ~> OVERFLOW)
16+
//
17+
// We previously incorrectly treated the `MultipleCandidates: Trait` as
18+
// overflow because it was in the cache and reached via an inductive cycle.
19+
// It should be `YES`.
20+
21+
struct Root;
22+
struct MultipleCandidates;
23+
24+
#[marker]
25+
trait Trait {}
26+
impl Trait for Root
27+
where
28+
MultipleCandidates: Trait,
29+
MultipleCandidates: Trait,
30+
{}
31+
32+
// overflow-impl
33+
impl Trait for MultipleCandidates
34+
where
35+
Root: Trait,
36+
{}
37+
// trivial-impl
38+
impl Trait for MultipleCandidates {}
39+
40+
fn impls_trait<T: Trait>() {}
41+
42+
fn main() {
43+
impls_trait::<Root>();
44+
}

‎tests/ui/traits/new-solver/cycles/inductive-cycle-discarded-coinductive-constraints.rs

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// check-pass
2+
// compile-flags: -Ztrait-solver=next
3+
#![feature(rustc_attrs, marker_trait_attr)]
4+
#[rustc_coinductive]
5+
trait Trait {}
6+
7+
impl<T, U> Trait for (T, U)
8+
where
9+
(U, T): Trait,
10+
(T, U): Inductive,
11+
(): ConstrainToU32<T>,
12+
{}
13+
14+
trait ConstrainToU32<T> {}
15+
impl ConstrainToU32<u32> for () {}
16+
17+
// We only prefer the candidate without an inductive cycle
18+
// once the inductive cycle has the same constraints as the
19+
// other goal.
20+
#[marker]
21+
trait Inductive {}
22+
impl<T, U> Inductive for (T, U)
23+
where
24+
(T, U): Trait,
25+
{}
26+
27+
impl Inductive for (u32, u32) {}
28+
29+
fn impls_trait<T, U>()
30+
where
31+
(T, U): Trait,
32+
{}
33+
34+
fn main() {
35+
impls_trait::<_, _>();
36+
}

‎tests/ui/traits/new-solver/cycles/inductive-not-on-stack.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ fn impls_ar<T: AR>() {}
3939

4040
fn main() {
4141
impls_a::<()>();
42-
//~^ ERROR overflow evaluating the requirement `(): A`
42+
// FIXME(-Ztrait-solver=next): This is broken and should error.
4343

4444
impls_ar::<()>();
4545
//~^ ERROR overflow evaluating the requirement `(): AR`

‎tests/ui/traits/new-solver/cycles/inductive-not-on-stack.stderr

+1-14
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,3 @@
1-
error[E0275]: overflow evaluating the requirement `(): A`
2-
--> $DIR/inductive-not-on-stack.rs:41:5
3-
|
4-
LL | impls_a::<()>();
5-
| ^^^^^^^^^^^^^
6-
|
7-
= help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`inductive_not_on_stack`)
8-
note: required by a bound in `impls_a`
9-
--> $DIR/inductive-not-on-stack.rs:25:15
10-
|
11-
LL | fn impls_a<T: A>() {}
12-
| ^ required by this bound in `impls_a`
13-
141
error[E0275]: overflow evaluating the requirement `(): AR`
152
--> $DIR/inductive-not-on-stack.rs:44:5
163
|
@@ -24,6 +11,6 @@ note: required by a bound in `impls_ar`
2411
LL | fn impls_ar<T: AR>() {}
2512
| ^^ required by this bound in `impls_ar`
2613

27-
error: aborting due to 2 previous errors
14+
error: aborting due to previous error
2815

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

‎tests/ui/traits/new-solver/leak-check-coinductive-cycle.rs ‎tests/ui/traits/new-solver/cycles/leak-check-coinductive-cycle.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
1-
// check-pass
21
// compile-flags: -Ztrait-solver=next
2+
// check-pass
33
#![feature(rustc_attrs)]
44

55
#[rustc_coinductive]

‎tests/ui/traits/new-solver/dyn-any-dont-prefer-impl.rs

+4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
// compile-flags: -Ztrait-solver=next
22
// check-pass
33

4+
// Test that selection prefers the builtin trait object impl for `Any`
5+
// instead of the user defined impl. Both impls apply to the trait
6+
// object.
7+
48
use std::any::Any;
59

610
fn needs_usize(_: &usize) {}

0 commit comments

Comments
 (0)
Please sign in to comment.