Make BuiltinByteString and BuiltInString more opaque #4206

treeowl · 2021-11-11T10:16:18Z

Hopefully fixes #4193.

Pre-submit checklist:

Branch
- Tests are provided (if possible)
- Commit sequence broadly makes sense
- Key commits have useful messages
- Relevant tickets are mentioned in commit messages
- Formatting, materialized Nix files, PNG optimization, etc. are updated
PR
- Self-reviewed the diff
- Useful pull request description
- Reviewer requested

Hopefully fixes IntersectMBO#4193.

treeowl · 2021-11-11T10:30:07Z

@michaelpj, let me know what you think.

treeowl · 2021-11-11T10:31:14Z

@phadej, your review would be appreciated too, of course. And I am rather curious what type of owl has possessed you.

plutus-tx/src/PlutusTx/Builtins/Internal.hs

treeowl · 2021-11-11T22:54:10Z

Note that in this representation, a BuiltinByteString in memory looks exactly the same as a ByteString; it just won't ever be unpacked. In theory, GHC could break this, by treating BuiltinByteString as a product type, but it doesn't do so at present. If that ever changes, we can work around it by making the Void field lazy. But that will be a bit more verbose to deal with and perhaps increase (off-chain) code size, so I think we should avoid that unless and until it's necessary.

`GHC.Exts.lazy`, like `GHC.Exts.noinline`, is a magical function with no unfolding. GHC erases it in Core Prep, which happens after we interrupt GHC's compilation process, so we have to erase it ourselves. We also need to `NOINLINE` the definition of `error`, because that's what the Plutus compiler is looking for.

treeowl · 2021-11-12T04:26:58Z

Some tests fail because compilation has changed. How do I determine if these changes are acceptable?

phadej · 2021-11-12T06:47:44Z

Note that in this representation, a BuiltinByteString in memory looks exactly the same as a ByteString;

Is it? I think there is an additional word for a header of wrapper data (somewhere the constructor tag has to go). Not bad, but not exactly the same. (EDIT: If that word matters, maybe ShortByteString should be used in the first place).

treeowl · 2021-11-12T07:03:56Z

@phadej, no, a heap object (e.g., any lifted data) always has a header word to point to the info table and entry code. This will be used by the mutator for lifted data any time the pointer to the object hasn't been tagged (or when there are too many constructors and the tag isn't sufficient to identify which). It's also used by the garbage collector every time to get the object size, pointer layout, etc.

treeowl · 2021-11-12T10:21:09Z

@michaelpj , your thoughts on the test deviations would be appreciated. I'm also curious how I can run those particular tests locally.

treeowl · 2021-11-12T22:20:18Z

@michaelpj, a lot of the compilation changes seem to be removal of redundant eta expansion. I'm guessing this has to do with the fact that GHC can now see the arities of the builtin functions. What's a bit odd is that strict bindings are then replaced by nonstrict ones. Guess: the Plutus compiler is assuming that things like addInteger might be thunks that shouldn't be evaluated. This is wrong. And the Plutus compiler should be able to see that it's wrong, by looking at the function arities calculated by GHC. Thunks can only ever have arity 0, so nothing with a positive arity is a thunk.

phadej · 2021-11-12T22:37:49Z

plutus-tx/src/PlutusTx/Builtins/Internal.hs

+trickier part is getting the `Rep` right. The `Rep` includes metadata about
+strictness, unpacking, constructor names, record selectors, etc. Some aspects
+are difficult or impossible to get right manually, particularly the exact
+package name and the "decided strictness". So instead of building the `Rep`


You can get the "package name" (actually the unit-id) with CURRENT_PACKAGE_KEY CPP macro.
OTOH using the fake type with generic trick is quite clever :)

@phadej, using a fake type is the only way I know to be sure the DecidedStrictness is right no matter how the module is compiled, and I think it's easier to see that the construction is correct than it would be to do it all by hand anyway. Copying the module name from the fake type makes it robust against module renaming too. For the sake of my curiosity, what is a unit-id?

what is a unit-id

It's a difficult question to answer exhaustively, but a short version is:

unit-ids are the identifiers libraries (or more generally units) are registered with in package db (or more generally identified by GHC). Package id (which is package version + package number) is metadata used by humans. For ghc boot libraries unit-ids and package-ids are the same, for cabal installed packages (in store) they are different:

For example:

% head -n 10 /cabal/store/ghc-9.0.1/package.db/aeson-2.0.2.0-ceb356429ca3538b1da748cbb4d8187275f311ecac1e7bd3d68bd0e65c65d68a.conf name: aeson version: 2.0.2.0 visibility: public id: aeson-2.0.2.0-ceb356429ca3538b1da748cbb4d8187275f311ecac1e7bd3d68bd0e65c65d68a key: aeson-2.0.2.0-ceb356429ca3538b1da748cbb4d8187275f311ecac1e7bd3d68bd0e65c65d68a license: BSD-3-Clause

(key was the predecessor of unit-ids before Backpack, IIRC, now there for compatibility reasons).

Unit-ids may be arbitrary strings (restricted alphabet), they don't need to start with package-id, it's there to help debugging.

treeowl · 2021-11-12T23:15:06Z

Hmm... Looking at the compiler code, it seems there already is logic for determining whether applications of builtins are saturated, so I don't know why this is happening. Hopefully someone who knows more about the Plutus compiler can explain.

kk-hainq · 2021-11-13T21:58:20Z

@treeowl I have no idea what is going on but it looks very interesting! I'll see if I could find anything on the interesting effects here.

Also, you can run local tests with cd plutus-tx-plugin && cabal test. You can also nuke all golden files find . -type f -name '*.golden' -delete, to re-generate with the new compiler changes in the next cabal test run. I wonder if committing the changes to this PR makes it easier for others to add inline comments?

michaelpj · 2021-11-19T16:30:02Z

Sorry, we had a workshop all last week. I'll try to look at this next week.

L-as · 2022-01-19T15:24:44Z

Any chance this could be merged?

michaelpj · 2022-01-19T16:56:16Z

Sorry I have been procrastinating on this one. Part of the issue is that I don't really understand the problem this is trying to solve, nor the solution. I would at least like to see a test or two that behave badly before this PR and better afterwards.

anton-k · 2022-01-20T07:29:29Z

Sorry I have been procrastinating on this one. Part of the issue is that I don't really understand the problem this is trying to solve, nor the solution. I would at least like to see a test or two that behave badly before this PR and better afterwards.

Typical case is to use BuiltinByteString operations with bang patterns. Issue is that we can not use bangs with ByteStrings in plutus code. Bangs are used in plutus to force let-expressions. And when we append two BS (for example we construct token name from Int and BS or two BSs), plutus can not compile the code. So BS computations of this kind are repeated if they are used in let-expressions.

It often bites you when you try to create custom token names and assemble them from parts:

let !tok = computeFooToken (...) (...) 
    !a = expr1 tok 
    !b = expr2 tok
in  expr3 a b

phadej · 2022-02-07T14:20:12Z

An example test is

From b33dfbfc0993c3f0d78a7b7843e84185c1b90e9e Mon Sep 17 00:00:00 2001
From: Oleg Grenrus <oleg@well-typed.com>
Date: Mon, 7 Feb 2022 16:19:03 +0200
Subject: [PATCH] Strict example

---
 plutus-tx-plugin/plutus-tx-plugin.cabal       |  1 +
 plutus-tx-plugin/test/Plugin/Spec.hs          |  2 +
 plutus-tx-plugin/test/Plugin/Strict/Spec.hs   | 41 +++++++++++++++++++
 .../test/Plugin/Strict/strictAdd.plc.golden   |  3 ++
 .../Plugin/Strict/strictAppend.plc.golden     |  0
 5 files changed, 47 insertions(+)
 create mode 100644 plutus-tx-plugin/test/Plugin/Strict/Spec.hs
 create mode 100644 plutus-tx-plugin/test/Plugin/Strict/strictAdd.plc.golden
 create mode 100644 plutus-tx-plugin/test/Plugin/Strict/strictAppend.plc.golden

diff --git a/plutus-tx-plugin/plutus-tx-plugin.cabal b/plutus-tx-plugin/plutus-tx-plugin.cabal
index 9b26d454e..7581b145d 100644
--- a/plutus-tx-plugin/plutus-tx-plugin.cabal
+++ b/plutus-tx-plugin/plutus-tx-plugin.cabal
@@ -101,6 +101,7 @@ test-suite plutus-tx-tests
         Plugin.Typeclasses.Spec
         Plugin.Typeclasses.Lib
         Plugin.Coverage.Spec
+        Plugin.Strict.Spec
         Plugin.Lib
         StdLib.Spec
         TH.Spec
diff --git a/plutus-tx-plugin/test/Plugin/Spec.hs b/plutus-tx-plugin/test/Plugin/Spec.hs
index 3e01d0976..c8b58e2f1 100644
--- a/plutus-tx-plugin/test/Plugin/Spec.hs
+++ b/plutus-tx-plugin/test/Plugin/Spec.hs
@@ -12,6 +12,7 @@ import Plugin.NoTrace.Spec
 import Plugin.Primitives.Spec
 import Plugin.Profiling.Spec
 import Plugin.Typeclasses.Spec
+import Plugin.Strict.Spec
 
 tests :: TestNested
 tests = testNested "Plugin" [
@@ -25,4 +26,5 @@ tests = testNested "Plugin" [
   , typeclasses
   , profiling
   , coverage
+  , strict
   ]
diff --git a/plutus-tx-plugin/test/Plugin/Strict/Spec.hs b/plutus-tx-plugin/test/Plugin/Strict/Spec.hs
new file mode 100644
index 000000000..fefebf000
--- /dev/null
+++ b/plutus-tx-plugin/test/Plugin/Strict/Spec.hs
@@ -0,0 +1,41 @@
+{-# LANGUAGE BangPatterns        #-}
+{-# LANGUAGE DataKinds           #-}
+{-# LANGUAGE OverloadedStrings   #-}
+{-# LANGUAGE ScopedTypeVariables #-}
+{-# LANGUAGE TypeApplications    #-}
+{-# OPTIONS_GHC -ddump-simpl -dsuppress-all #-}
+{-# OPTIONS_GHC -fplugin PlutusTx.Plugin #-}
+{-# OPTIONS_GHC -fplugin-opt PlutusTx.Plugin:defer-errors #-}
+{-# OPTIONS_GHC -fplugin-opt PlutusTx.Plugin:debug-context #-}
+
+module Plugin.Strict.Spec (strict, strictAddExample, strictAppendExample) where
+
+import Test.Tasty.Extras
+
+import PlutusCore.Test
+import PlutusTx.Builtins qualified as Builtins
+import PlutusTx.Code
+import PlutusTx.Lift
+import PlutusTx.Plugin
+import PlutusTx.Prelude qualified as P
+import PlutusTx.Test
+
+import Data.Proxy
+
+strict :: TestNested
+strict = testNested "Strict" [
+    goldenPir "strictAdd" strictAdd
+  , goldenPir "strictAppend" strictAppend
+  ]
+
+strictAdd :: CompiledCode (Integer -> Integer -> Integer)
+strictAdd = plc (Proxy @"strictLet") strictAddExample
+
+strictAddExample :: Integer -> Integer -> Integer
+strictAddExample !x !y = Builtins.addInteger x y
+
+strictAppend :: CompiledCode (P.BuiltinByteString -> P.BuiltinByteString -> P.BuiltinByteString)
+strictAppend = plc (Proxy @"strictLet") strictAppendExample
+
+strictAppendExample :: P.BuiltinByteString -> P.BuiltinByteString -> P.BuiltinByteString
+strictAppendExample !x !y = Builtins.appendByteString x y
diff --git a/plutus-tx-plugin/test/Plugin/Strict/strictAdd.plc.golden b/plutus-tx-plugin/test/Plugin/Strict/strictAdd.plc.golden
new file mode 100644
index 000000000..8f2b6246b
--- /dev/null
+++ b/plutus-tx-plugin/test/Plugin/Strict/strictAdd.plc.golden
@@ -0,0 +1,3 @@
+(program
+  (lam x (con integer) (lam y (con integer) [ [ (builtin addInteger) x ] y ]))
+)
\ No newline at end of file
diff --git a/plutus-tx-plugin/test/Plugin/Strict/strictAppend.plc.golden b/plutus-tx-plugin/test/Plugin/Strict/strictAppend.plc.golden
new file mode 100644
index 000000000..e69de29bb
-- 
2.34.1

In current master it fails with

      strictAppend:            FAIL
        Exception: Error: Unsupported feature: Type constructor: Addr#
        Context: Compiling type: Addr#
        Context: Compiling data constructor type: PS
        Context: Compiling type: ByteString
        Context: Compiling expr: case x `cast` <Co:1> of nt_snNO { PS _ _ _ _ ->
                                 case y `cast` <Co:1> of nt_snNW { PS _ _ _ _ ->
                                 appendByteString (nt_snNO `cast` <Co:2>) (nt_snNW `cast` <Co:2>)
                                 }
                                 }
        Context: Compiling expr: case x `cast` <Co:1> of nt_snNO { PS _ _ _ _ ->
                                 case y `cast` <Co:1> of nt_snNW { PS _ _ _ _ ->
                                 appendByteString (nt_snNO `cast` <Co:2>) (nt_snNW `cast` <Co:2>)
                                 }
                                 }
        Context: Compiling expr: \ y ->
                                   case x `cast` <Co:1> of nt_snNO { PS _ _ _ _ ->
                                   case y `cast` <Co:1> of nt_snNW { PS _ _ _ _ ->
                                   appendByteString (nt_snNO `cast` <Co:2>) (nt_snNW `cast` <Co:2>)
                                   }
                                   }
        Context: Compiling expr: \ x y ->
                                   case x `cast` <Co:1> of nt_snNO { PS _ _ _ _ ->
                                   case y `cast` <Co:1> of nt_snNW { PS _ _ _ _ ->
                                   appendByteString (nt_snNO `cast` <Co:2>) (nt_snNW `cast` <Co:2>)
                                   }
                                   }
        Context: Compiling definition of: strictAppendExample
        Context: Compiling expr: strictAppendExample
        Context: Compiling expr at: test/Plugin/Strict/Spec.hs:38:41-59
        Context: Compiling expr: strictAppendExample
        Context: Compiling expr at "strictLet"
        Use -p '/Strict/&&/strictAppend/' to rerun this test only.

Make BuiltinByteString and BuiltInString more opaque

46b4ad7

Hopefully fixes IntersectMBO#4193.

treeowl force-pushed the fixup-bytestring branch from 07c5ef8 to 46b4ad7 Compare November 11, 2021 10:29

phadej reviewed Nov 11, 2021

View reviewed changes

plutus-tx/src/PlutusTx/Builtins/Internal.hs Outdated Show resolved Hide resolved

treeowl force-pushed the fixup-bytestring branch from c652984 to ecb0bf4 Compare November 12, 2021 00:23

Fix up Generic instance

3b09a7a

treeowl force-pushed the fixup-bytestring branch from ecb0bf4 to 3b09a7a Compare November 12, 2021 00:25

More comments

4a8caeb

phadej reviewed Nov 12, 2021

View reviewed changes

Don't allow demand analysis or w/w for error

a35134b

treeowl mentioned this pull request Nov 15, 2021

overzZzealous: Non-lazy Semantics of || and && in Plutus Core #4114

Closed

4 tasks

phadej mentioned this pull request Feb 7, 2022

Strict example #4382

Closed

michaelpj closed this Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BuiltinByteString and BuiltInString more opaque #4206

Make BuiltinByteString and BuiltInString more opaque #4206

treeowl commented Nov 11, 2021 •

edited

Loading

treeowl commented Nov 11, 2021

treeowl commented Nov 11, 2021

treeowl commented Nov 11, 2021 •

edited

Loading

treeowl commented Nov 12, 2021

phadej commented Nov 12, 2021 •

edited

Loading

treeowl commented Nov 12, 2021 •

edited

Loading

treeowl commented Nov 12, 2021

treeowl commented Nov 12, 2021

phadej Nov 12, 2021

treeowl Nov 12, 2021

phadej Nov 12, 2021 •

edited

Loading

treeowl commented Nov 12, 2021

kk-hainq commented Nov 13, 2021

michaelpj commented Nov 19, 2021

L-as commented Jan 19, 2022

michaelpj commented Jan 19, 2022

anton-k commented Jan 20, 2022

phadej commented Feb 7, 2022

Make BuiltinByteString and BuiltInString more opaque #4206

Make BuiltinByteString and BuiltInString more opaque #4206

Conversation

treeowl commented Nov 11, 2021 • edited Loading

treeowl commented Nov 11, 2021

treeowl commented Nov 11, 2021

treeowl commented Nov 11, 2021 • edited Loading

treeowl commented Nov 12, 2021

phadej commented Nov 12, 2021 • edited Loading

treeowl commented Nov 12, 2021 • edited Loading

treeowl commented Nov 12, 2021

treeowl commented Nov 12, 2021

phadej Nov 12, 2021

Choose a reason for hiding this comment

treeowl Nov 12, 2021

Choose a reason for hiding this comment

phadej Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

treeowl commented Nov 12, 2021

kk-hainq commented Nov 13, 2021

michaelpj commented Nov 19, 2021

L-as commented Jan 19, 2022

michaelpj commented Jan 19, 2022

anton-k commented Jan 20, 2022

phadej commented Feb 7, 2022

treeowl commented Nov 11, 2021 •

edited

Loading

treeowl commented Nov 11, 2021 •

edited

Loading

phadej commented Nov 12, 2021 •

edited

Loading

treeowl commented Nov 12, 2021 •

edited

Loading

phadej Nov 12, 2021 •

edited

Loading