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

[Docs] [Builtins] Add Note [How to add a built-in function: simple cases] #4338

Conversation

effectfully
Copy link
Contributor

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.

@effectfully effectfully force-pushed the effectfully/builtins/add-note-how-to-add-a-simple-builtin-function branch from ddec59f to 0dc4306 Compare January 16, 2022 23:40
Comment on lines +322 to +323
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.
Copy link
Contributor Author

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".

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.

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
Copy link
Contributor

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 Integers. Remind me, is there an ironclad reason why we don't do this?

Copy link
Contributor

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...

Copy link
Contributor Author

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".

Copy link
Contributor

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...

Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@kwxm kwxm left a 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.

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs Outdated Show resolved Hide resolved
plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs Outdated Show resolved Hide resolved
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
Copy link
Contributor

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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?

Copy link
Contributor

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.)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Contributor

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).

Copy link
Contributor Author

@effectfully effectfully Jan 23, 2022

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.

Copy link
Contributor Author

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.

plutus-core/plutus-core/src/PlutusCore/Default/Builtins.hs Outdated Show resolved Hide resolved
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
Copy link
Contributor

Choose a reason for hiding this comment

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

OR ELSE?

Copy link
Contributor Author

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>
@effectfully
Copy link
Contributor Author

Comments addressed (only by responding to them), merging.

@effectfully effectfully merged commit 8abffd7 into master Jan 23, 2022
@effectfully effectfully deleted the effectfully/builtins/add-note-how-to-add-a-simple-builtin-function branch January 23, 2022 06:00
MaximilianAlgehed pushed a commit to Quviq/plutus that referenced this pull request Mar 3, 2022
…#4338)

Co-authored-by: Kenneth MacKenzie <kwxm@inf.ed.ac.uk>
@effectfully effectfully changed the title Add Note [How to add a built-in function: simple cases] [Docs] [Builtins] Add Note [How to add a built-in function: simple cases] Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants