Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
sort PR #6
base: master
Are you sure you want to change the base?
sort PR #6
Changes from 9 commits
654a829
a11ba47
4bf7dfb
9465f4e
189783b
a7d8388
a86707b
f07353f
54446dc
5b4b59c
fb95db7
735c8a3
0ba6b67
a8bd811
5064b30
f2cefeb
bcd0062
c3ddd55
e7497ed
5b3bb69
26a9d37
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
functions should also be annotated as terminating. Note that termination measures in interface methods' contracts have a special meaning - they define an upper bound on the termination measures of the implementations. If you are curious, I can explain why in the next meeting
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.
you are absolutely right of course. the lack of these annotations might also be the reason why i can't use
isPartiallySorted
in the spec of other methods. an appropriate termination measure forisPartiallySorted
would beend - start
.however, simply adding
decreases
toless_spec
causes the errorsince i need
less_spec
to be terminating forisPartiallySorted
to be terminating i also cannot add thedecreases end - start
clause toisPartiallySorted
as of now.i'd be glad to have a look at this in the meeting as you suggested
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.
for which line is this error message reported?
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.
1: If i put
decreases
for both the interface version ofless_spec
and on the implementation of(x IntSlice) less_spec
i get the above error pointing at the first line of the precondition ofless_spec
in the interface.Additionally, I get
Pure function might not terminate. Termination condition might not decrease
3 times.2: Once pointing at the line right above the new
decreases
clause in(x IntSlice) less_spec
.3,4: And TWICE pointing at the line right above the new
decreases
clause.5: Additionally i get
which is just the last 3 lines of the
x.Mem
predicate.It reports 5 errors, one of them twice but mentions
Gobra has found 4 errors
.Putting
decreases
only on the implementation(x IntSlice) less_spec
reports errors 2 and 5.Putting
decreases
only on the interface version ofless_spec
reports errors 1, 3, 4 and 5 but says it found only 3 errors.GobraIDE is a different story again. It verifies fine if we have
decreases
only on the implementation(x IntSlice) less_spec
which is not very useful alone even if it were true. In any other scenario it reports error 1 and additionallyPostcondition might not hold. Assertion "entire spec and declaration of less_spec" might not hold
at the same line as error 1.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 talked in the meeting how this transitivity requirement should be replaced via the introduction of a method like
lessTransitiveProof
to improve performance.While I understood the simple examples you presented in the meeting. I don't know how to go about this in this context. How would
lessTransitiveProof
have to look like on the implementation level, i.e. what would be its body if it even has a body?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.
There is no general answer to what the body would look like. Nonetheless, the body must always exist, otherwise this is not verified, it is just an assumption. In order to write the body of
lessTransitiveProof
, you will need to know the body ofless_spec
. Most often, if these implementations are trivial, the smt solver will be able to prove the postcondition oflessTransitiveProof
. In some other cases you may need to introduce some additional lemmas or to do a proof by induction, which can be encoded as a recursive (ghost) method in Gobra. We can take a look at examples in our next meetingThere 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.
no triggers have been specified here. I guess that
{ less_spec(a, b, elems), less_spec(b,c,elems) }
would be a good fitThere 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.
you are right. your proposed triggers also work fine, thank you!
i still struggle with some of the others as a result of the inconsistency between GobraIDE and the CLI version
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.
The function
IsSorted
below naturally yields the postcondition [1]:if the elements are sorted.
This definition of "sortedness" makes sense but does not easily lend itself to checking for sortedness of
data
of any length (i.e. it cannot be directly applied whenLen()<=1
). I thought it would be nice to have a ghost pure helper functionisPartiallySorted
that can then also be used to easily check for sortedness of "subslices" ofdata
as is required ininsertionSort
and even more useful in the invariants ofinsertionSort
.This implementation of
isPartiallySorted
follows the typical pattern of a recursive sortedness check. However, it does not manage to establish the same postcondition mentioned above in [1].Conversely, I tried to use my implementation of
isPartiallySorted
in the loop invariant ofIsSorted
but it could not be preserved.To be clear, I don't even need the postcondition [1] in line 81 here. It was just included as a sanity check to see why I could not use
isPartiallySorted
inIsSorted
.Nonetheless, I think it should still hold and cannot spot the issue.
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.
Why not? here, if len(elems) <= 1, then the property trivially holds - every sequence with 0 or 1 elements is sorted
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.
as commented above, this assertion does not hold
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 postcondition (and presumably the next one too) cannot get verified by the GobraCLI.
I run the most current versions of the GobraIDE and GobraCLI.
This postcondition only verifies for the GobraIDE stable build version but fails for the GobraIDE nightly build version and the GobraCLI.
Postcondition might not hold: Assertion elems[i] === swapped_elems[j] might not hold.
I can verify that
old(unfolding x.Mem(elems) in x[i]) === (unfolding x.Mem(swapped_elems) in x[j])
. Further, it should know from the predicatex.Mem(elems)
from the precondition that in theold
context we had a 1:1 mapping between the elements ofx
andelems
. The same is given in the new context byx.Mem(swapped_elems)
for a mapping betweenx
andswapped_elems
.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 was not able to make
haveSameElements
work on the implementation level.It cannot automatically be established after the elements in
x
have been swapped. I tried to implement a mapping ofx
to a multiset in an effort to try and prove this on the implementation level, however, this approach failed due to the error described in https://github.com/jcp19/verified_go_stdlib/pull/6/files#r1067661530.Nonetheless, this assumption should still be safe.
Swap
is the only method described in the interface that actively changes the order of the elements and it is safe to assume here that reallyhaveSameElements(elems, swapped_elems)
holds.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 postcondition causes Gobra to crash with the message:
This does not happen if
s2
is also of typemset[int]
.It is very possible that this is not even a legal operation I'm trying to do here but I still think Gobra should not crash outright.
Moreover, looking at the postcondition in
toSeq
it is possible to compare elements in the collections of type[]int
andseq[any]
. It is just the contains operatorin
that leads to the crash.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.
yes, ideally Gobra should insert a type conversion automatically here or should return a type error. The reason here is that ints and values of interface types are encoded differently (with different types at the viper level), but we are treating them at the viper level as if they were encoded as the same type.
As for now, as soon as PR viperproject/gobra#600 is merged, we can side-step this issue by replacing
e in s2
with(interface{})(e) in s2
. Can you please open an issue on the issue tracker about this?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.
opened issue https://github.com/viperproject/gobra/issues/601
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.
viperproject/gobra#600 was merged. The approach with a cast should work now