[draft] Generate 'sealed interfaces' for Conjure unions (feature flagged) #1838

iamdanfox · 2022-07-12T13:53:59Z

Before this PR

For hackweek, I'm hoping to enable folks to use Java 17's pattern-matching-switch-expressions (https://openjdk.org/jeps/406) to visit the different possible variants of a Conjure union. My primary goal is to make code more readable, but I think it should come with a performance boost too.

After this PR

==COMMIT_MSG==
A new feature flag (--sealedUnions) allows users to opt-in to generating Java 17 sealed interfaces for their conjure unions.
==COMMIT_MSG==

Other related work is tracked on https://quip/3YEnAt26mmtE.

Outstanding TODOs:

don't require conjure-java to build and publish using a java 17 JVM.
Generate a throwOnKnown method
- do we want to change from a SafeIllegalArgumentException now?
Generate a way to get the Known interface (maybe just optional initially?)
Name escaping of the 'Known' interface name
Testing
- get --enable-preview working in Intellij ([draft] com.palantir.baseline-enable-preview-flag works with IntelliJ integration correctly gradle-baseline#2319) so I can write some tests
- support switch expressions in palantir-java-format (maybe goethe 0.8.0 is all I need... pls mavencentral hurry up)
- exhaustive serialization tests, plus tostring tests, plus equality tests

Possible downsides?

carterkozak · 2022-07-12T14:05:07Z

...java-core/src/test/resources/sealedunions/sealedunions/com/palantir/product/SingleUnion.java

+        defaultImpl = UnknownWrapper.class)
+@JsonSubTypes(@JsonSubTypes.Type(FooWrapper.class))
+@JsonIgnoreProperties(ignoreUnknown = true)
+public sealed interface SingleUnion {


There are a few things I might suggest changing:

We could use this opportunity to become more opinionated about the visitor. It's a lot of code that becomes unnecessary with sealed type pattern matching. What if we drop the visitor entirely?

We're exposing the *Wrapper types as public api, we can rename them dropping the Wrapper suffix to make them more concise. SingleUnion.Foo is pretty clear imo.

Possibly a controversial take: What if we drop the static factories, and use standard records to implement the union type? We don't use constructors in most places, however these are wrappers around a single value (or a single key-value-pair in the case of unknown).

I've got some open threads about some of these questions on https://quip/3YEnAt26mmtE - to summarise:

what if we drop the visitor entirely? - as a long-term state, I definitely agree we should ditch the visitor. My thinking is that it'll probably be most efficient to keep generating it for now, to make the adoption of this flag as easy as possible.

I want to write the error-prone rule to rewrite visitors -> switch expressions. Then maybe when it's fully switched you over, it would flip another feature flag to stop generating these?

I had the tantalising thought that maybe if you have sourceCompat=17, we could actually just start generating these new sealed interfaces for everyone (because sealed interfaces are GA in 17, it's only the pattern-matching-switch-expressions that require the --enable-preview thing)... sadly I think revapi/ABI changes might derail this plan though.

*Wrapper types - 100% agree - this is just a commit I haven't made yet.

Use standard records to implement the union type - yep I'm planning on doing this too, especially because I think destructuring is coming which should make the switch statements even nicer https://openjdk.org/jeps/405.

drop the static factories I think I actually wanna keep them, because I find the IntelliJ autocomplete very natural when you type MyUnion. and then you get a nice dropdown of all the variants.

I also intend to add that .Known sub interface because i think the .throwOnUnknown ergonomics will be quite neat

Re dropping the visitor: I suppose it becomes a sequencing problem -- my preference is generally to reach for the aspirational goal, which we can set in new template projects, and follow-up with the sanest possible migration path.

Re jep405: I thought this was why you were opting into experimental features on jdk17, I don't think the sealed types and such need the experimental pattern matching support otherwise, no?

I find the IntelliJ autocomplete very natural when you type MyUnion.

Wouldn't that be equivalent to new MyUnion.? If we're using records, I'd prefer not to provide two public constructors for the same type, as that forces developers to decide between the two.

The main reason I have been opting-in to --enable-preview is strictly to get https://openjdk.org/jeps/406 (pattern matching for switch). I only just discovered https://openjdk.org/jeps/405 (Record Patterns), but I think it'll be another usability benefit!

carterkozak · 2022-07-15T14:58:54Z

conjure-java-core/src/test/java/com/palantir/conjure/java/types/Union2.java

+@JsonIgnoreProperties(ignoreUnknown = true)
+public sealed interface Union2 {
+
+    sealed interface Known permits Foo, Bar, Baz {}


Suggested change

sealed interface Known permits Foo, Bar, Baz {}

sealed interface Known extends Union2 permits Foo, Bar, Baz {}

Then below we don't need to implement Union2, Known, only Known

Idea here is that we can use MyUnion.Known internally for business logic, ensuring that nothing has to validate against unknown variants, but no casting/validation is required to return or pass MyUnion.Known as a MyUnion.

We may be able to do something clever with safety annotations on MyUnion.Known that can't be guaranteed on the base type with an unknown variant (some issues today since we don't allow safety to be redefined as safer than a supertype, but I think it's reasonable in this instance, trick is ensuring we know when is safe, and when isn't)

carterkozak · 2022-07-15T15:00:28Z

conjure-java-core/src/test/java/com/palantir/conjure/java/types/Union2.java

+    @JsonTypeName("baz")
+    record Baz(long value) implements Union2, Known {
+        @JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
+        public Baz(@JsonSetter("baz") @Nonnull long value) {
+            Preconditions.checkNotNull(value, "baz cannot be null");
+            this.value = value;
+        }
+
+        @JsonProperty(value = "type", index = 0)
+        @SuppressWarnings("UnusedMethod")
+        private String getType() {
+            return "baz";
+        }
+
+        @JsonProperty("baz")
+        public long getValue() {
+            return value;
+        }
+    }


Entirely untested, but I wonder if can do something like this:

Suggested change

@JsonTypeName("baz")

record Baz(long value) implements Union2, Known {

@JsonCreator(mode = JsonCreator.Mode.PROPERTIES)

public Baz(@JsonSetter("baz") @Nonnull long value) {

Preconditions.checkNotNull(value, "baz cannot be null");

this.value = value;

}

@JsonProperty(value = "type", index = 0)

@SuppressWarnings("UnusedMethod")

private String getType() {

return "baz";

}

@JsonProperty("baz")

public long getValue() {

return value;

}

}

@JsonTypeName("baz")

@JsonAppend(attrs = @JsonAppend.Attr(propName = "type", value = "baz"), prepend = true)

record Baz(long value) implements Union2, Known {

@JsonCreator(mode = JsonCreator.Mode.PROPERTIES)

public Baz(@JsonSetter("baz") @Nonnull long value) {

Preconditions.checkNotNull(value, "baz cannot be null");

this.value = value;

}

@JsonProperty("baz")

public long getValue() {

return value;

}

}

Diff didn't render how I expected... The above suggests adding an annotation to the record: @JsonAppend(attrs = @JsonAppend.Attr(propName = "type", value = "baz"), prepend = true) and removing the getType() method.

iamdanfox added 11 commits July 11, 2022 18:53

Union2 illustrates the codegen I want

b0178a7

Check serialization and deserialization

1b2eb0a

Add a --sealedUnions option

66b96a4

UnionGenerator WIP

883e899

Commit some new files

2564514

UnionGenerator generates a bit more stuff

77fb53b

static factories look more sensible

5fcd9f5

Generate the subclasses

e1ee2d7

Add the @JsonTypeInfo annotation

1607e90

Hook up the UnknownWrapper

670e61e

Add the 'sealedunions' prefix

b392291

carterkozak reviewed Jul 12, 2022

View reviewed changes

iamdanfox added 10 commits July 12, 2022 15:16

More annotations

789faef

Bring back the full visitor builder

0492f30

Explicitly discourage people from using the Visitor

9c15575

More code

522c851

Hide all the visitors behind another feature flag

294d87b

Simplify the hand-rolled code to match

6bdbc78

Switch to a record!

97d9773

Test for record equality

57cc7f2

Get rid of the 'Wrapper' suffix

64543d0

Remove --enable-preview from gradle

0de3b6a

iamdanfox force-pushed the dfox/sealed-interfaces branch from 9edbb1f to c4adf00 Compare July 12, 2022 18:24

Use jitpack.io to get square/javapoet#840

5ead7ed

iamdanfox force-pushed the dfox/sealed-interfaces branch from c4adf00 to 5ead7ed Compare July 12, 2022 18:26

iamdanfox added the no changelog label Jul 12, 2022

iamdanfox added 4 commits July 12, 2022 19:44

magic p-j-f incantation

52bec4e

Different fork

eac1c08

Add the 'Known' sealed interface

92321ae

Comment out the Union2 stuff until p-j-f works

38624a7

iamdanfox added 11 commits July 12, 2022 20:23

format

27970b6

Reduce diff

e6d716c

Just compile everything with java 17

a09a2bd

Bring back union test

4c1c04a

No incantation

d36994f

Fix escaping for the interface 'Known'

da38b32

format using java 15

d8c25eb

throwOnUnknown method

0dff803

checkstyle and format

acdd0c0

goethe 0.8.0

73d81d6

Include visitors to facilitate error-prone

15e622f

iamdanfox mentioned this pull request Jul 13, 2022

[draft] PreferUnionSwitch palantir/gradle-baseline#2321

Draft

Does this work on CI?

da21fde

carterkozak reviewed Jul 15, 2022

View reviewed changes

iamdanfox mentioned this pull request Mar 27, 2023

[FR] allow opting-in to sealed classes codegen for unions #1403

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[draft] Generate 'sealed interfaces' for Conjure unions (feature flagged) #1838

[draft] Generate 'sealed interfaces' for Conjure unions (feature flagged) #1838

iamdanfox commented Jul 12, 2022 •

edited

Loading

carterkozak Jul 12, 2022

iamdanfox Jul 12, 2022 •

edited

Loading

iamdanfox Jul 12, 2022

carterkozak Jul 12, 2022

iamdanfox Jul 12, 2022

carterkozak Jul 15, 2022

carterkozak Jul 15, 2022

carterkozak Jul 15, 2022

carterkozak Jul 15, 2022

	sealed interface Known permits Foo, Bar, Baz {}
	sealed interface Known extends Union2 permits Foo, Bar, Baz {}

[draft] Generate 'sealed interfaces' for Conjure unions (feature flagged) #1838

Are you sure you want to change the base?

[draft] Generate 'sealed interfaces' for Conjure unions (feature flagged) #1838

Conversation

iamdanfox commented Jul 12, 2022 • edited Loading

Before this PR

After this PR

Possible downsides?

Choose a reason for hiding this comment

iamdanfox Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamdanfox commented Jul 12, 2022 •

edited

Loading

iamdanfox Jul 12, 2022 •

edited

Loading