Add encodeUtf8/decodeUtf8 for bytestrings and strings. #3288

ghost · 2021-05-31T09:11:33Z

This PR fills the gap in strings/bytestrings functionality and will help for further literal bytestrings support.

Pre-submit checklist:

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

Pre-merge checklist:

Someone approved it
Commits have useful messages
Review clarifications made it into the code
History is moderately tidy; or going to squash-merge

effectfully · 2021-05-31T10:45:04Z

plutus-core/plutus-core/test/Evaluation/ApplyBuiltinName.hs

+        , test_applyBuiltinFunction EncodeUtf8
+        , test_applyBuiltinFunction DecodeUtf8


Please drop these changes, that module is going to be gone once #3208 is merged.

effectfully · 2021-05-31T10:45:10Z

plutus-core/untyped-plutus-core/test/Evaluation/ApplyBuiltinName.hs

+        , test_applyBuiltinFunction EncodeUtf8
+        , test_applyBuiltinFunction DecodeUtf8


effectfully · 2021-05-31T10:53:12Z

plutus-core/plutus-core/src/PlutusCore/Builtins.hs

+    pretty EncodeUtf8           = "encodeUtf8"
+    pretty DecodeUtf8           = "decodeUtf8"


Tangential: @michaelpj I'm so annoyed that we don't derive Pretty in terms of Show for DefaultFun, because of stuff like

pretty GreaterThanEqInteger = "greaterThanEqualsInteger"

Adding that pretty EncodeUtf8 = "encodeUtf8" line (and the other one) manually is stupid redundant work. Should we rename the constructors of DefaultFun to match with the Pretty instance or should we change the Pretty instance to be derived in terms of Show (with the first letter lowered) even if that breaks backwards compatibility?

Backwards compatibility for whom? Nobody uses the parser. I say change it. Not in this PR, though.

ghost · 2021-05-31T13:37:27Z

CI fails with

 serialization round-trip (CBOR):                               FAIL (4.39s)
      ✗ serialization round-trip (CBOR) failed at plutus-core/test/Spec.hs:99:5
        after 15 tests and 12 shrinks.
      
           ┏━━ generators/PlutusCore/Generators/Internal/Utils.hs ━━━
        57 ┃ forAllPretty :: (Monad m, Pretty a) => Gen a -> PropertyT m a
        58 ┃ forAllPretty = forAllWith display
           ┃ │ (program 0.0.1
           ┃ │   (lam a a { (lam a a (builtin encodeUtf8)) a })
           ┃ │ )
        
           ┏━━ plutus-core/test/Spec.hs ━━━
        96 ┃ propCBOR :: Property
        97 ┃ propCBOR = property $ do
        98 ┃     prog <- forAllPretty $ runAstGen genProgram
        99 ┃     Hedgehog.tripping prog serialise deserialiseOrFail
           ┃     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
           ┃     │ ━━━ Intermediate ━━━
           ┃     │ "\246\246\NUL\NUL\SOH\STX\246aa\NUL\NUL\246aa\NUL\ENQ\246\STX\246aa\NUL\NUL\246aa\NUL\t\246\CAN\GS\NUL\246aa\NUL"
           ┃     │ ━━━ - Original) (+ Roundtrip ━━━
           ┃     │ - Right
           ┃     │ -   (Program
           ┃     │ -      ()
           ┃     │ -      (Version () 0 0 1)
           ┃     │ -      (LamAbs
           ┃     │ -         ()
           ┃     │ -         Name { nameString = "a" , nameUnique = Unique { unUnique = 0 } }
           ┃     │ -         (TyVar
           ┃     │ -            ()
           ┃     │ -            TyName
           ┃     │ -              { unTyName =
           ┃     │ -                  Name { nameString = "a" , nameUnique = Unique { unUnique = 0 } }
           ┃     │ -              })
           ┃     │ -         (TyInst
           ┃     │ -            ()
           ┃     │ -            (LamAbs
           ┃     │ -               ()
           ┃     │ -               Name { nameString = "a" , nameUnique = Unique { unUnique = 0 } }
           ┃     │ -               (TyVar
           ┃     │ -                  ()
           ┃     │ -                  TyName
           ┃     │ -                    { unTyName =
           ┃     │ -                        Name { nameString = "a" , nameUnique = Unique { unUnique = 0 } }
           ┃     │ -                    })
           ┃     │ -               (Builtin () EncodeUtf8))
           ┃     │ -            (TyVar
           ┃     │ -               ()
           ┃     │ -               TyName
           ┃     │ -                 { unTyName =
           ┃     │ -                     Name { nameString = "a" , nameUnique = Unique { unUnique = 0 } }
           ┃     │ -                 }))))
           ┃     │ + Left (DeserialiseFailure 31 "Failed to decode BuiltinName")
      
        This failure can be reproduced by running:
        > recheck (Size 14) (Seed 2224849573486902166 16519328201700872397) serialization round-trip (CBOR)

🤔

effectfully · 2021-05-31T13:46:18Z

Looking at Builtin.hs I think you simply forgot to update the decode part of the Serialize instance?

ghost · 2021-05-31T17:39:52Z

@effectfully yeah, right, thanks! Missed the warnings from the compiler somehow.

Is there a reason why -Werror is not enabled by default? Or it's too irritating for the development process? Maybe should be enabled on CI?

effectfully · 2021-06-01T02:04:14Z

Missed the warnings from the compiler somehow.

I think there were no warnings? It's decoding that is problematic and it has a bunch of clauses for particular integers and a final catch-all clause, hence we don't get any warnings when we forget to update the decoding function. We need a cocoverage checker! Or do the two "co"s cancel each other and it's verage checker in fact?

Anyway, as long as tests catch such errors, we're fine.

effectfully · 2021-06-01T02:05:24Z

Is there a reason why -Werror is not enabled by default? Or it's too irritating for the development process? Maybe should be enabled on CI?

It is enabled in CI form what I know (@michaelpj, right?). We used to have it enabled for development as well, but people were annoyed and so we switched (I personally don't care either way).

ghost · 2021-06-01T06:51:29Z

It's decoding that is problematic and it has a bunch of clauses for particular integers and a final catch-all clause

Oh, right, my bad, should not write things in the late evening.

michaelpj · 2021-06-01T09:30:43Z

plutus-tx/src/PlutusTx/Builtins.hs

@@ -112,6 +114,10 @@ lessThanByteString = (<)
 greaterThanByteString :: ByteString -> ByteString -> Bool
 greaterThanByteString = (>)

+{-# NOINLINE decodeUtf8 #-}
+decodeUtf8 :: ByteString -> String


haddock for these. Yes, it's somewhat obvious, but it never hurts.

michaelpj · 2021-06-01T13:57:20Z

And yes, we run with -Werror in CI.

Has conflicts now.

michaelpj · 2021-06-02T15:09:16Z

Rebase went wrong, it seems?

ghost · 2021-06-03T05:38:12Z

I think it was the consequence of yesterday's GitHub issues. Updated.

ghost requested review from michaelpj, j-mueller, kwxm and effectfully May 31, 2021 09:11

effectfully approved these changes May 31, 2021

View reviewed changes

michaelpj reviewed Jun 1, 2021

View reviewed changes

michaelpj approved these changes Jun 1, 2021

View reviewed changes

Evgenii Akentev added 3 commits June 3, 2021 10:25

Add encodeUtf8/decodeUtf8 for bytestrings and strings.

392d8e7

Fix: add decode instance for encodeUtf8/decodeUtf8

d436b17

Add haddock for encodeUtf8/decodeUtf8

f88a107

ghost force-pushed the add-encode-decode-string-bytestring branch from b484433 to f88a107 Compare June 3, 2021 05:35

michaelpj merged commit 9658f16 into master Jun 3, 2021

ghost deleted the add-encode-decode-string-bytestring branch June 3, 2021 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add encodeUtf8/decodeUtf8 for bytestrings and strings. #3288

Add encodeUtf8/decodeUtf8 for bytestrings and strings. #3288

ghost commented May 31, 2021

effectfully May 31, 2021

ghost May 31, 2021

effectfully May 31, 2021

ghost May 31, 2021

effectfully May 31, 2021

michaelpj Jun 1, 2021

ghost commented May 31, 2021

effectfully commented May 31, 2021

ghost commented May 31, 2021

effectfully commented Jun 1, 2021

effectfully commented Jun 1, 2021

ghost commented Jun 1, 2021

michaelpj Jun 1, 2021

michaelpj commented Jun 1, 2021

michaelpj commented Jun 2, 2021

ghost commented Jun 3, 2021

		, test_applyBuiltinFunction EncodeUtf8
		, test_applyBuiltinFunction DecodeUtf8

		pretty EncodeUtf8 = "encodeUtf8"
		pretty DecodeUtf8 = "decodeUtf8"

Add encodeUtf8/decodeUtf8 for bytestrings and strings. #3288

Add encodeUtf8/decodeUtf8 for bytestrings and strings. #3288

Conversation

ghost commented May 31, 2021

effectfully May 31, 2021

Choose a reason for hiding this comment

ghost May 31, 2021

Choose a reason for hiding this comment

effectfully May 31, 2021

Choose a reason for hiding this comment

ghost May 31, 2021

Choose a reason for hiding this comment

effectfully May 31, 2021

Choose a reason for hiding this comment

michaelpj Jun 1, 2021

Choose a reason for hiding this comment

ghost commented May 31, 2021

effectfully commented May 31, 2021

ghost commented May 31, 2021

effectfully commented Jun 1, 2021

effectfully commented Jun 1, 2021

ghost commented Jun 1, 2021

michaelpj Jun 1, 2021

Choose a reason for hiding this comment

michaelpj commented Jun 1, 2021

michaelpj commented Jun 2, 2021

ghost commented Jun 3, 2021