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

Test cases for @assert notation #153

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

Conversation

langfield
Copy link
Contributor

@langfield langfield commented Feb 2, 2023

Add test cases for @assert notation

At a high-level, we add a bunch of tests and golds, break module name formatting into two parts, and sort the scopedNames when constructing module names.

This commit replaces the normalizedName function with several new functions, removing use of partial helper functions like init and head, and instead making use of Data.List.NonEmpty. The new functions are:

  • preprocessScopes
  • formatScopes
  • summarizeLabels

Additionally, we rename:

  • summarizeLabels -> summarizeLabels'
  • sansCommonAncestor -> detachCommonPrefix

This fixes a bug where a dot is not added between function name and label for anonymous asserts.

We no longer generate the prefix and labelsSummary within the same function, because that was kind of arbitrary.

Other things:

  • Adjust gold files to remove assert label, inlined markers
  • Add test assert_let_minimal.cairo
  • Fix order of results in assert_let.gold

Notes from squashed intermediate commits

  • Add precondition checking tags to golds
  • Fix branch identifiers in golds
  • Add mathsat back to oneoff.sh test running script.
  • Fix assert_let.gold. It now correctly shows something other than
    Verified for the functions with faulty asserts. In particular, it
    shows Contradictory premises.

Comments from old PR reviews

All this .gold should actually just be

prod
Verified

But without mathsat, CI will run into this timeout. I'm doubtful about what to do with it as I would like this test case to not be "forgotten".

Let's wait to merge this until after we get mathsat working in the CI again?

Note that tests/resources/golden/assert_func_peano_prod.gold currently times out. Not anymore!

@langfield langfield force-pushed the aemartinez/assert-tests branch from bbb3405 to 976df12 Compare February 2, 2023 21:08
@langfield langfield marked this pull request as ready for review February 2, 2023 21:08
@langfield langfield force-pushed the aemartinez/assert-tests branch 2 times, most recently from 44a653d to f8bec79 Compare February 14, 2023 18:27
In this commit, we add a function `userAnnotatedSources` which replaces
`isStandardSource`. It generates a list of all the user annotated
ScopedFunctions. This list is used to filter modules for solving. This
requires a slight refactor within `FunctionAnalysis.hs`.

* Add `extern_remove_dirty` test.

The basic idea is this:
1. Find all wrapper functions.
2. Compute their respective set of reachable functions.
3. Mark them all as 'don't check' unless they are referenced from a
   different source as well.
* Use `ssh-agent` to clone with specific private key
* Set `0o400` permissions on private key file
* Add `mathsat` to list of solvers used in tests
Add mathsat installation to Github actions workflow
…ted-specs

Add FAQ about commenting-out annotations
…eadme-additions

Add `README.md` section on details of `CairoSemanticsL`
@langfield langfield force-pushed the aemartinez/assert-tests branch 3 times, most recently from 10c8259 to 638c31a Compare March 29, 2023 12:43
At a high-level, we add a bunch of tests and golds, break module name
formatting into two parts, and sort the `scopedNames` when constructing
module names.

This commit replaces the `normalizedName` function with several new
functions, removing use of partial helper functions like `init` and
`head`, and instead making use of `Data.List.NonEmpty`. The new
functions are:

* preprocessScopes
* formatScopes
* summarizeLabels

Additionally, we rename:
* summarizeLabels -> summarizeLabels'
* sansCommonAncestor -> detachCommonPrefix

This fixes a bug where a dot is not added between function name and
label for anonymous asserts.

We no longer generate the `prefix` and `labelsSummary` within the same
function, because that was kind of arbitrary.

Other things:

* Adjust gold files to remove assert label, inlined markers
* Add test `assert_let_minimal.cairo`
* Fix order of results in `assert_let.gold`

Notes from squashed intermediate commits
========================================
* Add precondition checking tags to golds
* Fix branch identifiers in golds
* Add `mathsat` back to `oneoff.sh` test running script.
* Fix `assert_let.gold`. It now correctly shows something other than
  `Verified` for the functions with faulty asserts. In particular, it
  shows `Contradictory premises`.
@langfield langfield force-pushed the aemartinez/assert-tests branch from 10c8259 to b96c5e8 Compare March 29, 2023 14:13
@langfield langfield force-pushed the master branch 2 times, most recently from 9f9928b to 3397b7c Compare March 31, 2023 15:01
userAnnotatedSources :: Set ScopedFunction -> [LabeledInst] -> GlobalL (Set ScopedFunction)
userAnnotatedSources inlinableFs rows =
getProgram >>= \prog ->
let functionsWithBodies = functionsOf rows prog
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bug here that triggers fromJust under some circumstances unknown to me. This needs investigation. Preferably, this whole bit of code wouldn't be here in the first place because we're replicating this kind of logic in FunctionAnalysis.hs and frankly even in Module.hs.

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.

3 participants