kaitai_struct_compiler icon indicating copy to clipboard operation
kaitai_struct_compiler copied to clipboard

fix exception when using json output flag

Open smx-smx opened this issue 1 year ago • 2 comments

fixes the following exception, which breaks usage with ksdump:

Exception in thread "main" scala.MatchError: ArrayBuffer(...)

smx-smx avatar Mar 16 '24 23:03 smx-smx

This is really funny to me, because I fixed the exact same error before releasing 0.10 😄: https://github.com/kaitai-io/kaitai_struct_compiler/commit/74759af6b58781a9171aae54409bfd34296ed827

But you're right that it's now back with the latest compiler. I guess that in Scala 2.13 that the compiler got migrated to in https://github.com/kaitai-io/kaitai_struct_compiler/pull/266 (cc @GreyCat), ArrayBuffer no longer inherits from Seq like it did in Scala 2.12.

generalmimon avatar Mar 17 '24 00:03 generalmimon

I guess that in Scala 2.13 (...), ArrayBuffer no longer inherits from Seq like it did in Scala 2.12.

Just as I thought - see Migrating a Project to Scala 2.13's Collections | Scala Documentation:

The most important changes in the Scala 2.13 collections library are:

  • scala.Seq[+A] is now an alias for scala.collection.immutable.Seq[A] (instead of scala.collection.Seq[A]). Note that this also changes the type of Scala varargs methods.

Since mutable.ArrayBuffer is obviously not immutable, it's no longer a subclass of Seq.


So I guess the most straightforward solution would be Option 1: migrate back to scala.collection.Seq, which should restore the old behavior. The linked migration guide goes into detail how this should be done:

We recommend using import scala.collection/import scala.collection.immutable and collection.Seq/immutable.Seq.

We recommend against using import scala.collection.Seq, which shadows the automatically imported scala.Seq, because even if it’s a one-line change it causes name confusion. For code generation or macros the safest option is using the fully-qualified _root_.scala.collection.Seq.

In our case, that would be:

diff --git i/shared/src/main/scala/io/kaitai/struct/JSON.scala w/shared/src/main/scala/io/kaitai/struct/JSON.scala
index d38a0634..b86b3a25 100644
--- i/shared/src/main/scala/io/kaitai/struct/JSON.scala
+++ w/shared/src/main/scala/io/kaitai/struct/JSON.scala
@@ -1,6 +1,7 @@
 package io.kaitai.struct

 import io.kaitai.struct.translators.CommonLiterals
+import scala.collection

 /** Common trait for all objects that can be serialized as JSON. */
 trait Jsonable {
@@ -22,7 +23,7 @@ object JSON extends CommonLiterals {
       case v: Jsonable => v.toJson
       case v: Int => v.toString
       case v: String => stringToJson(v)
-      case v: Seq[_] => seqToJson(v)
+      case v: collection.Seq[_] => seqToJson(v)
       case v: Map[String, _] => mapToJson(v)
     }
   }
@@ -33,7 +34,7 @@ object JSON extends CommonLiterals {
   def stringToJson(str: String): String =
     doStringLiteral(str)

-  def seqToJson(obj: Seq[_]): String =
+  def seqToJson(obj: collection.Seq[_]): String =
     "[" + obj.map((x) => stringify(x)).mkString(",") + "]"

   def mapToJson(obj: Map[String, Any]): String = {

generalmimon avatar Mar 17 '24 00:03 generalmimon

Thanks for your research and suggestions, @generalmimon and @smx-smx!

I believe generally approach suggested by @generalmimon makes perfect sense. My only suggestion would be actually that we need to have a test somehow checking that JSON output works at all.

GreyCat avatar Mar 22 '24 20:03 GreyCat

My only suggestion would be actually that we need to have a test somehow checking that JSON output works at all.

Totally agree. Do you mean to add some unit tests to jvm/src/test/scala/io/kaitai/struct (https://github.com/kaitai-io/kaitai_struct_compiler/tree/master/jvm/src/test/scala/io/kaitai/struct)?

generalmimon avatar Mar 22 '24 20:03 generalmimon

Totally agree. Do you mean to add some unit tests to jvm/src/test/scala/io/kaitai/struct (https://github.com/kaitai-io/kaitai_struct_compiler/tree/master/jvm/src/test/scala/io/kaitai/struct)?

Yep, something like that. Maybe literally running compiler end-to-end with various options just to make sure we allow various permutations of inputs and outputs.

GreyCat avatar Mar 22 '24 22:03 GreyCat

Maybe literally running compiler end-to-end with various options just to make sure we allow various permutations of inputs and outputs.

Yeah, I was thinking about that as well, but I'm not sure how to integrate that into our infrastructure (maybe a new repo?). Right now, we're basically only testing one specific end-to-end invocation in build-formats, which is far from covering all possible settings.

generalmimon avatar Mar 22 '24 22:03 generalmimon

We don't really have to test all possible permutations of command line arguments × all possible .ksy files. This specific problem could be identified and tested by a simple unit test: one set of compilation problems in, one JSON output formatting expected.

GreyCat avatar Mar 23 '24 12:03 GreyCat