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

Add support for actions under temporal operators #1879

Merged
merged 11 commits into from
Jun 27, 2022

Conversation

p-offtermatt
Copy link
Collaborator

@p-offtermatt p-offtermatt commented Jun 24, 2022

Fixes buggy behaviour when actions are inside temporal operators by subtly changing the encoding.
It necessitates a change from the way the encoding is specified in the paper, which is necessary due to the technicalities of TLA+.
Essentially, double-priming is not allowed, while in LTL stacking next operators is no issue, so the subtle changes account for this.

  • Tests added for any new code
  • Entries added to ./unreleased/ for any new functionality

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2022

Codecov Report

Merging #1879 (6d5a528) into unstable (2e39803) will increase coverage by 0.02%.
The diff coverage is 94.44%.

@@             Coverage Diff              @@
##           unstable    #1879      +/-   ##
============================================
+ Coverage     76.83%   76.85%   +0.02%     
============================================
  Files           400      400              
  Lines         12271    12279       +8     
  Branches        577      584       +7     
============================================
+ Hits           9428     9437       +9     
+ Misses         2843     2842       -1     
Impacted Files Coverage Δ
...syte/apalache/tla/pp/temporal/TableauEncoder.scala 90.90% <91.66%> (-0.27%) ⬇️
...n/scala/at/forsyte/apalache/tla/pp/Desugarer.scala 82.00% <100.00%> (ø)
...he/io/annotations/parser/CommentPreprocessor.scala 97.80% <0.00%> (+1.09%) ⬆️
...a/at/forsyte/apalache/tla/lir/TlaLevelFinder.scala 100.00% <0.00%> (+2.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e39803...6d5a528. Read the comment docs.

@p-offtermatt p-offtermatt linked an issue Jun 24, 2022 that may be closed by this pull request
@p-offtermatt p-offtermatt requested a review from konnov June 24, 2022 12:03
@p-offtermatt p-offtermatt marked this pull request as ready for review June 24, 2022 12:03
@p-offtermatt p-offtermatt requested a review from thpani June 24, 2022 14:18
Copy link
Collaborator

@konnov konnov left a comment

Choose a reason for hiding this comment

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

Looks good. I am not sure about rewriteAssignmentsAsEquality though.

.unreleased/bug-fixes/1871.md Outdated Show resolved Hide resolved
@@ -33,6 +34,21 @@ class TableauEncoder(

private def inBoolSet(element: TBuilderInstruction): TBuilderInstruction = builder.in(element, builder.booleanSet())

/* replaces all occurences of foo := bar with foo = bar */
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add a comment on why it is sound?

Copy link
Collaborator Author

@p-offtermatt p-offtermatt Jun 24, 2022

Choose a reason for hiding this comment

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

You mean, why this is okay to do?
This is inside a temporal property, not in an actual action. Leaving these as assignments leads the transition finder to see an assignment on the right-hand side of another assignment.
E.g. consider this

CoolAction == x := 2

TemporalProperty == <>[CoolAction]_x

Due to the details of the encoding, a variable will be produced that gets assigned like this:

_temporal__CoolAction := (x := 2)

Then the assignment (x := 2) should be written as an equality, and that's what this function does.

I added the explanation to where the function is invoked, let me know what you think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean, usually you would have x' := 2, that is x with a prime. Would you keep the prime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clarified this a bit more. (TLDR: everything is kept the same, except := is replaced by =)

Copy link
Collaborator

@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.

👍

@p-offtermatt p-offtermatt merged commit f4a1a77 into unstable Jun 27, 2022
This was referenced Jul 4, 2022
@shonfeder shonfeder deleted the ph/temporal-actions branch July 21, 2022 21:28
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.

Temporal operators don't work when actions are inside
4 participants