Skip to content

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

Merged
merged 12 commits into from
Apr 24, 2025

Conversation

maximebuyse
Copy link
Contributor

@maximebuyse maximebuyse commented Mar 25, 2025

This PR adds invariants to while loops (using the macro hax_lib::loop_invariant that is already used in for loops). It also adds a macro to prove termination (hax_lib::loop_decreases), that takes a decreasing hax_lib::Int .

@maximebuyse maximebuyse force-pushed the while-loops-invariant-termination branch from 43d59aa to ffcd6b5 Compare April 14, 2025 07:31
@maximebuyse maximebuyse marked this pull request as ready for review April 14, 2025 07:34
@maximebuyse maximebuyse requested a review from a team as a code owner April 14, 2025 07:34
@maximebuyse maximebuyse requested a review from W95Psp April 14, 2025 07:35
Copy link
Collaborator

@W95Psp W95Psp left a 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_variant and extract_loop_invariant, instead have a extract_loop_annotation that takes out a let _ = ..; body and 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 ().

@maximebuyse
Copy link
Contributor Author

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_variant` and `extract_loop_invariant`, instead have a `extract_loop_annotation` that takes out a `let _ = ..; body` and 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 nat versus <<, I tried that at first but it seemed to be a problem for signed integer types, but maybe I did something wrong. I modified the macro so that the conversion to a hax_lib int is automatically inserted from any rust integer expression, so this should be nice to use. Maybe switching to << can be kept for later as a possible improvement?

@maximebuyse maximebuyse requested a review from W95Psp April 22, 2025 16:07
@karthikbhargavan
Copy link
Contributor

This PR is needed for some applications. Let's get this in?

Copy link
Collaborator

@W95Psp W95Psp left a 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.

@maximebuyse maximebuyse enabled auto-merge April 23, 2025 13:19
@maximebuyse
Copy link
Contributor Author

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.

@maximebuyse maximebuyse added this pull request to the merge queue Apr 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 23, 2025
@maximebuyse
Copy link
Contributor Author

Two jobs failed in the merge queue:

  • Bertie: this one seems to be failing for all PRs recently so this is probably not a problem
  • ml-kem: fails because we have a breaking change in how while loops are encoded, but I checked and using both hax and hax-lib from this PR it lax-checks.
    @W95Psp Should we force the merge?

@karthikbhargavan
Copy link
Contributor

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.

@W95Psp
Copy link
Collaborator

W95Psp commented Apr 23, 2025

@maximebuyse
Copy link
Contributor Author

I spawned https://github.com/cryspen/libcrux/actions/runs/14623000394

This failed but it also uses hax-lib from main so that is expected

@maximebuyse
Copy link
Contributor Author

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.

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?

@karthikbhargavan
Copy link
Contributor

Ok, after reviewing Bertie failure. I agree that this can be merged.

@karthikbhargavan karthikbhargavan merged commit d676e96 into main Apr 24, 2025
16 checks passed
@karthikbhargavan karthikbhargavan deleted the while-loops-invariant-termination branch April 24, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants