Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

we only need to prove things one way #754

Merged
merged 6 commits into from
Apr 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions chalk-recursive/src/combine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<I: Interner>(
interner: I,
domain_goal: &DomainGoal<I>,
Expand All @@ -12,7 +13,7 @@ pub(super) fn with_priorities<I: Interner>(
b: Solution<I>,
prio_b: ClausePriority,
) -> (Solution<I>, 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,
Expand All @@ -34,7 +35,9 @@ pub(super) fn with_priorities<I: Interner>(
}
}
(_, _, a, b) => (a.combine(b, interner), prio_a),
}
};
debug!(?result, "combined result");
result
}

fn calculate_inputs<I: Interner>(
Expand Down
13 changes: 8 additions & 5 deletions chalk-recursive/src/solve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,6 @@ trait SolveIterationHelpers<I: Interner>: SolveDatabase<I> {
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
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
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();
Expand All @@ -184,11 +179,19 @@ trait SolveIterationHelpers<I: Interner>: SolveDatabase<I> {
} 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)
}
}
Expand Down
57 changes: 40 additions & 17 deletions chalk-solve/src/solve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,30 +44,43 @@ impl<I: Interner> Solution<I> {
/// 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<?0>: Clone` and successful candidate `Option<?0>:
// 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<?0>: Clone` and successful candidate `Option<?0>:
/// Clone`.
///
/// But you get the idea.
pub fn combine(self, other: Solution<I>, interner: I) -> Solution<I> {
use self::Guidance::*;

if self == other {
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),
Expand All @@ -88,6 +101,16 @@ impl<I: Interner> Solution<I> {
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<I> {
match self {
Expand Down
133 changes: 133 additions & 0 deletions tests/test/ambiguity_issue_727.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
use super::*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like I'd like to see these tests more properly categorized. But I don't know how strongly I feel about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense-- but I'm not sure where to categorize them!


#[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<Self>;
}

#[non_enumerable] #[object_safe]
trait HasQueryGroup<G>
where
Self: Database,
G: QueryGroup,
G: Sized,
{ }

#[non_enumerable] #[object_safe]
trait HelloWorld
where
Self: HasQueryGroup<HelloWorldStorage>,
{ }

struct HelloWorldStorage {}

impl QueryGroup for HelloWorldStorage {
type DynDb = dyn HelloWorld + 'static;
}
impl<DB> HelloWorld for DB
where
DB: Database,
DB: HasQueryGroup<HelloWorldStorage>,
DB: Sized,
{ }
}

goal {
forall<T> {
if (FromEnv(T: Database); FromEnv(T: HasQueryGroup<HelloWorldStorage>); 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<Self>;
}

#[non_enumerable] #[object_safe]
trait HasQueryGroup<G>
where
Self: Database,
G: QueryGroup,
{ }

struct HelloWorldStorage {}

impl QueryGroup for HelloWorldStorage {
type DynDb = dyn HasQueryGroup<HelloWorldStorage> + 'static;
}
}

goal {
forall<T> {
if (FromEnv(T: HasQueryGroup<HelloWorldStorage>)) {
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<G>
where
Self: Database,
{ }

struct HelloWorldStorage {}

impl Database for HelloWorldStorage { }
}

goal {
forall<T, S> {
if (FromEnv(HelloWorldStorage: HasQueryGroup<T>); FromEnv(HelloWorldStorage: HasQueryGroup<S>)) {
HelloWorldStorage: Database
}
}
} yields[SolverChoice::slg_default()] {
expect![["Unique"]]
} yields[SolverChoice::recursive_default()] {
expect![["Unique"]]
}
);
}
8 changes: 5 additions & 3 deletions tests/test/misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"#]]
}
}
}
Expand Down Expand Up @@ -812,8 +812,10 @@ fn env_bound_vars() {
WellFormed(&'a ())
}
}
} yields {
} yields[SolverChoice::slg_default()] {
expect![["Ambiguous; definite substitution for<?U0> { [?0 := '^0.0] }"]]
} yields[SolverChoice::recursive_default()] {
expect![[r#"Unique; for<?U0> { substitution [?0 := '^0.0] }"#]]
nikomatsakis marked this conversation as resolved.
Show resolved Hide resolved
}
goal {
exists<'a> {
Expand Down Expand Up @@ -841,7 +843,7 @@ fn recursive_hang() {
} yields[SolverChoice::slg_default()] {
expect![["Ambiguous; definite substitution for<?U0,?U0> { [?0 := ^0.0, ?1 := '^0.1] }"]]
} yields[SolverChoice::recursive_default()] {
expect![["Ambiguous; suggested substitution for<?U0,?U0> { [?0 := ^0.0, ?1 := '^0.1] }"]]
expect![[r#"Ambiguous; no inference guidance"#]]
}
Comment on lines 843 to 847
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are, I believe, equivalent. That substitution is an "identity substitution", so it's not very useful guidance. I looked into why this was coming about -- it's because I removed the (erroneous) "early exit" that was causing us to bail out early. What we now do is bail out because we see ?T: '?a for some unknown ?T and we can't make progress (in fact, we could see that it's in the environment, but the code doesn't do that today).

Comment on lines 843 to 847
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact these are still different bothers me. Also, SLG is still given the identity solution as "ambiguous", which is somewhat at odds with what the aggregate hack is saying, right?

}
}
5 changes: 3 additions & 2 deletions tests/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down