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

[Builtins] Restructure everything #4363

Merged
merged 5 commits into from
Jan 31, 2022

Conversation

effectfully
Copy link
Contributor

This one restructures the whole builtins machinery. It doesn't change any logic whatsoever, just renames a bunch of definitions and moves stuff around.

(Prelude.id :: fi ~ Opaque term (TyAppRep f Integer) => fi -> fi)
(Prelude.id :: fi ~ Opaque val (TyAppRep f Integer) => fi -> fi)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it all started, Opaque is really supposed to be a wrapper around a value, not a general term. I was writing docs and got too disgusted of "we use term for historical reasons" (a couple of years back it did make sense), so I just went ahead and fixed it along with a bunch of other stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is purely cosmetic though, right? We could pass terms (or anything else) to buitlins if we wanted to make PLC lazy or something, but in the current setting the things we're passing are always "values" of some sort. This is the thing I was struggling to find a word for in the specification.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change does make things clearer though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could pass terms (or anything else) to buitlins if we wanted to make PLC lazy or something, but in the current setting the things we're passing are always "values" of some sort.

Not quite... So a couple years ago this whole machinery was expecting an evaluator to be passed along with a term to unlift from. The evaluator would turn the term into a value and then the user could perform the unlifting. Now that we don't pass any evaluators, there's no way we could turn a term into a value. Meaning if you try to unlift a 1 + 1, you don't know how to evaluate it to 2 to unlift the constant. I.e. if 1 + 1 is attempted to be unlifted, there's no way you could do that within the current machinery, irrespective of whether the language is strict or lazy. So only values can be passed to the unlifting machinery.

But it's fine to lift a constant into a term rather than a value. It's also fine to unlift from a Term as long as that term is in fact a value represented as a Term.

I.e. it's fine to implement KnownTypeIn uni val instances for vals that are general terms rather than values, but then it's your responsibility to ensure that only values represented as terms are passed into the instance, because there's no way the instance could handle a non-value.

import PlutusCore.Constant
import PlutusCore.Builtin
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've renamed the Constant folder to Builtin, 'cause that makes more sense.

toSig (acc ++ [Type $ PLC.toTypeAst $ PLC.argOf arr]) schB
toSig (acc ++ [Type $ PLC.toTypeAst $ PLC.argProxy arr]) schB
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per a review comment.

Comment on lines +15 to +17
module PlutusCore.Builtin.Elaborate
( ElaborateFromTo
) where
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 think it's nice to have all this weirdness isolated in a dedicated file.

Comment on lines -45 to +47
runSingKind :: SingKind k -> Kind ()
runSingKind SingType = Type ()
runSingKind (SingKindArrow dom cod) = KindArrow () (runSingKind dom) (runSingKind cod)
demoteKind :: SingKind k -> Kind ()
demoteKind SingType = Type ()
demoteKind (SingKindArrow dom cod) = KindArrow () (demoteKind dom) (demoteKind cod)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per a review comment.

plutus-core/plutus-core/src/PlutusCore/Builtin.hs Outdated Show resolved Hide resolved
type KnownBuiltinTypeAst uni a = KnownTypeAst uni (ElaborateBuiltin a)

type KnownTypeAst :: forall a. (GHC.Type -> GHC.Type) -> a -> GHC.Constraint
class KnownTypeAst uni x where
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that this class is unusual in missing some haddock!

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'll fix that separately.

GetArgs _ = '[]

-- | A class that allows us to derive a monotype for a builtin.
class KnownMonotype val args res a | args res -> a, a -> res where
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this and KnownPolytype be in Elaborate, since they deal with the type-scheme-guessing machinery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it doesn't do any elaboration, it just directly reflects a Haskell type as a TypeScheme. E.g. it would make sense to have this and no elaboration at all (which is how things were aligned originally). I could create a separate module for this thing too, but it seems small and straightforward enough for it not to need a separate module.

--
-- The @rep@ parameter specifies how the type looks on the PLC side (i.e. just like with
-- @Opaque val rep@).
newtype SomeConstant uni (rep :: GHC.Type) = SomeConstant
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't to do with polymorphism, this is just how we represent constants. Maybe should be somewhere 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.

No, it's literally for polymorphism. Note how similar it is to Opaque. It only makes sense to use SomeConstant when you have an argument whose type has type variables in it (or an even fancier Plutus type). Check out how SomeConstant is used in the Default.Builtins and the Examples.Builtins modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, I'm getting confused with ValueOf.

Co-authored-by: Michael Peyton Jones <michael.peyton-jones@iohk.io>
@effectfully effectfully merged commit 748a0c6 into master Jan 31, 2022
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.

This all looks very sensible.

@effectfully effectfully deleted the effectfully/builtins/restructure-everything branch January 31, 2022 19:48
MaximilianAlgehed pushed a commit to Quviq/plutus that referenced this pull request Mar 3, 2022
Co-authored-by: Michael Peyton Jones <michael.peyton-jones@iohk.io>
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