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

Generate more sensible constants #460

Merged
merged 2 commits into from
Jan 9, 2019
Merged

Conversation

vmchale
Copy link
Contributor

@vmchale vmchale commented Jan 7, 2019

This fixes #438

Before, we generated bad data for constants sometimes. This filters out that bad data (viz., integers which are out of range), which means that we no longer have to allow errors in the parser roundtrip tests.

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

Filtering out generated stuff is generally a bad idea, because it often increases time required to generate correct test data. It's usually better to generate something correct by construction than to deal with estimating whether test time grows too much or not. Since in this case it's easy to generate correct by construction data, I think we should do that.

You can just generate Integers and ByteStrings, compute the minimal sizes they fit into and construct the corresponding Constants. We have functions that do the latter two actions in Language.PlutusCore.Constant.Make:

makeAutoSizedBuiltinInt :: Integer -> Constant ()
makeAutoSizedBuiltinBS :: BSL.ByteString -> Constant ()

So you only need to generate Integers and ByteStrings and then use the available functions.

@vmchale
Copy link
Contributor Author

vmchale commented Jan 9, 2019

Fair enough.

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

Oh, weird, we check that integers are in bounds during parsing, but do not perform the same check for bytestrings?

@vmchale
Copy link
Contributor Author

vmchale commented Jan 9, 2019

Yes, that is correct. Perhaps put it in slack and we can discuss it at the next meeting if need be?

Copy link
Contributor

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

I'm pretty unconvinced that parsing is the right time for validity checks, but let's save that discussion for another time.

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.

Fix untyped terms generation
3 participants