[improvement] Generating Visitors for conjure enums

[jerzozwierz]

Before this PR

Although Conjure Unions support the visitor pattern out of the box, Enums did not. It is error-prone for server developers, as it's too easy to add an extra value to an enum and forget to update one of the switch clauses referring to it.

After this PR

Server and client maintainers have flexibility in choosing whether they want enum updates to cause a compile break.

[palantirtech]

Thanks for your interest in palantir/conjure-java, @jerzozwierz! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[uschi2000]

We're generally happy with the switch pattern (because it's more concise than the visitor pattern) in conjunction with static analysis to ensure all cases are covered, for instance via error-prone, see https://github.com/google/error-prone/blob/master/docs/bugpattern/MissingCasesInEnumSwitch.md .

[uschi2000]

Would that same pattern work for you, @jerzozwierz ?

[jerzozwierz]

I'm not exactly sure, @uschi2000. Consider the following example:

enum Color { RED, GREEN, BLUE }

String foo(Color color) {
    switch(color) {
        case RED:
            return "I love this color";
        case GREEN:
            return "I hate this color";
        default:
            throw new AssertionError("This cannot happen");
    }
}

Since the default clause is present, error-prone will not complain (at least according to my understanding, happy to be proven wrong).
In addition, if the default clause was replaced with case BLUE:, the code wouldn't compile at all. My point here being that most switches will end up having the default clause, just for the sake of making the compiler happy – and this "turns off" error-prone checks as a collateral.

[uschi2000]

Your analysis is correct. Interestingly, I can't actually remember encountering such a case in practice. Thoughts, @markelliot ?

[markelliot]

I think we and errorprone might recommend putting the asserterror after the switch statement to cause compilation to succeed and errorprone to indicate missing handling of BLUE.

…

________________________________ From: Robert Fink <[email protected]> Sent: Friday, December 7, 2018 12:31 AM To: palantir/conjure-java Cc: Mark Elliot; Mention Subject: Re: [palantir/conjure-java] [improvement] Generating Visitors for conjure enums (#132) Your analysis is correct. Interestingly, I can't actually remember encountering such a case in practice. Thoughts, @markelliot [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_markelliot&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=KiYrOSxAnZuw83jfBtgU-2cwQ9URQgSiJaZa3SOY4FA&s=lEKLlM_e_v-b6eraV7XH9BGwjKsUSZd87PNO0DdvsFg&e=> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_palantir_conjure-2Djava_pull_132-23issuecomment-2D445068691&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=KiYrOSxAnZuw83jfBtgU-2cwQ9URQgSiJaZa3SOY4FA&s=5HyetcfbIdyR2FpW1yMZsp2FtylUYzVjIKDxM6F-ueg&e=>, or mute the thread [github.com]<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAp3Q-2DgaEhAiQNO7joNvlh2FPl1bRpFRks5u2ajFgaJpZM4ZHdPf&d=DwMCaQ&c=izlc9mHr637UR4lpLEZLFFS3Vn2UXBrZ4tFb6oOnmz8&r=Vp81aOxWZvuVgVK_wp-VF3pIYG92B19LcCw6XKeYC0U&m=KiYrOSxAnZuw83jfBtgU-2cwQ9URQgSiJaZa3SOY4FA&s=E3Dkv2xj0bFj6cw-DVI74uZtzQNyp6A4YN9fy523oOI&e=>.

[jerzozwierz]

I see your point, and I obviously agree that using plain-vanilla switch is much simpler and concise than creating visitors here and there. Nevertheless, I'm still pretty convinced that we should leave the decision whether to rely on static analyzers or to have compile-time guarantees out of the box (i.e. use a Visitor), at a cost of slightly less pretty code.

[jerzozwierz]

Also tagging @iamdanfox for thoughts

[carterkozak]

I didn't see this PR when I put together #142, apologies for duplicating effort. Strongly in favor visitors for enums, it's much easier to build code to fail when cases aren't all covered.

Edit:
Either PR is fine with me, this one needs tests, support for enum values with multiple words (e.g. FOO_BAR), doesn't need handling for disabing safe enums (they're not configurable). No need to use the get() or toString() methods rather than hitting internal fields directly.

[carterkozak]

Error-prone is a great tool, but I don't think we should lean it for problems that the java compiler is a better fit for. Furthermore I'm not confident that all applications using conjure have this error-prone check enabled, and more importantly we cannot force all consumers of conjure-generated clients to enable the right checks.

[pkoenig10]

I have also encountered the problem mentioned by @jerzozwierz. This is especially problematic if an API adds new enum values, since this has the potential to silently cause breaks.

In palantir/gradle-baseline#481, I added a error-prone check that suggests avoiding default cases in switch statements to ensure that the MissingCasesInEnumSwitch check catches these types of bugs.

[iamdanfox]

Thanks for pushing this issue forwards @jerzozwierz, just closing out to focus on #142 as I think it achieves the same outcomes.