-
Couldn't load subscription status.
- Fork 10
New matching: no backtracking on SMT results #344
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
Conversation
|
I should add this means we can safely admit queries when checking pulse code, since they don't affect the behavior of the checker. Lax-checking is therefore reenabled. |
| cases_of_is_tree None l; | ||
| unfold is_tree_cases; | ||
| intro_is_tree_leaf x; | ||
| () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have run into this issue many times where you need to add an extra dummy statement to trigger the matcher. (Sometimes even let x = foo (); x...)
Could we fix this more generally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed here. I probably added it since when there's a failure in the last statement of a function there's no clear indication of whether it's the precondition that failed, or proving the function's postcondition. I want to fix that soon too, it's very annoying.
I'm not sure if I've seen an instance of what you mention on this branch, if you see it could you post an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let n = !p; | ||
| rewrite each node as n; | ||
| rewrite pts_to p n as pts_to (Some?.v tree) n; | ||
| // rewrite each ltree as tree.left; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover comment.
lib/pulse/lib/Pulse.Lib.AVLTree.fst
Outdated
| { | ||
| let new_left = insert_avl cmp n.left key; | ||
| vl := {data = n.data; left = new_left; right = n.right}; | ||
| admit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot an admit here.
lib/pulse/lib/Pulse.Lib.AVLTree.fst
Outdated
| { | ||
| let new_right = insert_avl cmp n.right key; | ||
| vl := {data = n.data; left = n.left; right = new_right}; | ||
| admit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot an admit here.
| match tree { | ||
| None -> { | ||
| is_tree_case_none None; | ||
| rewrite is_tree None 'l as is_tree tree 'l; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a common pattern in matches now. Can you do a rewrite each tree to None instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That rewrite is actually implicitly injected by the checker on a match, and now without the SMT it's kind of working against us. The state at the beginning of the branch is is_tree None ', so we lost the mention of tree, but we anyway need to prove a postcondition mentioning it.
I'm gonna try removing this automatic rewrite and see if things are better. We added it for compactness, but seems to be counterproductive now.
I suppose another possibly much nicer solution would be having the rewrite operate on the postcondition, but so far we don't do much to the postcondition at all.
| let unpacked c _v = pts_to c.r #0.5R true | ||
|
|
||
|
|
||
| #set-options "--print_implicits" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray debugging option.
| OR.on_range_put i j k #g | ||
| } | ||
|
|
||
| #set-options "--print_full_names --print_implicits" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray debugging option.
| fold (istar preds'); | ||
| fold (maybe_holds v q preds'); | ||
| rewrite (istar preds') as (maybe_holds v q preds'); | ||
| // fold (maybe_holds v q preds'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this no longer work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer my own question, it's because maybe_holds is a match. And so you need to tell the matcher which branch is true.
| } | ||
| Cons _ _ -> { | ||
| unfold is_list; | ||
| admit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot an admit here.
| rewrite each r._1 as p31; | ||
| rewrite each r._2 as p32; | ||
| rewrite each r._3 as pivot; | ||
| // ^FIXME: would be nicer to rewrite r as (p31, p32, pivot) but projectors don't unfold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed. Are you saying that (x, y)._1 doesn't unify with x? @nikswamy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not without unfolding, and we disable all unfolding during the unification checks performed by the matcher since we found them to go haywire previously. Probably something to revisit now.
I did add special support in Pulse to simplify (x,y)._1 ~> x for tuple2, I think I can extend this for all tuples and make this work, trying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I allowed some unfolding and now this is just let p31, p32, pivot = partition_wrapper a lo hi lb rb;
6ebf60f to
d8d96bb
Compare
Now that we do not backtrack on their result, there is no harm in doing laxer checking.
d8d96bb to
d6f13b8
Compare
d6f13b8 to
6273aae
Compare
without that simplification, Rust extraction needs to monomorphize fst and snd, and that does not work very well.
This changes the slprop matching procedure in the checker to never backtrack on the result of SMT queries, and instead decide "which path" to take only through syntactic and unification checks. We still allow using the SMT explicitly via rewrites, and also this introduces a notion of matching key to automatically allow the checker to generate SMT guards. As an example
marks the reference as a matching key, and if we ever have
pts_to r fooin the context and need to provepts_to r bar, the matcher will succeed generating a (delayed) guard forpts_to r foo == pts_to r bar, which we will later try to discharge. We also still perform ambiguity checks, so if we also hadpts_to r bazin the context, both matches would be "guarded" successes, and we would provide an error explaining the ambiguity:This impacts heavily on all existing Pulse code. All code in this repo is updated to work, but we should look at external repos.