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 GenMap to DAML-LF 1.dev spec #3260

Merged
5 commits merged into from
Nov 11, 2019
Merged

Add GenMap to DAML-LF 1.dev spec #3260

5 commits merged into from
Nov 11, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 28, 2019

This PR advances the implementation of #2256.

It introduces a new 'GenMap' primitive type in DAML-LF that generalizes the 'Map' primitive type. Here's a comprehensive summary of the differences between the two types:

  • 'GenMap' takes two type arguments: the type is 'GenMap' k v instead of 'Map' v. The first type argument to 'GenMap' is the key type for the map, where the original 'Map' type only supports 'Text' keys.

  • All the operations on 'GenMap' k v are the same as for 'Map' v except:

    • They are labelled GENMAP_EMPTY, GENMAP_INSERT, GENMAP_LOOKUP, GENMAP_DELETE, GENMAP_KEYS, GENMAP_VALUES, and GENMAP_SIZE, instead of MAP_*.

    • They take two type arguments (the key type and the value type) as opposed to MAP_* operations that only take one type argument (the value type).

    • Because we're phasing out LF tuples, we have GENMAP_KEYS and GENMAP_VALUES instead of GENMAP_LIST. These return a list of keys or values in the order that the items were inserted (rather than in alphabetical order by key). This is because keys can contain Contract IDs, which do not (and cannot) have a predetermined order, and we want the output of GENMAP_KEYS/GENMAP_VALUES to be deterministic regardless of the assignment of Contract IDs.

    • GENMAP_INSERT, GENMAP_LOOKUP and GENMAP_DELETE cause a runtime error whenever they are passed an invalid key, unlike MAP_* operations which can never cause a runtime error. See below for the definition of "invalid key".

    • Key comparison for GENMAP_INSERT, GENMAP_LOOKUP and GENMAP_DELETE is defined according to a key comparison relation ~ᵥ that generalizes text key equivalence. See below for the definition of this relation.

  • We define a generic partial equivalence relation ~ᵥ to be used for key comparison that generalizes the equality relation for all built-in types, and extends it to lists, optionals, enums, variants, record and tuples. It doesn't rely on type information at all, so we maintain type erasure. This relation is not exposed as a built-in operator.

  • We define a key to be valid iff it is equivalent to itself according to the key comparison relation ~ᵥ. This excludes all functions, type abstractions, update/scenario monads, and any composite value that contains them, from being considered valid keys. It also guarantees that ~ᵥ is an equivalence relation over all valid keys.

@ghost ghost changed the title Add GenMap to DAML-LF 1.dev Add GenMap to DAML-LF 1.dev spec Oct 28, 2019
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Looks great, nice work!

daml-lf/spec/daml-lf-1.rst Outdated Show resolved Hide resolved
daml-lf/spec/daml-lf-1.rst Show resolved Hide resolved
daml-lf/spec/daml-lf-1.rst Show resolved Hide resolved
Copy link
Contributor

@hurryabit hurryabit left a comment

Choose a reason for hiding this comment

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

I like the constructive nature of this very much. Thanks for pushing into that direction!

daml-lf/spec/daml-lf-1.rst Show resolved Hide resolved
daml-lf/spec/daml-lf-1.rst Outdated Show resolved Hide resolved
daml-lf/spec/daml-lf-1.rst Show resolved Hide resolved
Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da left a comment

Choose a reason for hiding this comment

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

First draft looks good to me.

daml-lf/spec/daml-lf-1.rst Outdated Show resolved Hide resolved
daml-lf/spec/daml-lf-1.rst Show resolved Hide resolved
daml-lf/spec/daml-lf-1.rst Show resolved Hide resolved
e₁ ~ᵥ e₁' … eₙ ~ᵥ eₙ'
——————————————————————————————————————————————————— GenEqTupleCon
⟨ f₁ = e₁, …, fₘ = eₘ ⟩ ~ᵥ ⟨ f₁ = e₁', …, fₘ = eₘ' ⟩

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are forgetting the "nice" case where keys are maps.
Of course we can a first implementation without map keys.

Copy link
Author

Choose a reason for hiding this comment

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

That's not nice! We probably will need to add them, but I'm worried the "insertion order" issue will mess people up.

But also I don't see map values under the values section (including 'Map'). Probably should add them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can postpone this problem for later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

TextMap (simply called 'Map' in the spec) are not concern with the insertion order.
They are sorted by key when converted to list or serialized in the ledger.

Copy link
Author

Choose a reason for hiding this comment

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

Right. I propose that two 'Map' (i.e. TextMap) are considered equal if they have the same key-value pairs, and two 'GenMap' are considered equal if they have the same key-value pairs in the same insertion order, so they result in the same output when using GENMAP_KEYS and GENMAP_VALUES.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Let's start without it. We can still add it letter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not averse to removing user-defined equality from DAML. Particularly if we find a way to keep the Eq type class as a marker but have no (or at least no user-implementable) methods in it. I'm not sure if that feasible though since Eq is probably wired quite deep into GHC.
@associahedron do you have some time at hand to investigate this a bit?
@bame-da what do you think about this?

Copy link
Author

Choose a reason for hiding this comment

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

@hurryabit I think we can outlaw custom Eq instances in the preprocessor, and tell users to use deriving Eq instead.

Copy link
Author

Choose a reason for hiding this comment

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

We could make it a warning for a while, and turn it into an outright error later.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds reasonable to me. We should then also replace the derived Eq instances with calls to our DAML-LF primitive during our conversion to DAML-LF.

daml-lf/spec/daml-lf-1.rst Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Nov 1, 2019

Updated the spec with reviewer comments.

@ghost ghost marked this pull request as ready for review November 11, 2019 14:18
Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit cacd12f into master Nov 11, 2019
@ghost ghost deleted the genmap branch November 11, 2019 16:36
@ghost ghost mentioned this pull request Nov 13, 2019
bame-da pushed a commit that referenced this pull request Nov 19, 2019
* Add GenMap to DAML-LF 1.dev

* Update LF spec

* remove type constraints again

* Revert "remove type constraints again"

This reverts commit 2081e5b.

* Fix a messed up merge
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants