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

Java Codegen: variants with unserializable cases are now accepted #946

Merged

Conversation

Projects
None yet
3 participants
@gerolf-da
Copy link
Contributor

commented May 6, 2019

If a variant itself is not serializable, but the synthesized record for
one of its constructors is, then said record is returned by the
interface reader in the set of type declarations, when the variant type
itself is not.
When constructing the InterfaceTree in preparation of the codegen, we
previously rejected such a situation.

We now generate Java code for such a synthesized record, as it is a more
generally correct way of interpreting DAML LF (i.e. the DAML compiler
could decide tomorrow to create such multi-component record names for
regular records).

In any case, we consider this to be an edge case, as the synthesized
record for the variant constructor cannot be used directly either from
DAML or the Ledger API.

Pull Request Checklist

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@gerolf-da gerolf-da requested review from bitonic and stefanobaghino-da May 6, 2019

@gerolf-da gerolf-da force-pushed the generate-standalone-types-with-multiple-name-components branch 2 times, most recently from 7b759c3 to dbcf67d May 6, 2019

@stefanobaghino-da
Copy link
Member

left a comment

LGTM. Approving to prevent getting stuck, but please wait for a second review as I contributed to this. And don't forget adding the line to the release notes. 😉

@gerolf-da gerolf-da force-pushed the generate-standalone-types-with-multiple-name-components branch 2 times, most recently from da511a5 to 055dc89 May 6, 2019

@gerolf-da gerolf-da changed the title Java Codegen: Generate types with multiple name components Java Codegen: variants with unserializable cases are now accepted May 7, 2019

@gerolf-da gerolf-da force-pushed the generate-standalone-types-with-multiple-name-components branch 2 times, most recently from aec4551 to dbeb7a8 May 7, 2019

Java Codegen: Generate types with multiple name components
If a variant itself is not serializable, but the synthesized record for
one of its constructors is, then said record is returned by the
interface reader in the set of type declarations, when the variant type
itself is not.
When constructing the InterfaceTree in preparation of the codegen, we
previously rejected such a situation.

We now generate Java code for such a synthesized record, as it is a more
generally correct way of interpreting DAML LF (i.e. the DAML compiler
could decide tomorrow to create such multi-component record names for
regular records).

In any case, we consider this to be an edge case, as the synthesized
record for the variant constructor cannot be used directly either from
DAML or the Ledger API.

@gerolf-da gerolf-da force-pushed the generate-standalone-types-with-multiple-name-components branch from dbeb7a8 to 6e33a16 May 7, 2019

@gerolf-da gerolf-da merged commit 2c04e1f into master May 7, 2019

7 checks passed

Summary 1 potential rule
Details
digital-asset.daml Build #20190507.4 succeeded
Details
digital-asset.daml (Linux) Linux succeeded
Details
digital-asset.daml (Windows) Windows succeeded
Details
digital-asset.daml (macOS) macOS succeeded
Details
digital-asset.daml (perf) perf succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@gerolf-da gerolf-da deleted the generate-standalone-types-with-multiple-name-components branch May 7, 2019

@hurryabit

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

Why did you add the complexity of filtering out this case in the first place?

@gerolf-da

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

@hurryabit: The assumption was the following:

Whenever we see a type with multiple components in the entity name, e.g. Foo.Bar, we assume that there is a variant type Foo that uses the "synthetic" type Foo.Bar for its constructor Bar, e.g. data Foo = Bar with x: Int | Baz.

We made this assumption, because otherwise the user experience for constructing a variant with a constructor wrapping a "synthetic" record type is complete garbage, which isn't noticeable from DAML itself, because DAML doesn't have a fundamental distinction between sum and record types from a user experience/ergonomics perspective.

Now we generate a default Record class for Foo.Bar that is anyway not usable at all either from within DAML or via the Ledger API, because the type cannot be referenced as far as I know.

@hurryabit

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@gerolf-da I see. Thanks for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.