Skip to content
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

Undo revert 966 and add fixes #968

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

keyboardDrummer
Copy link
Collaborator

@keyboardDrummer keyboardDrummer commented Oct 18, 2024

Note: review per commit and skip the first one, since that's an unrevert, so the code was previously reviewed approved and merged.

Changes

  • Undo revert Revert isolation #966, which was reverted because it caused unintended Dafny changes
  • Change the isolation of returns, so that each ensures clause gets its own VC when {:vcs_split_on_every_assert} is used, so we get #returns * #ensures additional VCs from return statements.
  • Change the isolation of returns, so that only explicit return statement are given their own VC branch

Testing

  • Added a CLI test that tests the above two changes
  • I have checked that updating Dafny to this latest Boogie version only causes expected failures in Dafny tests that can easily be repaired.


namespace VCGeneration;

public class ImplicitJump : TokenWrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that this token could nest another nested token? If that is the case, I would prefer to see a parameter bool ImplicitJump
inside TokenWrapper which should then be a record, to avoid what we experienced in Dafny with unexpected piles of token wrappers that are hard to unfold properly, especially when the order should not matter.
You can then have an extension method method WithImplicit(this IToken token) that, if the token is already wrapped, copy it while setting the implicit flag, and otherwise wrap it with a wrapped token accordingly

Copy link
Collaborator Author

@keyboardDrummer keyboardDrummer Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great callout. I've changed the check from originalReturn.tok is ImplicitJump to originalReturn.tok is not Token so that any wrapping, which I think indicates it's generated code, will prevent a separate VC from being generated.

I find it hard to say what implementation is the most maintainable here, without having more cases of token wrapping to reason with, so I hope we can leave it at this and stay vigilant.

Copy link
Contributor

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just reviewed the last two commits, trusted the previous review of the un-revert

@keyboardDrummer keyboardDrummer merged commit 7ea138f into boogie-org:master Oct 22, 2024
5 checks passed
@keyboardDrummer keyboardDrummer deleted the rerevert branch October 22, 2024 14:20
keyboardDrummer added a commit that referenced this pull request Oct 22, 2024
Stacks on #968

### Changes
Contains refactoring that were part of
#970

### Testing
No additional tests needed
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