diff --git a/chalk-recursive/src/combine.rs b/chalk-recursive/src/combine.rs index a1bcde72b94..682af6d3bef 100644 --- a/chalk-recursive/src/combine.rs +++ b/chalk-recursive/src/combine.rs @@ -4,6 +4,7 @@ use tracing::debug; use chalk_ir::interner::Interner; use chalk_ir::{ClausePriority, DomainGoal, GenericArg}; +#[tracing::instrument(level = "Debug", skip(interner))] pub(super) fn with_priorities( interner: I, domain_goal: &DomainGoal, @@ -12,7 +13,7 @@ pub(super) fn with_priorities( b: Solution, prio_b: ClausePriority, ) -> (Solution, ClausePriority) { - match (prio_a, prio_b, a, b) { + let result = match (prio_a, prio_b, a, b) { (ClausePriority::High, ClausePriority::Low, higher, lower) | (ClausePriority::Low, ClausePriority::High, lower, higher) => { // if we have a high-priority solution and a low-priority solution, @@ -34,7 +35,9 @@ pub(super) fn with_priorities( } } (_, _, a, b) => (a.combine(b, interner), prio_a), - } + }; + debug!(?result, "combined result"); + result } fn calculate_inputs( diff --git a/chalk-recursive/src/solve.rs b/chalk-recursive/src/solve.rs index c5c4adbb070..47db7080ab3 100644 --- a/chalk-recursive/src/solve.rs +++ b/chalk-recursive/src/solve.rs @@ -154,11 +154,6 @@ trait SolveIterationHelpers: SolveDatabase { for program_clause in clauses { debug_span!("solve_from_clauses", clause = ?program_clause); - // If we have a completely ambiguous answer, it's not going to get better, so stop - if cur_solution == Some((Solution::Ambig(Guidance::Unknown), ClausePriority::High)) { - return Ok(Solution::Ambig(Guidance::Unknown)); - } - let ProgramClauseData(implication) = program_clause.data(self.interner()); let infer = infer.clone(); let subst = subst.clone(); @@ -184,11 +179,19 @@ trait SolveIterationHelpers: SolveDatabase { } else { debug!("Error"); } + + if let Some((cur_solution, _)) = &cur_solution { + if cur_solution.is_trivial_and_always_true(self.interner()) { + break; + } + } } if let Some((s, _)) = cur_solution { + debug!("solve_from_clauses: result = {:?}", s); Ok(s) } else { + debug!("solve_from_clauses: error"); Err(NoSolution) } } diff --git a/chalk-solve/src/solve.rs b/chalk-solve/src/solve.rs index 96ef7ce51af..0734fc53ea3 100644 --- a/chalk-solve/src/solve.rs +++ b/chalk-solve/src/solve.rs @@ -44,23 +44,23 @@ impl Solution { /// There are multiple candidate solutions, which may or may not agree on /// the values for existential variables; attempt to combine them. This /// operation does not depend on the order of its arguments. - // - // This actually isn't as precise as it could be, in two ways: - // - // a. It might be that while there are multiple distinct candidates, they - // all agree about *some things*. To be maximally precise, we would - // compute the intersection of what they agree on. It's not clear though - // that this is actually what we want Rust's inference to do, and it's - // certainly not what it does today. - // - // b. There might also be an ambiguous candidate and a successful candidate, - // both with the same refined-goal. In that case, we could probably claim - // success, since if the conditions of the ambiguous candidate were met, - // we know the success would apply. Example: `?0: Clone` yields ambiguous - // candidate `Option: Clone` and successful candidate `Option: - // Clone`. - // - // But you get the idea. + /// + /// This actually isn't as precise as it could be, in two ways: + /// + /// a. It might be that while there are multiple distinct candidates, they + /// all agree about *some things*. To be maximally precise, we would + /// compute the intersection of what they agree on. It's not clear though + /// that this is actually what we want Rust's inference to do, and it's + /// certainly not what it does today. + /// + /// b. There might also be an ambiguous candidate and a successful candidate, + /// both with the same refined-goal. In that case, we could probably claim + /// success, since if the conditions of the ambiguous candidate were met, + /// we know the success would apply. Example: `?0: Clone` yields ambiguous + /// candidate `Option: Clone` and successful candidate `Option: + /// Clone`. + /// + /// But you get the idea. pub fn combine(self, other: Solution, interner: I) -> Solution { use self::Guidance::*; @@ -68,6 +68,19 @@ impl Solution { return self; } + // Special case hack: if one solution is "true" without any constraints, + // that is always the combined result. + // + // This is not as general as it could be: ideally, if we had one solution + // that is Unique with a simpler substitution than the other one, or region constraints + // which are a subset, we'd combine them. + if self.is_trivial_and_always_true(interner) { + return self; + } + if other.is_trivial_and_always_true(interner) { + return other; + } + debug!( "combine {} with {}", self.display(interner), @@ -88,6 +101,16 @@ impl Solution { Solution::Ambig(guidance) } + pub fn is_trivial_and_always_true(&self, interner: I) -> bool { + match self { + Solution::Unique(constrained_subst) => { + constrained_subst.value.subst.is_identity_subst(interner) + && constrained_subst.value.constraints.is_empty(interner) + } + Solution::Ambig(_) => false, + } + } + /// View this solution purely in terms of type inference guidance pub fn into_guidance(self) -> Guidance { match self { diff --git a/tests/test/ambiguity_issue_727.rs b/tests/test/ambiguity_issue_727.rs new file mode 100644 index 00000000000..6c8981fac38 --- /dev/null +++ b/tests/test/ambiguity_issue_727.rs @@ -0,0 +1,133 @@ +use super::*; + +#[test] +fn issue_727_1() { + test!( + program { + #[upstream] #[non_enumerable] #[lang(sized)] + trait Sized {} + + #[non_enumerable] #[object_safe] + trait Database {} + + #[non_enumerable] + trait QueryGroup + where + Self: Sized, + { + type DynDb: Database + HasQueryGroup; + } + + #[non_enumerable] #[object_safe] + trait HasQueryGroup + where + Self: Database, + G: QueryGroup, + G: Sized, + { } + + #[non_enumerable] #[object_safe] + trait HelloWorld + where + Self: HasQueryGroup, + { } + + struct HelloWorldStorage {} + + impl QueryGroup for HelloWorldStorage { + type DynDb = dyn HelloWorld + 'static; + } + impl HelloWorld for DB + where + DB: Database, + DB: HasQueryGroup, + DB: Sized, + { } + } + + goal { + forall { + if (FromEnv(T: Database); FromEnv(T: HasQueryGroup); FromEnv(T: Sized)) { + T: HelloWorld + } + } + } yields[SolverChoice::slg_default()] { // ok + expect![["Unique"]] + } yields[SolverChoice::recursive_default()] { // fails: "Ambiguous; no inference guidance" + expect![["Unique"]] + } + ); +} + +#[test] +fn issue_727_2() { + test!( + program { + #[non_enumerable] #[object_safe] + trait Database {} + + #[non_enumerable] + trait QueryGroup + { + type DynDb: Database + HasQueryGroup; + } + + #[non_enumerable] #[object_safe] + trait HasQueryGroup + where + Self: Database, + G: QueryGroup, + { } + + struct HelloWorldStorage {} + + impl QueryGroup for HelloWorldStorage { + type DynDb = dyn HasQueryGroup + 'static; + } + } + + goal { + forall { + if (FromEnv(T: HasQueryGroup)) { + T: Database + } + } + } yields[SolverChoice::slg_default()] { + expect![["Unique"]] + } yields[SolverChoice::recursive_default()] { + expect![[r#"Ambiguous; no inference guidance"#]] // FIXME rust-lang/chalk#727 + } + ); +} + +#[test] +fn issue_727_3() { + test!( + program { + #[non_enumerable] + trait Database {} + + #[non_enumerable] + trait HasQueryGroup + where + Self: Database, + { } + + struct HelloWorldStorage {} + + impl Database for HelloWorldStorage { } + } + + goal { + forall { + if (FromEnv(HelloWorldStorage: HasQueryGroup); FromEnv(HelloWorldStorage: HasQueryGroup)) { + HelloWorldStorage: Database + } + } + } yields[SolverChoice::slg_default()] { + expect![["Unique"]] + } yields[SolverChoice::recursive_default()] { + expect![["Unique"]] + } + ); +} diff --git a/tests/test/misc.rs b/tests/test/misc.rs index 198c3c437ee..e96e5ef1191 100644 --- a/tests/test/misc.rs +++ b/tests/test/misc.rs @@ -749,7 +749,7 @@ fn empty_definite_guidance() { } yields[SolverChoice::slg_default()] { expect![["Unique"]] } yields[SolverChoice::recursive_default()] { - expect![["Ambiguous; suggested substitution []"]] + expect![[r#"Unique"#]] } } } @@ -812,8 +812,10 @@ fn env_bound_vars() { WellFormed(&'a ()) } } - } yields { + } yields[SolverChoice::slg_default()] { expect![["Ambiguous; definite substitution for { [?0 := '^0.0] }"]] + } yields[SolverChoice::recursive_default()] { + expect![[r#"Unique; for { substitution [?0 := '^0.0] }"#]] } goal { exists<'a> { @@ -841,7 +843,7 @@ fn recursive_hang() { } yields[SolverChoice::slg_default()] { expect![["Ambiguous; definite substitution for { [?0 := ^0.0, ?1 := '^0.1] }"]] } yields[SolverChoice::recursive_default()] { - expect![["Ambiguous; suggested substitution for { [?0 := ^0.0, ?1 := '^0.1] }"]] + expect![[r#"Ambiguous; no inference guidance"#]] } } } diff --git a/tests/test/mod.rs b/tests/test/mod.rs index 824b00cd598..423e7e954e1 100644 --- a/tests/test/mod.rs +++ b/tests/test/mod.rs @@ -394,16 +394,17 @@ fn solve_aggregated( .expect("Test requires at least one solver"); for (i, other) in tail.iter().enumerate() { println!( - "\ncomparing solvers:\n\tleft: {:?}\n\tright: {:?}\n", + "\ncomparing solvers:\n\texpected: {:?}\n\tactual: {:?}\n", &choices[0], &choices[i + 1] ); - assert_same(head, other); + assert_same(other, head); } expected.assert_eq(head); } +mod ambiguity_issue_727; mod arrays; mod auto_traits; mod closures;