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

Enforce return types on interface view types #14570

Closed
Tracked by #14569
cocreature opened this issue Jul 28, 2022 · 10 comments
Closed
Tracked by #14569

Enforce return types on interface view types #14570

cocreature opened this issue Jul 28, 2022 · 10 comments
Assignees
Labels
roadmap/interfaces https://digitalasset.atlassian.net/browse/DAML-56

Comments

@cocreature
Copy link
Contributor

The design tries to keep the view types equivalent to templates, i.e., records with no type arguments. Daml Engine & the typechecker does not.

We need to change one of the two. I think changing Daml Engine makes more sense.

@cocreature cocreature self-assigned this Jul 28, 2022
@S11001001
Copy link
Contributor

I believe this matches the rules we want:

rawViewType <- toIfaceType(name, astIf.view)
viewType <- rawViewType match {
case TypeCon(TypeConName(tcn), Seq()) => \/-(Some(tcn))
case TypePrim(PrimType.Unit, _) => \/-(None)
case _ =>
invalidDataTypeDefinition(
name,
s"interface view type ${astIf.view.pretty} must be either a no-argument type reference or unit",
)

In other words

  1. a reference to a record type with no type arguments, or
  2. unit

So the LF representation of DefInterface#view can be a lot smaller than Type.

@cocreature
Copy link
Contributor Author

I'm not sure we want to allow unit. That still means we cannot just expose a Record on the ledger API.

@S11001001
Copy link
Contributor

S11001001 commented Jul 28, 2022

We can use absence of a Record in the InterfaceView as a representation of (). e: This would mean either it's () or include_interface_views=false. Is that a problem? e2: Or absence of the InterfaceView itself means the latter. I'm not 100% sure of the semantics here, as I've only been concerned with cases where include_interface_views=true.

One thing that was unclear from the design was whether a view would be required. It makes sense to me that it wouldn't be required, but permitting () is functionally identical.

On the ledger API, we can interpret absence of a view as just stated, though it might make more sense to just reject transaction filters that specify interfaces whose viewtype is ().

@cocreature
Copy link
Contributor Author

Currently we make it mandatory and I’d keep this. It means interface subscriptions always work exactly like template subscriptions and there are no interfaces you cannot subscribe to or get no meaningful return value. Relaxing it later to make them optional is also backwards compatible while the inverse isn’t so I’d start with the more restricted version.

If in our tests this becomes fairly annoying, we can define a shared record type (like Archive) that we use instead of unit where we don’t care about the view.

@S11001001
Copy link
Contributor

we make it mandatory and I’d keep this

@cocreature Do you mean, "it must be present, and must refer to a record type with no type arguments"?

If in our tests this becomes fairly annoying

It already would be, I'd wager. #14456 contains 79 uses of viewtype ().

@cocreature
Copy link
Contributor Author

Do you mean, "it must be present, and must refer to a record type with no type arguments"?

yes

@dylant-da
Copy link
Contributor

It already would be, I'd wager. #14456 contains 79 uses of viewtype ().

Many of them are typechecked, too, unfortunately.

@cocreature
Copy link
Contributor Author

Many of them are typechecked, too, unfortunately.

True but

data EmptyInterfaceView = EmptyInterfaceView {}

viewtype EmptyInterfaceView

doesn’t seem that terrible.

@dylant-da
Copy link
Contributor

data EmptyInterfaceView = EmptyInterfaceView {}

viewtype EmptyInterfaceView

doesn’t seem that terrible.

You're right it looks fine - started work on this in https://github.com/digital-asset/daml/tree/non-unit-viewtypes

@akrmn
Copy link
Contributor

akrmn commented Sep 7, 2022

Fixed by #14802 and #14932

@akrmn akrmn closed this as completed Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
roadmap/interfaces https://digitalasset.atlassian.net/browse/DAML-56
Projects
None yet
Development

No branches or pull requests

4 participants