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

Support quint sum types #2783

Merged
merged 14 commits into from
Dec 1, 2023
Merged

Support quint sum types #2783

merged 14 commits into from
Dec 1, 2023

Conversation

shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Nov 26, 2023

Closes informalsystems/quint#1034
(thanks to informalsystems/quint#1276).

This adds support for conversion of the following quint constructs:

  • sum types
  • the builtin variant operator
  • the builtin matchVariant operator

A notable change made to support this work was relaxing the rules of the builder library to allow constructing empty records and record types. This is a meaningful value and a well formed type, and is the current choice for the unit type in quint, used for variants that don't take a value with any information (e.g., the A in type T = A | B(int)).

This takes us ~75% of the way to full support for quint sum types, and enables
running verify on
https://github.com/informalsystems/quint/blob/main/examples/language-features/option.qnt

What remains to do:

I expect reviewing by commit is helpful, as they are logically isolated.


Thanks due to @bugarela, who found this problem and was a huge help in debugging.


  • Tests added for any new code
  • Ran make fmt-fix (or had formatting run automatically on all files edited)
  • Documentation added for any new functionality
  • Entries added to ./unreleased/ for any new functionality

@shonfeder shonfeder requested a review from Kukovec as a code owner November 26, 2023 23:22
@shonfeder shonfeder requested review from thpani and bugarela November 26, 2023 23:23
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (2d9a8e2) 78.83% compared to head (2f52f8e) 78.83%.

Files Patch % Lines
...ain/scala/at/forsyte/apalache/io/quint/Quint.scala 76.00% 6 Missing ⚠️
...forsyte/apalache/io/quint/QuintTypeConverter.scala 91.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2783   +/-   ##
=======================================
  Coverage   78.83%   78.83%           
=======================================
  Files         466      466           
  Lines       15905    15921   +16     
  Branches     2572     2592   +20     
=======================================
+ Hits        12539    12552   +13     
- Misses       3366     3369    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Great! I have left a few comments. Since this PR touches upon the work by @thpani and @Kukovec, I would leave it to them.

Shon Feder added 13 commits November 29, 2023 19:25
This gives us tests for supporting sum type conversions into apalache.
Also removes deprecated union types
There is no reason to enforce this bit of surface syntactic trivia
on the AST level, and it imposes needless overhead in the quint
conversion.

We enforce this in parser for the Apalache type annotations, and that
should suffice.
This gives us much more context when error conversion goes wrong due to
an operator we expect to be defined missing an entry in the lookup table.
Used as the unit type in quint, and causes no problems
when used in this way.
This reverts commit fc88f06.

We need to wait on updating this fixture until we have
informalsystems/quint#1278 in place, so we can
generate the correct, flattened fixtures.
Copy link
Collaborator

@bugarela bugarela left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the many super helpful comments ✨

Co-authored-by: Gabriela Moreira <gabrielamoreiramafra@gmail.com>
@shonfeder shonfeder enabled auto-merge December 1, 2023 00:39
@shonfeder shonfeder merged commit cb437eb into main Dec 1, 2023
10 checks passed
shonfeder pushed a commit that referenced this pull request Dec 1, 2023
This is a followup to #2783, which omitted support for the "wildcard",
default match case in match expressions.
@shonfeder shonfeder deleted the quint/1034/sum-types branch December 4, 2023 00:36
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.

Support conversion of sum types in Apalache
4 participants