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

Merge updated_enabled_cdot. #148

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

Merge updated_enabled_cdot. #148

wants to merge 44 commits into from

Conversation

kape1395
Copy link
Collaborator

Here is an attempt to merge old branch updated_enabled_cdot.

johnyf and others added 30 commits March 31, 2021 14:24
Otherwise the already computed weights of an operator can
rely on the arity of the operator before the dependence on
parameters of the definition (`Foo(x) == INSTANCE Inner`)
is added to the instantiated operator signature.

Parametric instantiation changes the arity of operators,
so the weights list needs to be recomputed, based on the
arity after instantiation has been performed.
This change ensures that the testing commands
are read after the closing horizontal rule (`=====*`)
of the module.

To do this, the nesting depth of submodules is tracked,
by incrementing the counter `submodule_nesting_depth` when
encountering a module opening (`-----*\s*MODULE`), and
decrementing this counter when encountering
a module closing (`=====*`).

Commands are read after the module's end.
Previously, commands were read after the first module closing
(`=====*`), which results in errors in the presence
of submodules.
that prints the names of modules in
a module context.
When instantiating inside a LET, definitions are kept,
and other module units omitted. A negative shift is
applied to the remaining module units. This negative shift
should equal minus the number of hypotheses that the
omitted module unit introduces in the context.

The number of hypotheses for each module unit is
computed by the function `M_t.hyps_of_modunit`.
`Module.Elab` replaces each `INSTANCE` statement
with definitions. If the instantiated module
extends the module `TLAPS`, then the definitions
include backend pragmas (constructor `Bpragma`).
`Module.Elab` replaces each `INSTANCE` statement
with definitions. If the instantiated module
extends the module `TLAPS`, then the definitions
include backend pragmas (constructor `Bpragma`).
This change makes fingerprints take into account
the result of the proof directive AutoUSE.

Previously, fingerprinting was done before any
expansion of definitions, normalization, and
automated expansion of definitions, expansion of
ENABLED and of \cdot.

This approach worked correctly for BY DEF definitions,
because those definitions were included in the
fingerprint as context.

With this change, only for proof obligations that
include the proof directive AutoUSE, the fingerprint
is computed after expansion of definitions, normalization,
automated expansion of definitions, expansion of ENABLED,
and of \cdot. This change ensures that the proof
obligation is fingerprinted with all automatic
expansions of definitions applied.

A test is added that catches this error.
summary of changes

- test fingerprints by running `tlapm` twice
- add new proof directive `ENABLEDrules`
- rewrite `ENABLEDaxioms` to remove the
  Boolean typeness assumptions
- rewrite two proof rules that previously were in
  `ENABLEDaxioms` and now are in `ENABLEDrules`,
  to remove the Boolean typeness assumptions
- correct soundness check for `ENABLEDrules`
  (previously for `ENABLEDaxioms`)
- fingerprint the level correctness of proof obligations
- revise renaming of variables in `ExpandENABLED`

- removal of older implementation of `ENABLEDaxioms`
- `ENABLEDaxioms` was not correctly collecting primed variables
- use expression level in fingerprint of definition
- record name of `Bpragma` in fingerprint
- implement rewriting system for `ENABLEDrewrites`
- add new proof directive `ENABLEDrewrites`
- add test modules
These changes are for now kept as a separate
commit, in case further rebasing is needed
before preparing the pull request for the
branch `update_enabled_cdot`.
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
@kape1395
Copy link
Collaborator Author

kape1395 commented Sep 1, 2024

The LSP server currently fails with

LSP request failed with exception Not_found, backtrace:
Raised at Stdlib__List.find in file "list.ml", line 232, characters 10-25
Called from Tlapm_lib__Property.get in file "src/util/property.ml", line 71, characters 9-54
Called from Tlapm_lib__E_levels.get_level_info in file "src/expr/e_levels.ml" (inlined), line 68, characters 23-47
Called from Tlapm_lib__E_levels.get_level in file "src/expr/e_levels.ml", line 70, characters 21-37
Called from Tlapm_lib__Fingerprints.fp_sequent.spin in file "src/backend/fingerprints.ml", line 446, characters 23-48
Called from Tlapm_lib__Fingerprints.fp_sequent.spin in file "src/backend/fingerprints.ml", line 441, characters 13-26
...

It seems like the levels are not assigned somewhere.

Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
@kape1395 kape1395 self-assigned this Sep 1, 2024
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
@kape1395 kape1395 marked this pull request as ready for review September 1, 2024 21:11
@kape1395
Copy link
Collaborator Author

kape1395 commented Sep 1, 2024

@muenchnerkindl, @damiendoligez, who could review this PR?

kape1395 and others added 2 commits September 11, 2024 21:21
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Damien Doligez <damien.doligez@inria.fr>
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 modulo a few suggestions.

examples/EWD840.tla Outdated Show resolved Hide resolved
lsp/lib/server/handlers.ml Show resolved Hide resolved
src/backend/fingerprints.ml Outdated Show resolved Hide resolved
src/backend/fpfile.ml Show resolved Hide resolved
src/expr/e_action.ml Outdated Show resolved Hide resolved
damiendoligez and others added 6 commits December 10, 2024 15:29
Signed-off-by: Damien Doligez <damien.doligez@inria.fr>
change magic number for fingerprint files
Co-authored-by: Damien Doligez <damien.doligez@inria.fr>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
Signed-off-by: Karolis Petrauskas <k.petrauskas@gmail.com>
@kape1395
Copy link
Collaborator Author

I accepted all the suggested changes and merged the changes from the main branch.

@kape1395
Copy link
Collaborator Author

With these changes, the parsing became slower.
For a document I work on, it takes 0.5 seconds to parse it on the main and 1.3 seconds on this branch.

However, from the LSP server perspective, the lag is even longer (10 or more seconds). I will investigate what makes it so slow.

@kape1395
Copy link
Collaborator Author

For the record on the performance.

This line

let obs = List.filter_map mule_obl (Array.to_list obs) in
now takes 30 seconds on my document (855 LOC).

This PR changed a call (in the body of mule_obl)

Tlapm_lib.Backend.Fingerprints.write_fingerprint o

with

Tlapm_lib.Backend.Prep.prepare_obligation o

to have proof methods assigned properly.

@kape1395
Copy link
Collaborator Author

Here is the flame graph from a profiler.

image

@kape1395
Copy link
Collaborator Author

add_expr_level also takes almost half of the TLAPM execution time in the normal (non-lsp) execution.

image

@damiendoligez, do you know if that's expected? Should I look to make the add_expr_level much faster or try to avoid it in the critical path in the LSP execution? Currently, it makes the LSP unusable on a bit larger proofs.

@kape1395
Copy link
Collaborator Author

I opened #186 to this branch to avoid having the level checking in the critical path of the LSP.

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.

5 participants