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

[iamdanfox]

What happened?

In the apollo codebases, we use unions pretty heavily and on the whole they're pretty great for compile-time safety and forcing devs to consider implications of changes everywhere. That being said, the verbosity of visitors is pretty frustrating from a readability perspective. Building visitors using the new shorthand builders is a lot better than the long anonymous classes, but limitations Java's type inference seems to require us to specify return types upfront so we get stuff like this. It also frustrates me that performance sensitive codepaths jump through indirection to avoid constructing and allocating a visitor on every invocation.

return reportedServiceState.accept(ReportedServiceState.Visitor.<ApolloReportedServiceState>builder()
                .assetServer(assetServerReportedServiceState -> ...)
                .blueGreen(blueGreenReportedServiceState -> ...)

What did you want to happen?

With LTS Java 17 arriving in a couple of months (14th Sep GA), we'll get access to Sealed Classes and a preview of pattern matching for switch expressions.

This means that java has first-class support for java unions, and I'd really like to be able to take advantage of the improved readability:

static String switchExpression(MyUnion union) {
    return switch (union) {
        case FooVariant foo -> String.format("foo %s", foo);
        case BarVariant bar -> String.format("bar %s", bar);
        case BazVariant baz -> String.format("baz %f", baz);
        default -> union.toString();
    };
}

Given how many variants some unions have, I reckon it's probably fine to emit code for these across different files:

sealed interface Celestial 
    permits Planet, Star, Comet { ... }

final class Planet implements Celestial { ... }
final class Star   implements Celestial { ... }
final class Comet  implements Celestial { ... }

Some open questions are:

• would we keep around a final class UnknownVariant implements Celestial { ... } to make consumers always acknowledge the possibility that the server might have started returning a new variant which their code wasn't compiled to handle.
• should we keep around the accept(Visitor) method? (I think yes, because it allows a convenient migration)

[carterkozak]

This is a fun idea, although I'm not sure when we'd want to implement it. If APIs are published, we generally don't want to require the latest java release for some time, but perhaps given jdk17 is an LTS we'll be able to move more quickly?

I'm in favor of using language features rather than reinventing them ourselves. Our visitor implementation gives us some guardrails, but we can empower engineers better by using standard APIs that they're already familiar with (or that they learn by leveraging the code we generate).

[gabrielsimoes]

Hey guys, wondering if we could revisit this idea here. I think most teams are already using java 17 features if not running services on java 17. This would be a major improvement to some of our usage of visitors, and allow for much simpler patterns than visitors allow for.

[iamdanfox]

In July 2022 I had a stab at this (see also the #hackweek-2022-sealed-unions channel). Relevant PRs we worked on include:

1. [draft] Generate 'sealed interfaces' for Conjure unions (feature flagged) #1838
2. runtime Add the --enable-preview flag based on the Baseline-Enable-Preview jar manifest attribute sls-packaging#1365
3. automation to rewrite visitors [draft] PreferUnionSwitch gradle-baseline#2321

There were a few unsolved problems for the first PR... one was that that javapoet library didn't support some java17 preview features we needed. Another was I haven't yet figured out how to name the classes in the switch arms, because once we start generating these we would need to be very careful to not break backcompat.

The complexity here is I don't want to generate names that clash with the union variant. I think it's quite common for conjure authors to use the same name for the union variant and also for the class it will be deserialized in: cat: Cat.

Animal:
  union:
    cat: Cat
    dog: Dog

return switch (animal) {
        case Cat cat -> cat.get().getAge()
          // ^^^ how to name this to avoid clashing with the user's defined 'Cat' type

[carterkozak]

The current blocker is getting everything actually upgraded to jdk17. We’re 98% there, but we need the rest before we can really roll out conjure features that rely on new language features. You can argue that we can flag in/out, but that adds complexity not only to conjure, but to those using conjure generated from projects that make incorrect assumptions about their consumers.
This isn’t far off anymore (likely a few months)