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

Extend the invariant filter to specify particular invariants #2034

Merged
merged 11 commits into from
Aug 5, 2022
Merged

Conversation

thpani
Copy link
Collaborator

@thpani thpani commented Aug 4, 2022

Closes #2031. Towards https://github.com/informalsystems/apalache-cloud/issues/106.

We currently expose an invariant filter via tuning flags. It accepts a regular expression recognizing words over step numbers. Apalache then only checks state and action invariants in the step numbers accepted by the filter.

We extend the invariant filter syntax to also specify which invariants to check in a step.
We achieve this by extending the regular expression to accepting words (really, triples) of the shape ${stepNo}->${invariantKind}${invariantNo}. This is similar to the existing transition filter syntax that accepts words ${stepNo}->${transitionNo}.

  • Tests added for any new code
  • Ran make fmt-fix (or had formatting run automatically on all files edited)
  • Documentation added for any new functionality
  • Entries added to ./unreleased/ for any new functionality

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #2034 (444583b) into main (86c13cd) will increase coverage by 0.00%.
The diff coverage is 93.33%.

@@           Coverage Diff           @@
##             main    #2034   +/-   ##
=======================================
  Coverage   77.74%   77.74%           
=======================================
  Files         419      420    +1     
  Lines       12844    12846    +2     
  Branches      590      588    -2     
=======================================
+ Hits         9985     9987    +2     
  Misses       2859     2859           
Impacted Files Coverage Δ
.../at/forsyte/apalache/tla/tooling/opt/TestCmd.scala 0.00% <0.00%> (ø)
...alache/tla/bmcmt/trex/TransitionExecutorImpl.scala 89.51% <ø> (ø)
.../at/forsyte/apalache/tla/bmcmt/InvariantKind.scala 100.00% <100.00%> (ø)
...t/forsyte/apalache/tla/bmcmt/SeqModelChecker.scala 86.28% <100.00%> (ø)
...tla/bmcmt/trex/ConstrainedTransitionExecutor.scala 97.05% <100.00%> (ø)
...he/tla/bmcmt/trex/FilteredTransitionExecutor.scala 86.36% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@thpani thpani marked this pull request as draft August 4, 2022 12:40
@thpani thpani marked this pull request as ready for review August 4, 2022 13:27
Copy link
Contributor

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

I've a few questions for my own edification, and some suggestions that might help improve the docs, but overall looks great to me :)

docs/src/apalache/tuning.md Outdated Show resolved Hide resolved
docs/src/apalache/tuning.md Outdated Show resolved Hide resolved
Comment on lines +80 to +82
* all invariants only *after* exactly 10 steps have been made,
* the *first* state invariant only after exactly 15 steps, and
* the *second* action invariant after exactly 20 steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this example: the first case seems to entail all variants are already checked from step 10 on, so I don't understand how the other two steps can be relevant. Is it possible this configures the checks to run only on the 10th step? Or are the second and third conditions here redundant? Or perhaps I'm misunderstanding the functionality?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I can see how this could be confusing!
The first clause does not apply from step 10 onwards, but only at step 10 (hence, "exactly 10 steps").
Do you think it would help to reword this? Or maybe it's enough to change the emphasis from "after" to "exactly"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that does clear things up for me! I would suggestion perhaps something like:

Suggested change
* all invariants only *after* exactly 10 steps have been made,
* the *first* state invariant only after exactly 15 steps, and
* the *second* action invariant after exactly 20 steps.
* *all* invariants (because of the wildcard match) at the 10th step,
* the *first* state invariant (`state0`) at the 15th step, and
* the *second* action invariant (`action1`) at the 20th step.

Copy link
Collaborator Author

@thpani thpani Aug 5, 2022

Choose a reason for hiding this comment

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

I think we're getting close :)
Still one issue though: saying "at the Xth step" here is inaccurate, as it might refer to both the states before and after taking the step. Technically, we're checking invariants at all (step+1)th (symbolic) states. How about this?

Suggested change
* all invariants only *after* exactly 10 steps have been made,
* the *first* state invariant only after exactly 15 steps, and
* the *second* action invariant after exactly 20 steps.
* *all* invariants (because of the wildcard match) at states reached after the 10th step,
* the *first* state invariant (`state0`) at states reached after the 15th step, and
* the *second* action invariant (`action1`) at states reached after the 20th step.

Copy link
Contributor

Choose a reason for hiding this comment

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

My issue here is that in this context I take "after" to mean "anytime from this point forward". I guess its because while you mean the plural "states" to range breadth wise over the immediate successor steps, I think the natural reading is to read it as both breadth- and depth-wise, ranging over all states that could follow.

Maybe using a phrase like "immediate successor states" would help?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess that's why Igor's original wording was "after exactly 10 steps", which to me communicates that it refers only to 10, not 9 or 11.
But I think you resolve the order of "after" and "exactly" differently than I do?

Copy link
Contributor

Choose a reason for hiding this comment

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

True “after exactly 5 minutes” means to me “any time > t+5m”. Could be i am unusually fixated on an indeterminate range for “after” :)

I don’t want to get you hung up here, especially not if it’s consistent with the other docs. Maybe just merge it as is and we can revisit if other users have trouble?

docs/src/apalache/tuning.md Show resolved Hide resolved
docs/src/apalache/tuning.md Show resolved Hide resolved
thpani and others added 3 commits August 5, 2022 10:34
Comment on lines +80 to +82
* all invariants only *after* exactly 10 steps have been made,
* the *first* state invariant only after exactly 15 steps, and
* the *second* action invariant after exactly 20 steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that does clear things up for me! I would suggestion perhaps something like:

Suggested change
* all invariants only *after* exactly 10 steps have been made,
* the *first* state invariant only after exactly 15 steps, and
* the *second* action invariant after exactly 20 steps.
* *all* invariants (because of the wildcard match) at the 10th step,
* the *first* state invariant (`state0`) at the 15th step, and
* the *second* action invariant (`action1`) at the 20th step.

docs/src/apalache/tuning.md Show resolved Hide resolved
@thpani thpani enabled auto-merge August 5, 2022 21:30
@thpani thpani merged commit 14a2f56 into main Aug 5, 2022
This was referenced Aug 8, 2022
@thpani thpani deleted the th/invfilter branch August 8, 2022 07:39
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.

Extend invariant filter to select invariants to check
3 participants