-
Notifications
You must be signed in to change notification settings - Fork 84
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
For loops #954
For loops #954
Conversation
Note: I think |
source/vir/src/ast.rs
Outdated
@@ -254,6 +254,15 @@ pub enum UnaryOp { | |||
StrIsAscii, | |||
/// Used only for handling casts from chars to ints | |||
CharToInt, | |||
/// Given an exec/proof expression used to construct a loop iterator, | |||
/// try to infer a pure specification for the loop iterator. | |||
/// Return Some(spec) if successful, None otherwise. |
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.
What does it mean 'Return Some(spec) if successful, None otherwise'? This isn't documentation for a function, so what's doing the returning?
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 fixed it to say Evaluate to
pub struct VecGhostIterCopy<'a, T> { | ||
pub seq: Seq<T>, | ||
pub cur: int, | ||
pub unused_phantom_dummy: &'a Vec<T>, // use the 'a parameter, which I don't even want; ugh |
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.
What's this comment about?
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 fixed this to use an actual PhantomData
value
I'm not sure what a better way to do it is, but I think the current
Also, in both traversals, the 'state' object is going to be updated, even though the result is going to be thrown away. This seems particularly problematic in modes.rs, where an error bubbling up would cause the 'state' to end up in an invalid state. |
I changed this to perform the modification entirely in |
source/vir/src/modes.rs
Outdated
check_ghost_blocks: _, | ||
block_ghostness, | ||
fun_mode: _, | ||
ret_mode: _, |
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.
ret_mode needs to be saved too
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.
Overall, I think this is a really cool and important feature to add!
A few comments/questions:
-
Are there situations where
iter.cur
is necessary, and we cannot just use the iteration variablex
? If so, this might be good to document with an example when explaining the feature. Overall I do believe that in some casesiter.cur
might be more readable thanx
, so I think we should have it anyways, but yeah, if we have an example, that would motivate it even more. -
for x in 0..10
in Rust is slightly different from C'sfor (int x = 0; x < 10; x++)
in that the variablex
is never actually set to 10 in Rust, instead the iterator just "runs out". Do we think that can be a point of confusion in the invariant for more complex iterators in the future? Specifically, considerfor x in (0..10).map(|a| a * 3).filter(|b| b < 13)
, which doesn't have a trivial "last value", so the final value exposed to the outside might not be obvious. Fwiw, this might possibly be fixed simply through sufficient documentation in the book, and might not need a fix to the design, but is definitely something we need to pay attention to, since folks who understand how Rust iterators work can be confused by this. -
Is the trait design restricting us to terminating iterators only? Supporting non-terminating iterators would be important btw. I believe that the current design would support them, but just bringing up a few examples that we should look into to confirm that (eventually when we add the other iterator combinators) would not require a re-design of the
ForLoopGhostIterator
trait:std::iter::repeat(()).take(10)
is terminating, since therepeat
is non-terminating (so would just have an arbitrary value in itsghost_decrease
), but then thetake
could make it terminating (by having an actually decreasing value inghost_decrease
). Also(0..10).take_while(|a| a * a < 10)
is terminating because even thoughtake_while
may or may not be terminating by itself, the0..10
is terminating. -
For
ghost_peek_next
, is there a reason that it is not returning anOption<Self::Item>
and using.unwrap_or(vstd::pervasive::arbitrary())
or similar in the expansion? That might be a cleaner thing for implementers of the trait to implement. Similar forghost_decrease
. It is possible I'm missing something obvious here btw, so mostly asking for clarification (but I do think modulo a good reason, we might want to put theOption
s there) -
I really love the automatic inference of
0 <= iter.cur <= 10
<3 I also appreciate that this has the machinery to support this for arbitrarily complex user-defined iterators too. -
Can we (or indeed even should we) support things like
let mut iter = (0..10); for x in iter { ... }
, without needing to sayfor x in iter: iter
or similar to be able to access theiter
view and such. I don't think this is a common use case though, so it might be fine to even just say "live with theiter: iter
" btw.
Could we split Typing into two objects, one passed by & and one passed by &mut, to match the style of the other recursions? That would make the 'state' a lot smaller, so we could just clone it. |
I think this will come up when we use iterators generated by slices and vectors. There are some unrelated technical issues to resolve in working with slices before this will work properly with the std library iterators, but in the meantime the
Here, x is the element from the Vec in each iteration, and x isn't nearly as useful in invariants as iter is. So I agree, when these iterators are ready, the Verus guide documentation on for loops should explain why it's necessary to use the iterator in the invariants for iterators from collection types.
The value of x in the invariant comes from the
In principle, the design supports nonterminating iterators, since it allows any invariant and ensures and it makes decreases optional. You could, for example, have a nonterminating iterator ensure false. You could also have iterator combinators supply their own invariants that are, in principle, built out of the iterators that they are combining, and this could, in principle, turn a nonterminating iterator into a terminating iterator. In practice, I don't know how easy it is to compose the invariants and ensures when combining iterators together.
I like this suggestion and I just pushed it in a commit. In the short term, we will just use
I'm inclined to just say "live with the |
My plan is to merge #961 into this just before merging this into main, and squash it all into a single commit. But I'll wait for the remaining reviews first. |
I should also add: this is all just for automatically generating some convenient default invariants and ensures in common cases. If it turns out to be too hard to compose these defaults together for complex iterator combinators, that's ok. We can just have the combinators generate defaults of |
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.
@Chris-Hawblitzel provided some context offline for the translation to loop
.
It is now an exact match of the canonical translation in the rust reference (which addresses my concern of potential inconsistencies of treatment of the translation for verification and compilation). Thanks!
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.
Awesome, lgtm! Looking forward to have for
loops in Verus :)
* Refactor state in modes.rs * Move Typing into submodule
This showed up in one of the files in `rust_verify/example`. See the [for-loop PR](verus-lang/verus#954) for a few details on the syntax.
Examples:
Currently, this has been tested on ranges but other iterators that implement the appropriate traits could also be used:
It is conventional, but not mandated, that ghost iterators will also implement a
view
method that returns aSeq
of all the elements iterated over so far.The syntax macro translates for loops into regular loops with an added ghost iterator and added invariants, so that
for x in y: e invariant inv { body }
is translated into:This translation is only used for verification; when erasing spec or proofs, the translation produces the original for loop with the spec code erased. For verification, regular loops are used in order to get access to the executable iterator so that
exec_invariant
can be applied to it.The
ghost_invariant
method takes an optionalinit
value containing the original expression that was used to create the exec and ghost iterators. This expression is an exec expression, but if it is legal to view the expression as a spec expression (e.g. viawhen_used_as_spec
), and the spec expression only depends on variables that are unmodified inside the loop, then Verus passes the expression asSome(expression)
toinit
. This enables a much strongerghost_invariant
that, for example, can talk about theend
of theRange
without the programmer having to write an explicit invariant likeiter.end == 10
or0 <= iter.cur < 10
.