S131 false positive for enhanced switch merged statements (revisited)

[Michi (Michael Henn)]

I’m using

• SonarLint for Eclipse 7.3.0.44160
• Eclipse Version: 2022-03 (4.23.0), Build id: 20220310-1457
• No connected mode

and I’m having the same issue as in S131 false positive for enhanced switch merged statements (issue already closed, so I cannot answer):

public enum Position {
  FIRST,
  SECOND,
  THIRD
}

public class TestClass {
  public void testMethod(Position pos) {
    switch (pos) { // <---- FALSE POSITIVE S131
      case FIRST, SECOND -> doSomething();
      case THIRD -> doSomethingElse();
    }
  }
}

but the solution provided there is not sufficient or appropriate for the following reasons:

1. One suggestion is to “just don’t have a duplicate branch” and using default instead like this:

   switch (pos) {
     case THIRD -> doSomethingElse();
     default -> doSomething();
   }
   

   This will not work if you have multiple “duplicate” branches, like

   switch (pos) {
     case FIRST, SECOND -> doSomething();
     case THIRD, FOURTH -> doSomethingElse();
     case FIFTH, SIXTH -> doSomethingEvenDifferent();
   }
   

   because you cannot cover these with a simple default.

2. Another suggestion is to add a default case.
   This is also unfavorable because if someone adds an entry to the enum (e.g. FOURTH in the example), you won’t notice that you forgot to actually cover that case in the switch and it will just be “covered” by the default, even if you actually needed to do something specifically for the FOURTH case in the switch, but you won’t notice until you actually discover the wrong behavior. No Sonar warning, no compile time error. If you omit the default, you have at least the chance to either get

   • a justified S131 warning or
   • a compiler error (see e.g. Eclipse → Preferences → Java → Compiler → Errors/Warnings → Potential programming problems → Incomplete ‘switch’ cases on enum)

   This argument also applies to 1.

3. This answer

   var result = switch (pos) { // NO S131 warning
     case FIRST, SECOND -> doSomething();
     case THIRD -> doSomethingElse();
   };
   

   is a partial solution, which only works if the switch returns something, so it doesn’t work for functions returning void.
   Furthermore, the answer suggests that this is intended behavior, which I doubt, given the rule description.

---

Anyway, the exception of the rule description of S131 written in the rule itself simply does not apply correctly:

If the switch parameter is an Enum and if all the constants of this enum are used in the case statements, then no default clause is expected.

Therefore, the suggested “solution” in the thread above is not a solution. I should not be compelled to add a default case according to the rule. This still looks like a bug in the rule implementation and the suggested solutions in the thread above do not remedy the problem.

Bottom line is, the rule should not complain if all enum cases are covered in the switch, even not if there is no default.

[ganncamp (G Ann Campbell)]

Hi,

7.5.0.54140 is the latest version of SonarLint for Eclipse. Would you mind upgrading and making sure this is replicable?

 
Thx,
Ann

[Michi (Michael Henn)]

Hi Ann,

thanks for the reply.
I just installed SonarLint 7.5.0.54140 and ran this example:

public class SonarLintRspec131 {
  public enum Fruit {
    APPLE,
    PEAR,
    CHERRY
  }

  public static void main(String... args) {
    Fruit fruit = Fruit.APPLE;
    switch (fruit) {
//  ^^^^^^ False positive S131: Complete cases by adding the missing enum constants or add a default case to this switch
      case APPLE -> System.out.println("Yay, it's an apple!");
      case PEAR, CHERRY -> System.out.println("Awww, it's not an apple!");
    }
    switch (fruit) {
//  ^^^^^^ False positive S131: Complete cases by adding the missing enum constants or add a default case to this switch
      case APPLE:
        System.out.println("Yay, it's an apple!");
        break;
      case PEAR, CHERRY:
        System.out.println("Awww, it's not an apple!");
        break;
    }
  }
}

However, with the same (wrong) result.

[Michi (Michael Henn)]

Furthermore, we are seeing this error in our SonarQube run:

ERROR: Unable to parse source file : ‘some/path/to/some/file.java’
ERROR: Parse error at line YYY column XX: A switch expression should have a default case

with the following configuration:

INFO: SonarScanner 4.7.0.2747
INFO: Java 17.0.2 Eclipse Adoptium (64-bit)
INFO: Windows 7 6.1 amd64
INFO: SONAR_SCANNER_OPTS=-Xmx4096m
…
INFO: Analyzing on SonarQube server 9.5.0.56709

It looks as if SonarScanner even stops scanning. Shouldn’t this not just lead to a rule violation instead of the Scanner actually stopping the scan?
Not having a default case shouldn’t cause an error, I guess?

Is this the same topic, or should I start a new thread for this?

[Leonardo_Pilastri]

Thank you Michael for pointing out a scenario that was not considered by the rule when analyzing switch statements, it’s a nice improvement to implement!
I opened a ticket about this, and we will fix it as soon as possible