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

ENH: parse and expand bound tuples #140

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

johnyf
Copy link
Contributor

@johnyf johnyf commented Jun 26, 2024

Parsing and translation support for bound tuples, examples:

  • rigid quantification \E <<x, y>> \in A \X B: ...
  • choose expressions CHOOSE <<x, y>> \in A \X B: ...
  • set constructor for subset {<<x, y>> \in A \X B: ...}
  • set constructor {...: <<x, y>> \in A \X B}
  • function definitions f[<<x, y>> \in A \X B] = ...
  • function constructors [<<x, y>> \in A \X B |-> ...]

The syntax tree is changed to represent bound tuples. The translation is after the parser and before conversion to indexed identifiers. Subexpression references where bound tuples occur are not supported.

If a Cartesian product is found in the expression, of the same length as the bound tuple, for example <<x, y>> \in A \X B, then a rewriting to a simpler expression is applied. For example,

{<<x, y>> \in A \X B:
    P(x, y)}

is translated to (the names of auto-generated identifiers differ):

{y \in A \X B:
    LET
        a == y[1]
        b == y[2]
    IN
        P(a, b)}

Another case is:

{x + y: <<x, y>> \in A \X B}

which is translated to:

{x + y: x \in A, y \in B}

Otherwise the rewriting results in expressions that contain CHOOSE or quantification.

Includes fixes to parsing of declarations in function definitions and quantification.

lemmy added a commit to lemmy/pbft-tlaplus that referenced this pull request Sep 3, 2024
lemmy added a commit to heidihoward/pbft-tlaplus that referenced this pull request Sep 6, 2024
@ahelwer
Copy link
Collaborator

ahelwer commented Nov 4, 2024

@johnyf could you rebase on the latest changes in main to see whether these changes pass the new syntax tests from #159? I tried to rebase myself but could not successfully resolve the merge conflicts in src/expr/e_anon.ml.

@johnyf
Copy link
Contributor Author

johnyf commented Nov 4, 2024

I will try to rebase the changes.

@ahelwer
Copy link
Collaborator

ahelwer commented Nov 4, 2024

Great! You can run the parser tests with dune runtest test/parser --force --always-show-command-line; they should take less than a second. It should cause these tests to fail:

(* https://github.com/tlaplus/tlapm/issues/11 *)
"Bounded Quantification With Tuples";
"Mixed Bounded Quantification With Tuples";
"Bounded CHOOSE With Tuple";
"Unbounded CHOOSE With Tuple";
"Set Filter with Tuple";

If you delete those test names from the list then the test should pass if these changes fix all those cases.

and update the code throughout `tlapm`,
because this commit changes the existing types in the
module `Expr.T`.

Signed-off-by: Ioannis Filippidis <[email protected]>
The change in this commit corrects a bug with function
definitions. Parsing of function definitions allowed using unbounded
declarations (and also mixing bounded and unbounded declarations).
For example, before this change, parsing allowed:

```tla
f[x \in S, y] == TRUE
```

and even:

```tla
f[x] == TRUE
```

These syntax errors are detected by SANY.
In any case, this change ensures that `tlapm` does not parse
such definitions.

The error was due to calling the function `bounds` within the
function `ophead`, instead of calling the function `boundeds`
(which was called before commit
5958dfa
in order to handle function constructors within the function
`atomic_expr`).

This commit also adds tests for this bug.

Signed-off-by: Ioannis Filippidis <[email protected]>
because TLA+ does not allow this kind of syntax.
A single rigid quantifier can include either:
- only unbounded declarations,
  for example `\E x, y, z:  ...`, or
- only bounded declarations,
  for example `\A x \in A, y \in B, z \in C:  ...`.

A single rigid quantifier cannot include both bounded and
unbounded declarations.

This syntax is caught by SANY.

Signed-off-by: Ioannis Filippidis <[email protected]>
in other words, that the following expression is not allowed:

```tla
[x \in S, y |-> TRUE]
```

and the following expression is allowed:

```tla
[x \in S, y \in S |-> TRUE]
```

Also, add a reminder in a comment in the parser code.

Signed-off-by: Ioannis Filippidis <[email protected]>
instead of appending during the fold.
This approach is more efficient.

Also:

- STY: blankspace formatting

Signed-off-by: Ioannis Filippidis <[email protected]>
Signed-off-by: Ioannis Filippidis <[email protected]>
## ENH: tuply declarations

This commit adds parsing and translation support for
tuply declarations that appear in:
- rigid quantification `\E ... here ...: ` and `\A ... here ...: `
- choose expressions `CHOOSE ... here ...: `
- set constructors
  `{... here ... \in ...:  ...}`
- set constructors `{...:  ... here ...}`
- function definitions `f[...here...] = ...`
- function constructors `[... here ... |-> ...]`

Commit 5958dfa introduced
parsing of tuple declarations in function constructors,
for example for expressions of the form:

```tla
[<<x, y>> \in A \X B |-> x + y]
```

This commit rewrites that functionality, and uses the
new functionality to parse tuple declarations that can
appear in function definitions, i.e., definitions of the form:

```tla
f[<<x, y>> \in A \X B] == x + y
```

The parsed tuply declarations are then transformed to either:

- optimized expressions that are simplified and do not
  contain additional `CHOOSE` operators, in the presence of
  a syntactically-recognizable Cartesian product of
  suitable length, or

- the expressions defined by TLA+ semantics (in most cases
  these expressions contain `CHOOSE`), otherwise.


This commit also adds tests for the above functionality.


## BUG: correctly represent tuple declarations in function constructors

In general, tuple declarations in function constructors
are defined using `CHOOSE` with tuple declarations,
which in turn is defined using unbounded `CHOOSE` and
quantification.

This was not how function constructors were implemented
in commit 5958dfa.
This commit corrects this error. The previous encoding
is closer to what is currently done when a Cartesian
product of the appropriate length is recognized in syntax.


## Tuplification

This commit adds a translation of function constructor
signatures that tuplifies comma-separated declarations,
and fuses the domain-bounds of such declarations into
a Cartesian product.

The motivation for doing so is that it enables passing
such functions (of tuples) to Isabelle and Zenon with
the existing support that is available in those backends
(for tuples and Cartesian products), and the proofs succeed.


## Other changes

- DOC: update file `todo.txt`

- ENH: add function `E_visit.name_operators` for
  converting syntax trees with positional operator
  references to syntax trees with operator
  references by name (i.e., conversion of `Ix` nodes
  to `Opaque` nodes).

Signed-off-by: Ioannis Filippidis <[email protected]>
@johnyf
Copy link
Contributor Author

johnyf commented Nov 6, 2024

Thank you for noting how to configure the parser tests. Rebased (omitted blankspace and formatting changes) and updated to recent changes. The parsing tests pass, including also one more, about {x \in Nat}.

@ahelwer
Copy link
Collaborator

ahelwer commented Nov 6, 2024

Okay great! Are you able to join the next TLA+ community call on November 12th and talk about these changes? They seem really great so I would like to get everybody talking about the best way to get them in.

@johnyf
Copy link
Contributor Author

johnyf commented Nov 7, 2024

Yes, I can join the next TLA+ community call and talk about the changes. Thank you for suggesting it!

Copy link
Contributor

@damiendoligez damiendoligez left a comment

Choose a reason for hiding this comment

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

This looks good but I have a few remarks and suggestions you might want to take into account.

src/expr/e_namespaces.ml Outdated Show resolved Hide resolved

(* Scheme 1: Planar coordinates

Fresh identifier based on location.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this method doesn't guarantee a fresh identifier, as the user could have an identifier in scope named somefile_line_123_column_45. We can probably ignore this problem as too unlinkely to happen in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. The module E_namespaces was added to be used in the module E_tuply_declarations, but E_namespaces is currently not used, so it could be removed, or the function mint_from_hint removed.

(In coalescing a similar mapping was experimentally applied to all identifiers of a proof obligation (when with_unique_identifiers = true in Coalesce), but if applied partially identifiers can coincide with existing ones.)

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 it's better to avoid unused code and remove E_namespaces entirely, we can add it back if we need it in the future.

src/expr/e_namespaces.ml Outdated Show resolved Hide resolved
src/expr/e_namespaces.ml Outdated Show resolved Hide resolved
src/expr/e_parser.ml Outdated Show resolved Hide resolved
src/expr/e_visit.ml Outdated Show resolved Hide resolved
src/expr/e_visit.ml Outdated Show resolved Hide resolved
src/expr/e_visit.ml Outdated Show resolved Hide resolved
src/global/intermediate.ml Outdated Show resolved Hide resolved
src/module/m_visit.ml Show resolved Hide resolved
@johnyf
Copy link
Contributor Author

johnyf commented Nov 12, 2024

Thank you for reviewing! I will work on revising the changes.

Copy link
Contributor Author

@johnyf johnyf left a comment

Choose a reason for hiding this comment

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

Thank you for the remarks and suggestions, I revised the changes by adding 2 new commits.


(* Scheme 1: Planar coordinates

Fresh identifier based on location.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. The module E_namespaces was added to be used in the module E_tuply_declarations, but E_namespaces is currently not used, so it could be removed, or the function mint_from_hint removed.

(In coalescing a similar mapping was experimentally applied to all identifiers of a proof obligation (when with_unique_identifiers = true in Coalesce), but if applied partially identifiers can coincide with existing ones.)

src/expr/e_namespaces.ml Outdated Show resolved Hide resolved
src/expr/e_namespaces.ml Outdated Show resolved Hide resolved
src/expr/e_namespaces.ml Outdated Show resolved Hide resolved
src/expr/e_parser.ml Outdated Show resolved Hide resolved
src/expr/e_visit.ml Outdated Show resolved Hide resolved
src/expr/e_visit.ml Outdated Show resolved Hide resolved
src/global/intermediate.ml Outdated Show resolved Hide resolved
src/module/m_visit.ml Show resolved Hide resolved
src/util/fmtutil.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@damiendoligez damiendoligez 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. We can merge this after you remove E_namespaces or if you prefer to keep it.

@johnyf
Copy link
Contributor Author

johnyf commented Nov 27, 2024

Thank you for approving the changes. I removed E_namespaces in a new commit.

Signed-off-by: Ioannis Filippidis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants