StatementSwitchToExpressionSwitch shows misleading message

[bannmann]

Consider the following switch statement:

switch (mode) {
    case "alfa":
        return Result.A;
    case "bravo":
        return Result.B;
    case "delta":
        return Result.D;
    default:
        log.warn("Ignoring unknown mode indicator '{}'", mode);
        break;
}

Error Prone 2.38.0 shows this error:

Example.java:[21,12] [StatementSwitchToExpressionSwitch] This statement switch can be converted to an equivalent expression switch
    (see https://errorprone.info/bugpattern/StatementSwitchToExpressionSwitch)
  Did you mean 'switch (mode) {'?

I see several issues with StatementSwitchToExpressionSwitch here:

• The "did you mean" message is not actually suggesting a change.
• The claim that this can be "converted to an equivalent expression switch" is wrong as the default branch has no return statement. Maybe in this case, it was the intention that the bug pattern only suggests using arrow case labels to eliminate the potential for unintended fall through?
  • Indeed, when changed as below the error message disappears. However, at least in my view that change is not really for the better, as the version above is already safe (due to each branch having either return or break).

    switch (mode) {
        case "alfa" -> {
            return Result.A;
        }
        case "bravo" -> {
            return Result.B;
        }
        case "delta" -> {
            return Result.D;
        }
        default -> log.warn("Ignoring unknown mode indicator '{}'", mode);
    }
    

Here's the full example:

package com.example;

import lombok.extern.slf4j.Slf4j;

@Slf4j
final class Example {
    private enum Result {
        A,
        B,
        C,
        D,
        E
    }

    public static void main(String[] args) {
        log.info("Result: {}", analyze(args[0], args[1]));
    }

    private static Result analyze(String mode, String extraData) {
        if (mode != null && !mode.isEmpty()) {
            switch (mode) {
                case "alfa":
                    return Result.A;
                case "bravo":
                    return Result.B;
                case "delta":
                    return Result.D;
                default:
                    log.warn("Ignoring unknown mode indicator '{}'", mode);
                    break;
            }
        }

        if (extraData.contains("*")) {
            return Result.C;
        }

        if (extraData.length() % 2 == 0) {
            return Result.E;
        }

        return Result.B;
    }
}

[Stephan202]

The "did you mean" message is not actually suggesting a change.

This is due to #20.

[graememorgan]

However, at least in my view that change is not really for the better, as the version above is already safe (due to each branch having either return or break).

This is an interesting point to discuss, and I suspect it's one we're going to have to wrestle with sooner or later.

I think there's a good argument that arrow switches are always better where possible (where falling through isn't used), because then they can't fall through, and you don't have to inspect each branch to be sure they don't.

[PhilippWendler]

I think there's a good argument that arrow switches are always better where possible (where falling through isn't used), because then they can't fall through, and you don't have to inspect each branch to be sure they don't.

I agree. In addition, code with classic case labels sometimes ends up using a variable declared inside one case in another case.

As one data point, in our project we agreed to rewrite all switches into arrow switches (thanks to Error Prone for helping tremendously with that!) and have now a CI check enforcing this. Fall throughs needed to be rewritten manually, but there was not a single case where the fall through was really worth it (outside of generated parser code).

Another argument might be less confusion with switch expressions. In my experience, because switch expressions and arrow switches were introduced together in Java and are also covered together in tutorials/blog posts/etc., developers tend to mix them up and call everything that looks not like old code a "switch expression". This can be dangerous if they then assume that all these modern switches are checked for exhaustiveness by the compiler (whereas only actual switch expressions are, and arrow rules are an orthogonal feature). If one uses arrow switches exclusively it might clear up this confusion and let developers understand better (or sooner) the actual difference between switch expressions and switch statements.

[bannmann]

In my experience, because switch expressions and arrow switches were introduced together in Java and are also covered together in tutorials/blog posts/etc., developers tend to mix them up and call everything that looks not like old code a "switch expression".

Agree. One such example is this bug pattern - not only the error message that I got (which is obviously not about statement vs expression), but also its documentation, which contrasts the two types as follows:

Statement switch statements: Have a colon between the case and the case’s code.

Expression switch statements Have an arrow between the case and the case’s code.

So this part should be rewritten to at least acknowledge that there can be switch statements that have arrow labels.