-
Notifications
You must be signed in to change notification settings - Fork 126
Add loop-contracts support for for
loop
#4143
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
Add loop-contracts support for for
loop
#4143
Conversation
1dc30e6
to
a5d568b
Compare
6377782
to
1a8dceb
Compare
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 haven't reviewed the implementation yet, these are just some nits. I have two requests to make reviewing on my end easier:
- This is a big PR, and it'd be much easier to review if each new piece of functionality was separated into its own commit along with its tests. I'm envisioning something like:
commit 1: code to support iter & expected tests
commit 2: code to support array & expected tests
commit 3: code to support slice & expected tests
commit 4: code to support vec & expected tests
Can you do an interactive rebase to make it look like this? It doesn't need to be exactly this--I'd just appreciate if 1) changes are committed with their tests and 2) commits irrelevant to the review (e.g., fix clippy, merges) are kept to a minimum. FWIW, I have a pre-commit hook that runs clippy, our formatting check, and copyright check so that I can't even commit unless those checks pass; I'd recommend it.
- Can you add more comments? I added some places that I think need some. I'd like to see each function (unless it's super simple) have a doc comment that describes 1) what it does and 2) why it exists (i.e., why we need it in the first place)
Can you expand the PR description and/or the comments in |
2a22aaf
to
8e69667
Compare
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.
Please address my documentation requests.
33b34b2
to
746311a
Compare
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.
Should we really make all these changes from "invariant" to "contract"? I'm making a suggestion to explain how "loop contracts" and "loop invariants" are related, but I don't know whether others share my view on this matter.
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.
In short:
- Can this be done without the "clone" of a? I see no need for it, and possibly huge performance impact.
- The "60" bounds on b is wrong surely? b should have the same size as a.len(), right?
- Please demonstrate how auto-harness generation works on these functions, so the harness functions are not required.
…into forloopnew1
Co-authored-by: Michael Tautschnig <mt@debian.org>
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.
We may still need to follow up this work with some refinement, but this should be ready for broader evaluation.
a7d917d
This PR adds loop-contracts support for
for
loop for 12 types: array, slice, Iter, Vec, Range, StepBy, Map, Chain, Zip, Enumerate, Take, Rev.Main ideas:
for
loop implementation:A
for
loop of the formis transformed into a
while
loop:Note that
expr's type is required to
impl KaniIntoIter
trait, which is the overrideimpl
of Rust'sIntoIter
(see library/kani_core/src/iter.rs).Reason:
a) Reduce stack-call
b) Avoid types that cannot be havoc effectively (user cannot state the type invariant in the loop invariant due to private fields)
c) There is no generic way to handle Rust's
into_iter()
.The
init_index_stmt
andinit_pat_stmt
support writing the loop-invariant properties that involve pat and kaniindexThe
iter_assume_stmt
assumes some truths about allocation, so that the user does not need to specify them in the loop invariantBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.