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

Convert viewtype and view methods to LF #14456

Merged
merged 43 commits into from
Aug 3, 2022
Merged

Conversation

dylant-da
Copy link
Contributor

@dylant-da dylant-da commented Jul 18, 2022

This implements LF conversion, and typechecks the generated LF.
Builds on #14456 and #14439, will constitute fewer lines when those PRs are merged.

@dylant-da
Copy link
Contributor Author

dylant-da commented Jul 18, 2022

Splitting up #14408
PR for view method and viewtype syntax: #14435
PR for LF protobuf serialization: #14439
PR for LF DefInterface changes & typechecking: #14456
PR for adding EViewInterface function to LF: #14486

Corresponding GHC changes: digital-asset/ghc#128

@dylant-da dylant-da force-pushed the interface-views-lfconversion branch 6 times, most recently from 3486299 to fe85d96 Compare July 20, 2022 15:46
@dylant-da dylant-da force-pushed the interface-views-lfconversion branch 2 times, most recently from f9756c0 to 44f368a Compare July 20, 2022 17:05
@dylant-da dylant-da force-pushed the interface-views-lfconversion branch 4 times, most recently from a7de08b to 053c0bc Compare July 27, 2022 08:33
@dylant-da dylant-da force-pushed the interface-views-lfconversion branch 2 times, most recently from 4515960 to daffa51 Compare July 28, 2022 13:35
@dylant-da
Copy link
Contributor Author

Marking this ready for review.

Git tells me that there are ~128 lines worth of changes to non-daml/non-expected files. There are a few daml files with non-trivial changes, but most of them add unit viewtypes.

@@ -161,3 +164,10 @@ _exerciseInterfaceGuard : forall i t.
ContractId t -> (t -> Bool) -> i -> Bool
_exerciseInterfaceGuard cid tpred ivalue =
tpred (unsafeFromInterface (coerceContractId cid) ivalue)

-- Read: Interface `i` has a view of type `v`
class HasInterfaceView i v | i -> v where
Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes this makes more sense here, thanks

@@ -9,6 +9,7 @@ import DA.Assert ((===))

-- | An interface comment.
interface Token where
viewtype ()
Copy link
Contributor

Choose a reason for hiding this comment

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

this made me wonder, would users get an error for trying to define a method named view?

@@ -85,8 +101,8 @@ Empty Interfaces
:end-before: -- EMPTY_INTERFACE_END

- It is possible (though not necessarily useful) to define an interface without
methods, precondition or choices. In such a case, the ``where`` keyword
can be dropped.
methods, precondition or choices. However, a view type must always be defined,
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for updating the docs :)

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.

Nice work, thank you! Please address the comment on computeInterfaceView before merging but looks good otherwise.

Copy link
Contributor

@akrmn akrmn left a comment

Choose a reason for hiding this comment

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

Looks good to me other than

  • (these two can be addressed in future PRs)
    • error message for trying to define declare a method named view in an interface (not in an implements block)
    • moving the interface declarations checks to da-ghc
  • @cocreature's point about computeInterfaceView

@dylant-da dylant-da merged commit 8e8e0da into main Aug 3, 2022
@dylant-da dylant-da deleted the interface-views-lfconversion branch August 3, 2022 15:23
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.

5 participants