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

Changes to avoid unnecessary assumptions #246

Merged
merged 27 commits into from
Feb 19, 2024
Merged

Conversation

jcp19
Copy link
Contributor

@jcp19 jcp19 commented Feb 10, 2024

After a conversation with @mlimbeck, we figured that the specs that we use in the router are sometimes a bit awkward and force us to introduce a few adhoc assumptions (e.g., assume d.WellConfigured() in Run) which can be made into preconditions if we change slightly our predicates. The goal of this PR is to restructure the predicates used in the dataplane to avoid such assumptions, and to eliminate the need for calling // @ IgnoreFromHere() before the call to d.mtx.Unlock in Run. In the process, we changed slightly the specifications of some public methods in a few ways:

  • they no longer access private fields in the specification, except for d.mtx. We can easily address this later, and it is not urgent at all
  • they are a bit more permissive in terms of preconditions (and may allow to return nil)
  • due to an incompleteness in the closure encoding, we were not able to call the read closure from inside other closures. To overcome that, we manually asserted the preconditions (there are no postconditions to establish). This is cumbersome, ugly, and prone to errors when specs change. In this PR, I introduce a much better temporary fix that requires passing d as a parameter to the closure, which allows us to perform the call.
    To avoid unnecessary work at the moment, we did not update the postconditions to preserve information hiding in the setters as well. Instead, we dropped them for now and we plan to add them lazily, on a per-need basis.

This PR can be seen as a step towards addressing #155 (although completely solving that would require strengthening Gobra's type system).

While doing this PR, I also noticed a few unused ghost methods and some small issues that I fixed.

I also run into a very annoying incompleteness in the termination checker of Viper that I will likely report reported (viperproject/silver#773 (comment)) as a bug.

Finally, verifying Run now seems to be much faster due to better info hiding. However, verifying initMetrics takes much longer. I could spend some time looking into this, but given our current priorities and the fact that the spec of this method should not change again, it is better to spend my time somewhere else.

The only thing missing to conclude this PR is to refactor Run. Once that is done, this PR is ready for review and for merging

@jcp19 jcp19 mentioned this pull request Feb 12, 2024
@jcp19 jcp19 marked this pull request as ready for review February 19, 2024 07:54
@jcp19 jcp19 requested a review from Dspil February 19, 2024 07:55
@jcp19 jcp19 merged commit e71ba3a into master Feb 19, 2024
4 checks passed
@jcp19 jcp19 deleted the joao-drop-run-assumption branch February 19, 2024 12:37
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.

2 participants