-
Notifications
You must be signed in to change notification settings - Fork 45
Add invariants for while loops. #1375
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
43d59aa to
ffcd6b5
Compare
W95Psp
left a comment
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.
Thanks, as we discussed there's a few thing we should change (rought notes from what we cahtted about):
- make the loop invariant macro produce two different calls to hax_lib according to wether we gave it a closure or not
- add a
loop_annotation_kind type e.g.| LoopInvariant of { index_pat: pat option } | LoopDecreaseMeasure` - remove
extract_loop_variantandextract_loop_invariant, instead have aextract_loop_annotationthat takes out alet _ = ..; bodyand produces(expr * expr * loop_annotation_kind) - we should accept annotations in any order
Also, why is the decrease clause nat? You can use the << operator in F* too reason about decreasing of any type. Then the default decreasing clause can be ().
Thanks, I pushed a new version which resolves all 4 points. About |
|
This PR is needed for some applications. Let's get this in? |
W95Psp
left a comment
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.
Thanks!
I have two last comments, but mainly about style and readability.
If you can improve the code there a bit that'd be nice.
Thanks, I made some style improvements where you pointed and it is indeed much nicer. Merging now. |
|
Two jobs failed in the merge queue:
|
|
Let's revisit this again tomorrow. I would like to understand the bertie failure, and if it is an atomic update issue for ML-KEM, yes we can force the merge here. |
This failed but it also uses hax-lib from main so that is expected |
The Bertie failure is a dependency issue that I documented in cryspen/bertie#151. This is unrelated, should we wait for this to be fixed or force the merge anyway? |
|
Ok, after reviewing Bertie failure. I agree that this can be merged. |
This PR adds invariants to
whileloops (using the macrohax_lib::loop_invariantthat is already used in for loops). It also adds a macro to prove termination (hax_lib::loop_decreases), that takes a decreasinghax_lib::Int.