-
Notifications
You must be signed in to change notification settings - Fork 483
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
[Docs] [Builtins] Add Note [How to add a built-in function: simple cases] #4338
[Docs] [Builtins] Add Note [How to add a built-in function: simple cases] #4338
Conversation
ddec59f
to
0dc4306
Compare
This concludes the list of simple cases. Before we jump to the hard ones, we need to talk about how | ||
polymorphism gets elaborated, so read Note [Elaboration of polymorphism] next. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I created this barrier between the simple cases and complicated ones is because in the simple cases it's really trivial to add a builtin, you don't need any knowledge of the elaboration machinery, how things are represented etc -- you just write the most natural thing and it works. So pretty much anyone can do it and the simple cases cover the majority of usages. But for complicated cases you really have to take some deep dives into all this stuff and so only a trained individual can handle them. Hence I think it's worth distinguishing between "this is really simple, you don't need any extra knowledge and it'll be sufficient in the majority of cases" and "here's a shitload of material to learn before you attempt to do anything of this kind".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great docs!
more simple cases. | ||
|
||
4. Certain types are not built-in, but can be represented via built-in ones. For example, we don't | ||
have 'Int' as a built-in, but we have 'Integer' and we can represent the former in terms of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which still kind of bothers me... I somewhat wish we forced people to explicitly turn things into Integer
s. Remind me, is there an ironclad reason why we don't do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that the builtin application machinery handles this case nicely so people can't make mistakes, it just feels like it's the wrong place to put that logic...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that the builtin application machinery handles this case nicely so people can't make mistakes, it just feels like it's the wrong place to put that logic...
What do you think would be the right place?
I can change the existing KnownTypeIn
instance for Int
to give the user a TypeError
specifying how exactly they're supposed to handle Int
and then the user will follow the instructions and copy-paste some code within the definition of the builtin, but... why? This is more error-prone ("ok, I'm getting some weird error when I'm trying to use Int
, so I'll just use fromIntegral
") and quite less convenient. I don't think there's any ironclad reason why we handle Int
automatically currently, but I don't see at the moment how to change that without making things substantially worse.
There's of course the problem that currently we only special-case Int
while there's also Word64
and plenty of other similar types, but I'm having hopes of creating a general TypeError
saying "this type is neither built-in nor one of the special types, you either want to add a new built-in type or use an existing one, for that do blah-blah-blah".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's weird magic that can confuse people. I always get confused by this because I think "wait, we only have Integer
in our universe, how can this work?" I wouldn't mind having to do it explicitly...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, this is off-topic, I guess.
|
||
Bool -> Opaque term Var0 -> Opaque term Var0 -> Opaque term Var0 | ||
|
||
which then gets digested, so that we can compute what Plutus type it corresponds to. The procedure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this elaborate/digest terminology one you use consistently in the code too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm going to come up with some brand new docs like this one with established terminology etc and then polish existing ones, as well as file structure etc. After so many updates the builtins machinery look quite messy (the code is fine, but how it's documented and how it's aligned is not), so I'll have a separate (or a few) PR on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! It's very helpful to have all this written down very clearly in one place.
to the corresponding 'Int', and if it doesn't, then the builtin application fails. | ||
|
||
For the list of types that can be converted to/from built-in ones look into the file with the | ||
default universe. If you need to add a new such type, just copy-paste what's done for an existing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth saying what the name of that file is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm intentionally avoiding such specifics. We have way too much documentation that is terribly outdated, so nowadays I'm trying to write docs that will last. I think the person adding a new built-in function is supposed to know where the default universe is anyway.
It's completely fine to define a builtin that takes an 'Int' directly, though. How so? That's due | ||
to the fact that the builtin application machinery checks that an 'Integer' is in the bounds of | ||
'Int' before doing the conversion. If the bounds check succeeds, then the 'Integer' gets converted | ||
to the corresponding 'Int', and if it doesn't, then the builtin application fails. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where exactly does that error happen? When the argument is supplied to the function, or when the function is fully applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm wondering about this for the specification, which doesn't mention this automated conversion at all because I was unaware of it or had at last failed to register it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the argument is supplied to the function, or when the function is fully applied?
The former. I'll make it clear in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed my mind, I think this Note shouldn't dive into the specifics of how exactly things get evaluated. I'll create a separate Note.
builtin (out of the scope of this Note) and handling it within the @Flat DefaultFun@ instance | ||
(see Note [Stable encoding of PLC]). | ||
|
||
2. If the type of the denotation has any constrained type variables in it, all of them need to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps emphasise that this is the Haskell type of the denotation and that's where the type variables are coming from (I know the denotation's a Haskell object, but it's easy to get confused when there are two different type systems in play).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent several minutes trying to come up with a way to make that clarification, but it just all looks worse. So I could say "If the Haskell type of the denotation has any constrained type variables in it", but that sounds like a denotation can have a non-Haskell type, which it can't. A builtin can have a Plutus type and a Haskell type, but a denotation is always a Haskell object. Plus, it's really unambiguous here which language is meant, because Plutus Core does not have any constraints. I agree it's easy to get confused in general, but I think in this particular case things are pretty unambiguous, given that the "denotation" term is introduced have a screen above and it's explicitly stated that a denotation is a Haskell implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you think there's a better way of formulating that part, do write it down, I'll include it in a subsequent PR, or feel free to adjust the docs yourself.
But note that | ||
|
||
'EvaluationResult' MUST BE EXPLICIYLY USED FOR ANY FAILING BUILTIN AND THROWING AN EXCEPTION | ||
VIA 'error' OR 'throw' OR ELSE IS NOT ALLOWED AND CAN BE A HUGE VULNERABILITY. MAKE SURE THAT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OR ELSE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorWithoutStackTrace
, throwTo
, you name it.
Co-authored-by: Kenneth MacKenzie <kwxm@inf.ed.ac.uk>
Comments addressed (only by responding to them), merging. |
…#4338) Co-authored-by: Kenneth MacKenzie <kwxm@inf.ed.ac.uk>
I ran out of time, so this is like half-written (I mean, the single topic of adding a built-in function). It's probably good to have these docs reviewed in batches anyway, given how much stuff there's to digest.