kaitai_struct_compiler
kaitai_struct_compiler copied to clipboard
KS expression language: optimize parenthesis generation
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 numericBinOpandstrConcatuse and respect this, basing their decision onOPERATOR_PRECEDENCEmap- Postfix method invocations (
foo.method()) can use nowtranslateonfoowithMETHOD_PRECEDENCEwhich will ensure it's properly guarded against stuff like.method invocation if necessary.
- AbstractTranslator/BaseTranslator
- Adjust all the per-language
*Translatorimplementations to use precedence tracking.- Removed forced parenthesis everywhere they were used.
- Invoke
translateor similar for underlying parts in a smart, precedence-aware way.
Out of scope of this PR:
- Left-associativity optimizations (e.g.
a + b + cnow 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}