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

Fix and update ewd840 spec #1178

Merged
merged 7 commits into from
Nov 8, 2023
Merged

Fix and update ewd840 spec #1178

merged 7 commits into from
Nov 8, 2023

Conversation

bugarela
Copy link
Collaborator

Hello :octocat:

This fixes #581 by:

  1. Commenting out property AllNodesTerminateIfNoMessages which has a negation of an action, not allowed by quint's effect system
  2. Commenting out and init section of SpecWFstep since init is an action in quint and therefore cannot be used as a predicate

We should think about whether we want so support this stuff in quint and, if so, how.

In order to make this work with verify, I also had to:

  • Fix one error where oneOf() was called in a val (and not nondet). See Infer effects for non-determinism of oneOf #381
  • Change how the range is computed in Inv (copied from EWD840.tla in the apalache project), as apalache doesn't support non constant values in ranges (tpos).

I also made cosmetic changes in comments and renamed Node -> Nodes.

  • Tests added for any new code
  • [-] Documentation added for any new functionality
  • [-] Entries added to the respective CHANGELOG.md for any new functionality
  • [-] Feature table on README.md updated for any listed functionality

Copy link
Contributor

@thpani thpani left a comment

Choose a reason for hiding this comment

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

LGTM, but I would like to get @konnov's view on commenting out init.

Comment on lines 154 to 166
/// The following property says that eventually all nodes will terminate
/// assuming that from some point onwards no messages are sent. It is not
/// supposed to hold: any node may indefinitely perform local computations.
/// However, this property is verified if the fairness condition WF_vars(step)
/// is used instead of only WF_vars(System) that requires fairness of the
/// actions controlled by termination detection.
///
/// Quint translation note: we cannot add `init` to this property since, in quint,
/// it is an action - not a predicate
temporal SpecWFstep = always(step.orKeep(vars) and step.weakFair(vars)) // and init

// The following property is not expressible in quint (for now), as it negates an action
// temporal AllNodesTerminateIfNoMessages = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "the following property" refers to AllNodesTerminateIfNoMessages, not to SpecWFstep. I got confused here, I think this meaning is lost with the current formatting.

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, yes, I haven't noticed that. Will fix.

///
/// Quint translation note: we cannot add `init` to this property since, in quint,
/// it is an action - not a predicate
temporal SpecWFstep = always(step.orKeep(vars) and step.weakFair(vars)) // and init
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should check with @konnov on how to treat init in temporal formulas, and note this here with a reference to an issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@konnov pinging you here since you probably missed it!

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I think it is the fifth time I am staring at it. Let me open an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe refer to the issue and comment out the whole formula?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Ok, will do

System,
Environment,
}

val vars = (active, color, tpos, tcolor)

// temporal Spec = Init and always(Next.orKeep(vars) and System.weakFair(vars))
// temporal Spec = init and always(step.orKeep(vars) and System.weakFair(vars))
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we already had this commented out because of init?
Probably good to treat it the same as below (i.e., just comment out init, or whatever we decide to do there)

@bugarela bugarela requested a review from thpani November 7, 2023 12:39
Copy link
Contributor

@thpani thpani left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for revising this spec 🎉

@bugarela bugarela enabled auto-merge November 8, 2023 14:08
@bugarela bugarela merged commit 766b7de into main Nov 8, 2023
15 checks passed
@bugarela bugarela deleted the gabriela/ewd840-adaptations branch November 8, 2023 18:04
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.

Typechecking ewd840
3 participants