Skip to content

Return early when treating ambiguous cycles #48

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

Merged
merged 2 commits into from
Jun 15, 2017
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
20 changes: 14 additions & 6 deletions src/solve/solver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,12 +183,20 @@ impl Solver {
match self.cycle_strategy {
CycleStrategy::Tabling if slot.cycle => {
let actual_answer = result.as_ref().ok().map(|s| s.clone());
if actual_answer == answer {
// Fixed point: break
return result;
} else {
answer = actual_answer;
}
let fixed_point = answer == actual_answer;

// If we reach a fixed point, we can break.
// If the answer is `Ambig`, then we know that we already have multiple
// solutions, and we *must* break because an `Ambig` solution may not perform
// any unification and thus fail to correctly reach a fixed point. See test
// `multiple_ambiguous_cycles`.
match (fixed_point, &actual_answer) {
(_, &Some(Solution::Ambig(_))) | (true, _) =>
return result,
_ => ()
};

answer = actual_answer;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to make a test for this? or is that test basically just in your PR for #12?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that's funny because in fact if I provide a test for that and deactivate the fix, the test program will not even load because of the overlap check which will loop forever. So I was indeed relying on this behaviour to test the fix in my PR for #12 (the trait_wf test would not load without the fix). Still I added a general test in this PR, which is better for documentation.

}
_ => return result,
};
Expand Down
46 changes: 46 additions & 0 deletions src/solve/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,52 @@ fn cycle_unique_solution() {
}
}

#[test]
fn multiple_ambiguous_cycles() {
test! {
program {
trait WF { }
trait Sized { }

struct Vec<T> { }
struct Int { }

impl Sized for Int { }
impl WF for Int { }

impl<T> WF for Vec<T> where T: Sized { }
impl<T> Sized for Vec<T> where T: WF, T: Sized { }
}

// ?T: WF
// |
// |
// |
// Int: WF. <-----> (Vec<?T>: WF) :- (?T: Sized)
// |
// |
// |
// Int: Sized. <-------> (Vec<?T>: Sized) :- (?T: Sized), (?T: WF)
// | |
// | |
// | |
// cycle cycle
//
// Depending on the evaluation order of the above tree (which cycle we come upon first),
// we may fail to reach a fixed point if we loop continuously because `Ambig` does not perform
// any unification. We must stop looping as soon as we encounter `Ambig`. In fact without
// this strategy, the above program will not even be loaded because of the overlap check which
// will loop forever.
goal {
exists<T> {
T: WF
}
} yields {
"Ambig"
}
}
}

#[test]
fn normalize_basic() {
test! {
Expand Down