kaitai_struct icon indicating copy to clipboard operation
kaitai_struct copied to clipboard

Java: invalid generation of switch with expressions in `case` clause

Open Mingun opened this issue 2 years ago • 3 comments

The following KSY generates invalid Java code:

meta:
  id: java_switch_bug
seq:
  - id: a
    type: u1
  - id: if
    type:
      switch-on: 0
      cases:
        a: u1
        _: u8be

Generator always uses switch statement for the switch-on expression which is incorrect in case of non-constant cases. Unlike JavaScript Java only supports constant case labels (only int before Java 7, int and String from Java 7).

class Test {
  final int a = 0;
  final long b = 0L;
  int c = 0;

  final String d = "0";
  String e = "0";

  void test() {
    switch (...) {
      case 0:      // OK
      case a:      // OK
      case this.a: // error: constant expression required
      case b:      // error: incompatible types: possible lossy conversion from long to int
      case c:      // error: constant expression required

      case "0":    // OK from Java 7
      case d:      // OK from Java 7
      case this.d: // error: constant expression required
      case e:      // error: constant expression required
    }
  }
}

Mingun avatar Nov 20 '21 14:11 Mingun

Related issue: https://github.com/kaitai-io/kaitai_struct/issues/599

Mingun avatar Dec 12 '21 09:12 Mingun

What I'd suggest is a patch like this one:

--- a/shared/src/main/scala/io/kaitai/struct/languages/JavaCompiler.scala	(revision 44eaeda56ef6ab30b922b3887299ef05414a28ea)
+++ b/shared/src/main/scala/io/kaitai/struct/languages/JavaCompiler.scala	(date 1655822973672)
@@ -546,7 +546,7 @@
   }
 
   override def switchRequiresIfs(onType: DataType): Boolean = onType match {
-    case _: IntType | _: EnumType | _: StrType => false
+    //case _: IntType | _: EnumType | _: StrType => false
     case _ => true
   }

In other words, for now back out the optimization to use switch. Always generate if ladders. Yes, switch can be faster, but there are too many cases where the choice is made to use a switch but it doesn't work. Alternatively, instead of assuming switch by default and then special casing the ones that require ifs, why not default to an if ladder and detect the cases that are known to work with switch?

mikehearn avatar Jun 21 '22 15:06 mikehearn

This PR implements a more precise check for when switch statements can be used

https://github.com/kaitai-io/kaitai_struct_compiler/pull/247

mikehearn avatar Jun 21 '22 16:06 mikehearn