kaitai_struct_compiler icon indicating copy to clipboard operation
kaitai_struct_compiler copied to clipboard

Fix writing enums in repetitions

Open generalmimon opened this issue 6 years ago • 1 comments

When you want to write enum value, you need to call method translator.enumToInt, which expects its arguments of type Ast.expr.

So we need a way to convert Identifier to Ast.expr.Name. I thought that method Identifier.toAstIdentifier would serve this purpose, but it doesn't work and I don't know how it was supposed to work. Anyway, the conversion had been done as Ast.expr.Name(Ast.identifier(idToStr(id))). However, this simple conversion is wrong. The Identifier is language-agnostic object, but idToStr(id) converts it to language-dependent name.

So when you try to call

translator.translate(
  Ast.expr.Subscript(
    Ast.expr.Name(Ast.identifier(idToStr(id))),
    Ast.expr.Name(Ast.identifier(Identifier.INDEX))
  )
)

with id = NamedIdentifier('some_array'), idToStr(id) will return 'someArray' for Java, and the execution will fail when detecting type of the container you try to subscript, because there is no field with name 'someArray' (actualy, the ClassTypeProvider.determineType method internally creates NamedIdentifier(attrName), so it fails on constraint ^[a-z][a-z0-9_]*$).

Hence I felt the need to inject language-specific code to AST object, so I created class Ast.expr.CodeLiteral(code) for this purpose. The BaseTranslator for it just returns the code itself. In the future, we may need also specify the DataType of the code literal, so that detecting type on it doesn't fail, but I haven't needed it yet.

generalmimon avatar Nov 17 '19 08:11 generalmimon

@GreyCat Have you looked at the code? What's the problem? You hinted in https://github.com/kaitai-io/kaitai_struct_compiler/pull/192#discussion_r368288211 that you're not a fan of the idea of CodeLiteral. I also don't like it much, but my defense is that it just works.

Actually, the only way I can think of how to do it better is to rewrite the methods in *Translator classes to accept Strings instead of Ast.exprs. I think it makes sense, because we can reuse code in the *Translator classes this way, without having to repeat the code of the methods in the compiler classes on places where we are working with strings already, not with expression trees.

Some examples:

https://github.com/kaitai-io/kaitai_struct_compiler/blob/dc83f27d3cb21275c285d34d35e9c3b8ac446dec/shared/src/main/scala/io/kaitai/struct/languages/JavaCompiler.scala#L478-L482

https://github.com/kaitai-io/kaitai_struct_compiler/blob/dc83f27d3cb21275c285d34d35e9c3b8ac446dec/shared/src/main/scala/io/kaitai/struct/languages/JavaCompiler.scala#L497-L502

Another reason why I think it makes sense is that the vast majority of translators actually build Strings anyway, so you see a lot of s"${translate(v)}..." stuff here (a quick search for translate( in JavaTranslator.scala shows 48 occurrences). This may seem normal, but it looks to me like the methods are virtually finding their inputs themselves, instead of accepting them as parameter.

It's similar to write this

  public static int add() {
    Scanner in = new Scanner(System.in);
    System.out.print("Enter number A: ");
    int a = in.nextInt();
    System.out.print("Enter number B: ");
    int b = in.nextInt();
    in.close();
    return a + b;
  }

instead of

  public static int add(int a, int b) {
    return a + b;
  }

Of course, this is an exaggerated example, because the methods in the translators are just transforming the input arguments, not getting them from nothing, but you hopefully get the idea.

Yet another advantage of moving the translate() calls out of all the methods of the translators except the translate method itself. I personally think it's better when the recursion occurs just in one method (only the translate method would call itself multiple times recursively, the methods that it calls wouldn't contain any translate calls), there's no need for mutual recursion that happens now.

Either way, we really should find some neat way to solve this situation. Of course, we could just duplicate the code of the methods we need to work with strings and adjust them so, but I suggest to take a step forward and eliminate the methods accepting Ast.exprs completely. What do you think?

generalmimon avatar Jun 26 '20 19:06 generalmimon

Problem solved in https://github.com/kaitai-io/kaitai_struct_compiler/commit/939cf98d495b941920753dad379a65346a1d8f2e using Ast.expr.InternalName

generalmimon avatar Aug 28 '22 22:08 generalmimon