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

Add some comments on FromBuiltin/ToBuiltin/Lift #5585

Merged
merged 7 commits into from
Nov 22, 2023
Merged

Conversation

michaelpj
Copy link
Contributor

Hopefully this helps a bit.

@kwxm kwxm added the No Changelog Required Add this to skip the Changelog Check label Oct 16, 2023
Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

This does help me understand the setup better, but it doesn't answer my questions regarding various inconsistencies, I replicated those in the review comments.

plutus-tx/src/PlutusTx/Builtins/Class.hs Outdated Show resolved Hide resolved
Comment on lines +212 to +223
For some functions we have two conflicting desires:
- We want to have the unfolding available for the plugin.
- We don't want the function to *actually* get inlined before the plugin runs, since we rely
on being able to see the original function for some reason.

'INLINABLE' achieves the first, but may cause the function to be inlined too soon.
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember RV having a somewhat similar problem. I think the solution I came up at the time was to turn

f = body
{-# INLINEABLE f #-}

into

f = res where
   res = body
   {-# NOINLINE res #-}
{-# INLINE f #-}

Might want to try it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, although the current hack has worked okay for some time.

for types that don't have a separate "normall Haskell" version. For example:
integer. Integer in Plutus Tx user code _is_ the opaque builtin type, we don't
expose a different one. Essentially: if it's a datatype then there's a substantive
conversion.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds like we have two cases:

  1. Bool vs BuiltinBool -- Plutus Tx type vs built-in type, both appear in the ToBuiltin/FromBuiltin instance
  2. Integer vs BuiltinInteger -- Plutus Tx type and built-in type, both appear in the ToBuiltin/FromBuiltin instance

but we actually have a third one:

  1. Data vs BuiltinData -- Plutus Tx type vs built-in type, only the latter appears in the ToBuiltin/FromBuiltin instance

So this Note doesn't clarify this particular quirk for me.

@zliu41 did provide some explanations in slack, so could you please incorporate those if you agree with them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I agree that the Data case is odd. I am actually not sure why it's like that. I'm going to try changing it and see what breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually quite odd, I'm trying to pin down how it should work.

-}

{- Note [Strict conversions to/from unit]
Converting to/from unit *should* be straightforward: just ``const ()`.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Converting to/from unit *should* be straightforward: just ``const ()`.`
Converting to/from unit *should* be straightforward: just `const ()`.

plutus-tx/src/PlutusTx/Lift/Class.hs Show resolved Hide resolved
@michaelpj michaelpj enabled auto-merge (squash) November 22, 2023 12:33
@michaelpj michaelpj merged commit 4c02a25 into master Nov 22, 2023
6 checks passed
@michaelpj michaelpj deleted the mpj/builtins-notes branch November 22, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants