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

Remove bug with integers in return values #240

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

nikolaushuber
Copy link
Contributor

This PR removes a bug/shortcoming within the qcheck-stm plugin when testing functions that return integers.
Gospel has two different types for integers (int & integer), but only int has a pretty-printer defined in STM.
Gospel also automatically adds casting operations for ints to integers where needed.

The code added in this PR dynamically detects where these casts have been added and automatically adds reverse-castings for error pretty-printers where needed.

This responds to #238.

Copy link
Collaborator

@n-osborne n-osborne 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 this PR!

This looks really good.

I rather have the changes in the tests in the same commit than the changes in the code that cause them. I know that this generate a lot of "noise" in the diff, but we can always give git a path to focus on the part we want to inspect. And that way, we have coherent commits.

I've given some thoughts on how to handle the pretty printing of Gospel.integers.
In your very detailed commit message (ddcdb3d)(thanks for that :-)) you describe two ways:

  • Add integer pretty-printers to STM
  • Automatically cast integers back to ints when needed

The problem I see with the second option is that integer_of_int can raise an exception that is not caught in the generated code. Even, if we catch it, we are loosing some information when the integer is larger than an int (not sure it will happen that often though).

Gospel.integerss are implemented as Z.t, I think it would be worth it to ask the STM devs whether it could be intersting to add Z.t support. If not, there is also the solution to add them in the generated code (in the Ty module). We can add them by default: there is a rather high probability that the tested module contains some functions returning an int (casted by Gospel as an integer). In the case there is not, the cost is not prohibitive.

Copy link
Collaborator

@n-osborne n-osborne left a comment

Choose a reason for hiding this comment

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

Very nice :-)

I just made a few located remarks.

plugins/qcheck-stm/src/stm_of_ir.ml Outdated Show resolved Hide resolved
plugins/qcheck-stm/src/stm_of_ir.ml Outdated Show resolved Hide resolved
plugins/qcheck-stm/src/ir_of_gospel.ml Outdated Show resolved Hide resolved
Extends STM.ty with a type for Gospel.integer and includes a smart constructor
to create a value of type ty_show.
Since Gospel.integer is not used in every test we also ignore warning 32
for the unused smart constructor.
nikolaushuber and others added 2 commits July 1, 2024 11:14
Co-authored-by: Nicolas Osborne <nicolas.osborne@tarides.com>
@n-osborne n-osborne merged commit cd2c936 into ocaml-gospel:main Jul 1, 2024
3 checks passed
@n-osborne
Copy link
Collaborator

Thanks!

This pull request was closed.
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.

2 participants