kaitai_struct_compiler icon indicating copy to clipboard operation
kaitai_struct_compiler copied to clipboard

KS expression language: optimize parenthesis generation

Open GreyCat opened this issue 1 year ago • 5 comments

Big, scary looking PR, but in reality it's rather straightforward which mostly addresses https://github.com/kaitai-io/kaitai_struct/issues/69 in 3 steps:

  • TranslatorSpec: adjust existing tests which were ok with too many parenthesis to have optimal output, add some more tests.
    • Many of these are inspired/already available in tests repo, but I think it's still good to have them contained here for unit testing.
  • Implement precedence tracking:
    • AbstractTranslator/BaseTranslator translate(...) now has optional precedence argument which influences whether to guard result with parenthesis or not
    • numericBinOp and strConcat use and respect this, basing their decision on OPERATOR_PRECEDENCE map
    • Postfix method invocations (foo.method()) can use now translate on foo with METHOD_PRECEDENCE which will ensure it's properly guarded against stuff like . method invocation if necessary.
  • Adjust all the per-language *Translator implementations to use precedence tracking.
    • Removed forced parenthesis everywhere they were used.
    • Invoke translate or similar for underlying parts in a smart, precedence-aware way.

Out of scope of this PR:

  • Left-associativity optimizations (e.g. a + b + c now gets rendered as (a + b) + c, because we have no idea that + is left associative). I'll figure out how to fix this later. Relevant tests are "silenced" and pass now, but in future we'll revert https://github.com/kaitai-io/kaitai_struct_compiler/commit/a2b3bef4eeaea1e64f66f0887b2f25fac849df18 and address it.
  • Unary operators
  • Comparison operators
  • More testing for low precedence operators (e.g. BitAnd, BitOr, BitXor)

As I've mentioned, it doesn't solve 100% of all the problems, but it makes things much better, so I believe it's a good intermediate step. End result is expected to be like that:

Full test report

cpp_stl/_98/gcc4.8

🟢 1 tests fixed:

  • ExprOpsParens: {"status"=>"format_build_failed", "elapsed"=>0.0, "is_kst"=>true} -> {"status"=>"passed", "elapsed"=>34.0, "is_kst"=>true}

go//1.21

➕ 1 tests added:

  • EnumToIInvalid -> {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true}

java//temurin17

🟢 1 tests fixed:

  • ExprOpsParens: {"status"=>"format_build_failed", "elapsed"=>0, "is_kst"=>true} -> {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true}

javascript//nodejs12

🟢 1 tests fixed:

  • ExprOpsParens: {"status"=>"failed", "elapsed"=>0.0, "failure"=>{"file_name"=>nil, "line_num"=>nil, "message"=>nil, "trace"=>"Expected values to be strictly equal:\n\n'012345' !== 10\n\nAssertionError [ERR_ASSERTION]: Expected values to be strictly equal:\n\n'012345' !== 10\n\n at /tests/spec/javascript/test_expr_ops_parens.js:10:10\n at /tests/helpers/javascript/testHelper.js:16:11\n at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)"}, "is_kst"=>true} -> {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true}

🔴 2 tests broken:

  • BcdUserTypeBe: {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true} -> {"status"=>"failed", "elapsed"=>0.0, "failure"=>{"file_name"=>nil, "line_num"=>nil, "message"=>nil, "trace"=>"Expected values to be strictly equal:\n+ actual - expected\n\n+ 22446688\n- 12345678\n\n + expected - actual\n\n -22446688\n +12345678\n \nAssertionError [ERR_ASSERTION]: Expected values to be strictly equal:\n+ actual - expected\n\n+ 22446688\n- 12345678\n at /tests/spec/javascript/test_bcd_user_type_be.js:7:10\n at /tests/helpers/javascript/testHelper.js:16:11\n at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)"}, "is_kst"=>true}
  • BcdUserTypeLe: {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true} -> {"status"=>"failed", "elapsed"=>0.0, "failure"=>{"file_name"=>nil, "line_num"=>nil, "message"=>nil, "trace"=>"Expected values to be strictly equal:\n+ actual - expected\n\n+ 22446688\n- 12345678\n\n + expected - actual\n\n -22446688\n +12345678\n \nAssertionError [ERR_ASSERTION]: Expected values to be strictly equal:\n+ actual - expected\n\n+ 22446688\n- 12345678\n at /tests/spec/javascript/test_bcd_user_type_le.js:7:10\n at /tests/helpers/javascript/testHelper.js:16:11\n at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)"}, "is_kst"=>true}

lua//5.3

(no changes)

perl//5.38

(no changes)

python//3.11

➕ 1 tests added:

  • ExprFstring0 -> {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true}

ruby//3.3

🟢 1 tests fixed:

  • ExprOpsParens: {"status"=>"failed", "elapsed"=>0.0, "failure"=>{"file_name"=>"./spec/ruby/expr_ops_parens_spec.rb", "line_num"=>4, "message"=>"no implicit conversion of Integer into String", "trace"=>"./compiled/ruby/expr_ops_parens.rb:75:in `+'\n./compiled/ruby/expr_ops_parens.rb:75:in `str_concat_len'\n./spec/ruby/expr_ops_parens_spec.rb:10:in `block (2 levels) in <top (required)>'"}, "is_kst"=>true} -> {"status"=>"passed", "elapsed"=>0.0, "is_kst"=>true}

GreyCat avatar Mar 13 '24 19:03 GreyCat