Consider disallowing default cases in switch statements

[pkoenig10]

What happened?

If a switch statement has a default case, then the MissingCasesInEnumSwitch check is ineffective. This makes is easy to accidentally not handle new enum variants.

For example, suppose we have:

enum Colors {
    RED,
    GREEN
}

boolean isRedOrBlue(Color color) {
    switch (color) {
        case RED:
            return true;
        case GREEN:
            return false;
        default:
            throw new RuntimeException("Unknown color");
    }
}

And then a BLUE variant is added to the enum:

enum Colors {
    RED,
    GREEN,
    BLUE
}

There will not be a warning that the switch statement does not explicitly handle the BLUE case and that the method is now incorrect.

What did you want to happen?

Each enum variant should be explicitly handled to ensure that we get a warning when new variants are added. To ensure the MissingCasesInEnumSwitch check always checks for missing cases, default cases should be disallowed.

The initial example would change to:

boolean isRedOrBlue(Color color) {
    switch (color) {
        case RED:
            return true;
        case GREEN:
            return false;
    }

    throw new RuntimeException("Unknown color");
}

[iamdanfox]

Closed by #481

[whickman]

hey @iamdanfox @pkoenig10 , why did we do this? there's also this http://errorprone.info/bugpattern/MissingDefault which requires default.

also the docs you linked say "To avoid this ambiguity, the Google Java Style Guide requires each switch statement on an enum type to either handle all values of the enum, or have a default statement group," so it seems like having default is fine.

cc @bwaldrep

[pkoenig10]

The error prone docs also state:

A switch statement for an enum type may omit the default statement group, if it includes explicit cases covering all possible values of that type.

This check enforces our preference for using this strategy to handle enum values. The issue with using a default is that you will not be warned when a new enum variant is added that you may want to handle differently that the current default behavior.

This was primarily motivated by the desire to enforce proper handling of enums returned from API endpoints, which may return values the client does not know about. There are certainly cases where this check is more of a hindrance than a help and in those cases you should feel free to suppress the warning. But having the warning forces you to consider the tradeoff you are making by doing so.

[whickman]

I understand the argument. My concerns: this is a pretty big break for our codebase in a minor version and the preference for enumerating all options doesn't really seem like it was discussed, nor does it appear standard elsewhere.

Would like to have this sort of thing called out in the release notes at a minimum. Breaks like this make excavator changes very painful.

[iamdanfox]

@whickman I think this is actually only a warning? We intentionally only promote things to error when we have high confidence that they should be applied everywhere, and I think this was still in the 'rollout' phase?

EDIT it's actually even lower, SUGGESTION really should not block PRs merging

[bwaldrep]

@iamdanfox You are correct, this is not actually blocking the merge. I got confused by the gradle output with -Werror as to which warnings were blocking and which were just advisory.

[whickman]

Aha, sorry @pkoenig10 and @iamdanfox for all this, then. fine with this being a suggestion