digital-asset / daml

The Daml smart contract language
https://www.digitalasset.com/developers
797 stars 199 forks source link

Make JSON encoding of DAML-LF variants easier to pattern match on #3622

Closed hurryabit closed 4 years ago

hurryabit commented 4 years ago

Currently, values of DAML-LF variant types like

variant Foo =
  | Bar Int
  | Baz Foo.Baz
record Foo.Baz = {baz : Text}

get encoded to JSON values like

{ "Bar": 5 }
{ "Baz": {"baz": "text"} }

"Pattern matching" on this representation in JavaScript/TypeScript is a bit inconvenient since you have to build a chain of if-statements and add a final unreachable else branch.

I suggest we rather follow the approach described in the TypeScript handbook, i.e., represent the same values as

{ "type": "Bar", "data": "Int" }
{ "type": "Baz", "data": {"baz": "text"} }

I'm also open to inlining the record in the second case, i.e.

{ "type": "Baz", "baz": "text" }

but I don't care too much.

SamirTalwar commented 4 years ago

Seconded. If you’re going to in-line the data, I recommend using a key for the type field with a reserved symbol (perhaps ”@type” or something to avoid conflicts.

hurryabit commented 4 years ago

@S11001001 @leo-da any opinions on this?

cocreature commented 4 years ago

I fully support the idea of this proposal but I would prefer if we didn’t use the name type. We already have the concept of types in DAML-LF and this field does not represent a type but a variant constructor name. tag seems to be fairly popular ime and while we don’t use the name in DAML-LF, I think the potential for confusion is much lower than using type.

leo-da commented 4 years ago

We just chatted with @S11001001 about this feature. This can be done, given it is prioritized.

I personally don't know much about pattern matching in JavaScript/TypeScript. My only concern is that the proposed JSON encoding for variants will be more verbose and harder to interpret by humans, especially when debugging nested variants.

leo-da commented 4 years ago

What about Optional variant? It is a special case, described here: https://github.com/digital-asset/daml/blob/e13d9624ad00cb9bd07c5107f51cb6808ce2fd1e/docs/source/json-api/lf-value-specification.rst#optional

S11001001 commented 4 years ago

@leo-da Optional isn't a Variant at all in the language of LF, so I don't consider it in scope of this issue.

bame-da commented 4 years ago

I'd like to get the API stable as soon as possible, and this seems to be fairly universally supported change. Please prioritise.

leo-da commented 4 years ago

@hurryabit so what do we call it type, %type, tag, typeTag, variant or dataConstructor?

hurryabit commented 4 years ago

I prefer tag since people call these types "tagged unions". We also need a field name for the payload. I suggest value. @bame-da do you have an opinion?

bame-da commented 4 years ago

No, I'll leave it to your judgement.

On Wed, 18 Dec 2019, 13:07 Martin Huschenbett, notifications@github.com wrote:

I prefer tag since people call these types "tagged unions". We also need a field name for the payload. I suggest value. @bame-da https://github.com/bame-da do you have an opinion?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/digital-asset/daml/issues/3622?email_source=notifications&email_token=AJW7WQRYCCYQQKME3ABPXRLQZIG7XA5CNFSM4JRNBUV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHF5AYY#issuecomment-567005283, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJW7WQXYC3BHM62GY5H6QBTQZIG7XANCNFSM4JRNBUVQ .

-- This message, and any attachments, is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at  http://www.digitalasset.com/emaildisclaimer.html http://www.digitalasset.com/emaildisclaimer.html. If you are not the intended recipient, please delete this message.

leo-da commented 4 years ago

The following DAML

data Qux =  Quux | Quuz

gets compiled into DAML LF Enum:

JsonEncodingTest:Qux -> Normal(DefDataType(ImmArray.ImmArraySeq(),Enum(ImmArray.ImmArraySeq(Quux, Quuz)))),

We currently encode Enums as JsStrings. I am changing it, to keep it consistent with Variants. So, it will be encoded as:

{"tag": "Quux"}
S11001001 commented 4 years ago

I am changing it, to keep it consistent with Variants.

This opposes the goal of introducing enums in the first place. Keep enums as strings; this should only change variants that are LF variants.

hurryabit commented 4 years ago

@leo-da Please keep enums as strings. That seems to be idiomatic JavaScript/TypeScript. There's no problem with enums and variants being different. That was pretty much why we introduced enums in the first place.