eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
164 stars 130 forks source link

[Switch][Sealed types] Bad static analysis with the old switch syntax + an exhaustive pattern matching on a sealed type throws a MatchException #3096

Closed forax closed 3 weeks ago

forax commented 3 weeks ago

Hello, One of my TA has found the following bug, if a switch is exhautive, i.e. the cases cover all subtypes of the sealed interface, as the JLS says the compiler adds a default -> throw new MatchException(); and a break is added in the last case if necessary.

Here Eclipse fails to properly analyze the if (bs == null) so it does not insert the break after the last case so at runtime, the case B fallthrough and throws a MatchException.

public class EclipseBugFallThroughSwitch {
  sealed interface I permits A, B {}
  record A(String s) implements I {}
  record B(String s) implements I {}

  public void add(I i) {
    switch (i) {
    case A a:
      break;
    case B b:
      if (b.s == null) {
        throw new NullPointerException();
      }
      //break;  // this fix the issue
    }
  }

  public static void main(String[] args) {
    var container = new EclipseBugFallThroughSwitch();
    container.add(new B("bar"));

    // Exception in thread "main" java.lang.MatchException
    // at EclipseBugFallThroughSwitch.add(EclipseBugFallThroughSwitch.java:9)
    // at EclipseBugFallThroughSwitch.main(EclipseBugFallThroughSwitch.java:25)
  }
}
srikanth-sankaran commented 3 weeks ago

Thanks for the report Rémi. Reproduced.

Smaller test case:

public sealed interface X permits X.R {
  record R(String s) implements X {}

  public static void add(X x) {
    switch (x) {
    case R r:
      if (r.s == null) {
        throw new NullPointerException();
      }
    }
  }

  public static void main(String[] args) {
    add(new R("bar"));
  }
}
srikanth-sankaran commented 3 weeks ago

Problem dates back to 4.30

srikanth-sankaran commented 3 weeks ago

Same underlying problem as https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2513 - the proposed fix there was using too much local context. That deficiency should be addressed by the current PR https://github.com/eclipse-jdt/eclipse.jdt.core/pull/3098

srikanth-sankaran commented 3 weeks ago

To check and to address separately if needed: whether JDK18- code falls into the compiler inserted default that throws an IncompatibleClassChangeError

srikanth-sankaran commented 3 weeks ago

To check and to address separately if needed: whether JDK18- code falls into the compiler inserted default that throws an IncompatibleClassChangeError

Yes, this scenario is correctly handled. I have added more tests here: https://github.com/eclipse-jdt/eclipse.jdt.core/pull/3099