[Builtins] Add 'MakeBuiltinMeaning' and type errors doctests #4649
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
effectfully
force-pushed
the
effectfully/builtins/add-MakeBuiltinMeaning
branch
from
8b7de14 to
1fa9f51
Compare
|
Sorry, don't review yet, I need to fix something. |
effectfully
force-pushed
the
effectfully/builtins/add-MakeBuiltinMeaning
branch
from
1fa9f51 to
108bac9
Compare
|
/benchmark plutus-benchmark:validation |
|
Comparing benchmark results of 'plutus-benchmark:validation' on '207a979bd' (base) and '108bac959' (PR) Results table
|
|
^ looks like a tiny speedup, but I don't see why this PR would give us a speedup, so must be noise. Ready for review. |
effectfully
commented
May 25, 2022
| -- <interactive>:1:1: error: | ||
| -- • A built-in function must take at least one type or term argument | ||
| -- ‘Bool’ is a built-in type so you can embed any of its values as a constant | ||
| -- If you still want a built-in function, add a dummy ‘()’ argument |
There was a problem hiding this comment.
It would be nice to turn those into actual doctests, but that's definitely not for now.
michaelpj
approved these changes
May 25, 2022
zliu41
reviewed
May 26, 2022
| -- Are you trying to define a polymorphic built-in function over a polymorphic type? | ||
| -- In that case you need to wrap all polymorphic built-in types having type variables | ||
| -- in them with either ‘SomeConstant’ or ‘Opaque’ depending on whether its the type | ||
| -- of an argument or the type of the result, respectively |
There was a problem hiding this comment.
This sounds like the distinction between SomeConstant and Opaque is that one is for argument and the other is for result, but that's not the case, right?
There was a problem hiding this comment.
It's not the case, but previously it was the case that if you were dealing with a polymorphic built-in type, you had to always use SomeConstant for the argument and Opaque for the result for efficiency reasons. I'm not sure if there's any difference now that we've done so much work on making sure everything related to builtins is properly inlined. I will check that separately. Created PLT-290.
zliu41
approved these changes
May 26, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This polishes the
PlutusCore.Builtin.Meaningmodule and adds another tool toPlutusCore.Builtin.Debugallowing us to debug the wholeBuiltinMeaninginference pipeline.