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

[PLC] [Builtins] Removed static typed builtins #983

Conversation

effectfully
Copy link
Contributor

@effectfully effectfully commented May 1, 2019

This removes typed static builtins (integer and bytestring) that previously were special-cased due to size problems.

Do not review yet, it's here mostly because I want CI to check things like the playground for me.

(Who again failed to use draft pull requests? Yep, me!)

@effectfully effectfully self-assigned this May 1, 2019
@effectfully effectfully force-pushed the effectfully/builtins/remove-typed-static-builtins branch 2 times, most recently from 00e2a63 to 08424e5 Compare May 1, 2019 17:21

-- | Convert a Haskell value to the corresponding PLC value.
-- 'Nothing' represents a conversion failure.
makeDynamicBuiltin :: dyn -> Maybe (Term TyName Name ())
makeKnown :: a -> Term TyName Name ()
Copy link
Contributor Author

@effectfully effectfully May 1, 2019

Choose a reason for hiding this comment

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

Removed the Maybe, because we now do not have any failing conversions from Haskell to Plutus Core.

@@ -43,7 +43,6 @@ data BuiltinName = AddInteger
| GreaterThanInteger
| GreaterThanEqInteger
| EqInteger
| IntToByteString
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@@ -78,10 +77,10 @@ instance Monad ConstAppResult where
ConstAppStuck >>= _ = ConstAppStuck
ConstAppError err >>= _ = ConstAppError err

type ConstAppResultDef = ConstAppResult (Value TyName Name ())
type ConstAppResultDef = ConstAppResult (Term TyName Name ())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

type synonyms bitrot too quickly... Perhaps it would be better to turn Value into a newtype wrapper.

Copy link
Contributor

Choose a reason for hiding this comment

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

How do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently we have type Value = Term and we use Value to indicate that a term is evaluated up to WHNF, but things change constantly and something that is no longer supposed to be a value is still marked as such. A newtype would help here.

@effectfully effectfully force-pushed the effectfully/builtins/remove-typed-static-builtins branch from 08424e5 to fe46800 Compare May 1, 2019 18:31
@effectfully
Copy link
Contributor Author

Ready for review, but do not merge yet, because docs are likely to be out of sync.

@effectfully effectfully requested review from michaelpj and vmchale and removed request for michaelpj May 1, 2019 18:33
@effectfully effectfully force-pushed the effectfully/builtins/remove-typed-static-builtins branch 4 times, most recently from 5b53104 to c4dccdf Compare May 2, 2019 11:55
@effectfully effectfully requested a review from michaelpj May 2, 2019 12:06
@effectfully effectfully mentioned this pull request May 2, 2019
@effectfully effectfully force-pushed the effectfully/builtins/remove-typed-static-builtins branch 2 times, most recently from ae970dd to cb263a8 Compare May 2, 2019 14:21
@effectfully
Copy link
Contributor Author

Yeah, there were a lot of outdated docs and comments (related to sizes and other stuff that changed some time ago).

Should be mergeable now, please review.

@@ -78,10 +77,10 @@ instance Monad ConstAppResult where
ConstAppStuck >>= _ = ConstAppStuck
ConstAppError err >>= _ = ConstAppError err

type ConstAppResultDef = ConstAppResult (Value TyName Name ())
type ConstAppResultDef = ConstAppResult (Term TyName Name ())
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you mean?

@effectfully effectfully force-pushed the effectfully/builtins/remove-typed-static-builtins branch from cb263a8 to 10e9997 Compare May 2, 2019 14:56
@effectfully
Copy link
Contributor Author

I'll merge this, because I need it to fix the 1/0 bug.

@effectfully effectfully merged commit 2064f49 into IntersectMBO:master May 2, 2019
@effectfully effectfully deleted the effectfully/builtins/remove-typed-static-builtins branch May 2, 2019 18:23
@effectfully effectfully restored the effectfully/builtins/remove-typed-static-builtins branch May 3, 2019 22:03
@effectfully effectfully deleted the effectfully/builtins/remove-typed-static-builtins branch May 8, 2019 21:59
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.

None yet

2 participants