-
Notifications
You must be signed in to change notification settings - Fork 71
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
Replace height intrinsic with is_smaller_than and make height a partial order #570
Conversation
and in the SMT encoding, encode height as a Z3 partial order rather than an int to allow decreases on infinite maps.
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'm a little confused about how tuples work. Is the lexicographic ordering of tuples consistent with the height-ordering of tuples?
If I write is_smaller_than((a, b), (c, d))
then it looks like rust_to_vir_expr lexicographic ordering, but we can also talk about the partial-order on tuple objects themselves:
let x = (a, b);
let y = (c, d);
is_smaller_than(x, y)
Is this equivalent to is_smaller_than((a, b), (c, d))
?
LinearOrder, | ||
TreeOrder, | ||
PiecewiseLinearOrder, | ||
} |
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 could use some documentation
@@ -59,7 +67,7 @@ pub enum BinaryOp { | |||
Gt, | |||
EuclideanDiv, | |||
EuclideanMod, | |||
|
|||
Relation(Relation, u64), |
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.
same - what's the u64 for?
It's not equivalent. Maybe the larger question is what the syntax for |
Perhaps we should just have |
FYI, I made this change to help smoothen the transition to z3 4.12.2 (and for future version updates): 6f5342c (adds a message that points the user to |
} else { | ||
(vec![args[0]], vec![args[1]]) | ||
}; | ||
// convert is_smaller_than((x0, y0, z0), (x1, y1, z1)) into |
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.
Can you please extract this conversion into a separate function?
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.
Thank you! Having is_smaller_than(_lexicographic)
will hopefully make debugging failing termination proof easier.
Looks good to me, modulo @tjhance's and my review comments.
I added a restriction similar to FStarLang/FStar#2954 . I think this should be ready to merge into main now. |
This pull request allows decreases that go through
Seq
,Map
,FnSpec
, and user-defined abstract datatypes. For example:The new
is_smaller_than(a, b)
intrinsic expresses thatb
can decrease toa
in a decreases clause:is_smaller_than
also works on lexicographic tuples, which can be useful for debugging termination failures:is_smaller_than
replaces the previousheight
intrinsic.height
is still used in the SMT encoding, but heights are no longer integers, and are instead Z3 partial orders. This allows the SMT encoding to express the heights of infinite maps andFnSpec
functions.Partial orders are a new feature in Z3, so this pull request requires upgrading to the latest Z3 release (4.12.2).