Implement optimal parenthesis for ternary operator + add tests #289
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,9 +226,50 @@ class TranslatorSpec extends AnyFunSpec { | |
| } | ||
|
|
||
| describe("ternary operator") { | ||
| everybodyExcept("2 < 3 ? 123 : 456", "2 < 3 ? 123 : 456", ResultMap( | ||
| GoCompiler -> | ||
| """var tmp1 int8; | ||
| |if (2 < 3) { | ||
| | tmp1 = 123 | ||
| |} else { | ||
| | tmp1 = 456 | ||
| |} | ||
| |tmp1""".stripMargin, | ||
| LuaCompiler -> "utils.box_unwrap((2 < 3) and utils.box_wrap(123) or (456))", | ||
| PythonCompiler -> "123 if 2 < 3 else 456", | ||
| )) | ||
|
|
||
| // When evaluated, must be 12 + 34 = 46 | ||
| everybodyExcept("2 < 3 ? 12 + 34 : 45 + 67", "2 < 3 ? 12 + 34 : 45 + 67", ResultMap( | ||
|
There was a problem hiding this comment. I think, it is better to explicitly test all possible expressions in arms, not only There was a problem hiding this comment. Not sure if I follow what you're suggesting? What extra value it will bring or rather what type of situation you anticipate to catch with that? Also, what limit on "all possible expressions" you want to put in here? "All possible" literally is infinite set, we won't be able to test infinite permutations. There was a problem hiding this comment.
Different operators have different priority, so in some cases we need to wrap them in parenthesis. I cannot be sure that in all languages it is handled correctly until it is not tested. For example, I'm working on bringing Rust tests to TranslatorSpec and found a bug where
I don't think so. KS expression language has a limited set of operators, just test them all here. There was a problem hiding this comment. If I understood your correctly, what you're suggesting is effectively testing all permutations of all pairs:
I don't think it's a great strategy to move forward with, because of two reasons:
I don't think it's practical approach to follow. Instead, what I propose is to be aware of the fact that in all languages we track this operator is lower precedence than anything and test only corner cases. There was a problem hiding this comment. No, just There was a problem hiding this comment.
My 2 cents: I don't think it makes sense to go super crazy with these manual In my view, these tests should only be complementary to normal .ksy + .kst tests, not replace them (that would be a step back). They can provide some value that .ksy/.kst tests can't - they can be used to catch that we have unnecessarily too many parentheses, which makes the generated code ugly, but doesn't affect functionality. But IMHO they shouldn't be meant to catch too few parentheses - .ksy/.kst tests are much better suited for this, because then there's always a set of values for which the actual evaluation of the translated expression in the target language will result in a different value than expected, which proves that we really have a problem, and we'll instantly know in what target languages exactly. I wrote a very immature proof-of-concept Python script that demonstrates what automatic generation of such .ksy/.kst pair could look like: https://gist.github.com/generalmimon/a24d65d1890e73b4036fdd48e7d6772f To give you an idea, the generated files look something like this: # expr_gen.ksy
meta:
id: expr_gen
enums:
fruit:
9: apple
6: banana
instances:
l0_r0_lsig0_rsig0:
value: (0 + 7) + 5
l0_r0_lsig0_rsig1:
value: (0 + 7) + 13.7
l0_r0_lsig1_rsig2:
value: (3 + 4.4) + 13
l0_r0_lsig1_rsig3:
value: (3 + 4.4) + 1.1
# ...
l0_r4_lsig0_rsig0:
value: (3 + 12) % 6
l0_r5_lsig0_rsig0:
value: (11 + 13) | 8
l0_r6_lsig0_rsig0:
value: (7 + 13) ^ 14
l0_r7_lsig0_rsig0:
value: (6 + 1) & 12
# ...
l11_r0_lsig3_rsig0:
value: fruit::banana.to_i + 7
l11_r0_lsig3_rsig1:
value: fruit::banana.to_i + 5.3
l11_r0_lsig4_rsig0:
value: false.to_i + 1
l11_r0_lsig4_rsig1:
value: false.to_i + 11.1
# ...
l20_r10_lsig5_rsig1:
value: '(true ? [50, 48, 52] : [56, 57, 51, 48]).to_s(''ASCII'')'
# ...# expr_gen.kst
id: expr_gen
data: enum_negative.bin
asserts:
- actual: l0_r0_lsig0_rsig0
expected: '12'
- actual: l0_r0_lsig0_rsig1
expected: '20.7'
- actual: l0_r0_lsig1_rsig2
expected: '20.4'
- actual: l0_r0_lsig1_rsig3
expected: '8.5'
# ...
- actual: l0_r4_lsig0_rsig0
expected: '3'
- actual: l0_r5_lsig0_rsig0
expected: '24'
- actual: l0_r6_lsig0_rsig0
expected: '26'
- actual: l0_r7_lsig0_rsig0
expected: '4'
# ...
- actual: l11_r0_lsig3_rsig0
expected: '13'
- actual: l11_r0_lsig3_rsig1
expected: '11.3'
- actual: l11_r0_lsig4_rsig0
expected: '1'
- actual: l11_r0_lsig4_rsig1
expected: '11.1'
# ...
- actual: l20_r10_lsig5_rsig1
expected: '''204'''
# ...It's not a silver bullet (and the script needs some serious refactoring, comparison operators need to be added, the right-side grouping like And I've already found some bugs with this that I shared in #277 (comment). There was a problem hiding this comment. The problem with KST is that they does not run in PRs and I do not know how to run them locally. So it is hard to change code which potentially can break some other languages when I'm working on a language that's interest for me. I do not know many Kaitai languages and I wouldn't be happy that I should blindly change their or common code. If you could write a documentation how to do that on Windows (and even better also on Windows 7) and setup corresponding checks on CI, it would be great simplification! There was a problem hiding this comment.
Happy to help you with that (I'm on Windows too), but not here - are you on Gitter? We can talk privately/publicly as you wish.
Both things would be nice, but as with everything, the problem is that someone has to do it. There was a problem hiding this comment. @generalmimon Love the idea, that's actually a very good separation line that you've drawn here! Would you mind creating a PR with that script to tests repo? |
||
| GoCompiler -> | ||
| """var tmp1 int; | ||
| |if (2 < 3) { | ||
| | tmp1 = 12 + 34 | ||
| |} else { | ||
| | tmp1 = 45 + 67 | ||
| |} | ||
| |tmp1""".stripMargin, | ||
| LuaCompiler -> "utils.box_unwrap((2 < 3) and utils.box_wrap(12 + 34) or (45 + 67))", | ||
| PythonCompiler -> "12 + 34 if 2 < 3 else 45 + 67", | ||
| )) | ||
|
|
||
| // When evaluated, must be (12 + 34) + 67 = 113 | ||
| everybodyExcept("(2 < 3 ? 12 + 34 : 45) + 67", "(2 < 3 ? 12 + 34 : 45) + 67", ResultMap( | ||
| GoCompiler -> | ||
| """var tmp1 int; | ||
| |if (2 < 3) { | ||
| | tmp1 = 12 + 34 | ||
| |} else { | ||
| | tmp1 = 45 | ||
| |} | ||
| |tmp1 + 67""".stripMargin, | ||
| LuaCompiler -> "utils.box_unwrap((2 < 3) and utils.box_wrap(12 + 34) or (45)) + 67", | ||
| PythonCompiler -> "(12 + 34 if 2 < 3 else 45) + 67", | ||
| )) | ||
|
|
||
| full("2 < 3 ? \"foo\" : \"bar\"", CalcIntType, CalcStrType, ResultMap( | ||
| CppCompiler -> "2 < 3 ? std::string(\"foo\") : std::string(\"bar\")", | ||
| CSharpCompiler -> "2 < 3 ? \"foo\" : \"bar\"", | ||
| GoCompiler -> | ||
| """var tmp1 string; | ||
| |if (2 < 3) { | ||
|
|
@@ -237,13 +278,53 @@ class TranslatorSpec extends AnyFunSpec { | |
| | tmp1 = "bar" | ||
| |} | ||
| |tmp1""".stripMargin, | ||
| JavaCompiler -> "2 < 3 ? \"foo\" : \"bar\"", | ||
| JavaScriptCompiler -> "2 < 3 ? \"foo\" : \"bar\"", | ||
| LuaCompiler -> "utils.box_unwrap((2 < 3) and utils.box_wrap(\"foo\") or (\"bar\"))", | ||
| PerlCompiler -> "2 < 3 ? \"foo\" : \"bar\"", | ||
| PHPCompiler -> "2 < 3 ? \"foo\" : \"bar\"", | ||
| PythonCompiler -> "u\"foo\" if 2 < 3 else u\"bar\"", | ||
| RubyCompiler -> "2 < 3 ? \"foo\" : \"bar\"" | ||
| )) | ||
|
|
||
| full("true ? foo._io : bar._io", KaitaiStructType, KaitaiStreamType, ResultMap( | ||
| CppCompiler -> "true ? foo()->_io() : bar()->_io()", | ||
| CSharpCompiler -> "true ? Foo.M_Io : Bar.M_Io", | ||
| GoCompiler -> | ||
| """var tmp1 *kaitai.Stream; | ||
| |if (true) { | ||
| | tmp1 = this.Foo._io | ||
| |} else { | ||
| | tmp1 = this.Bar._io | ||
| |} | ||
| |tmp1""".stripMargin, | ||
| JavaCompiler -> "true ? foo()._io() : bar()._io()", | ||
| JavaScriptCompiler -> "true ? this.foo._io : this.bar._io", | ||
| LuaCompiler -> "utils.box_unwrap((true) and utils.box_wrap(self.foo._io) or (self.bar._io))", | ||
| PerlCompiler -> "1 ? $self->foo()->_io() : $self->bar()->_io()", | ||
| PHPCompiler -> "true ? $this->foo()->_io() : $this->bar()->_io()", | ||
| PythonCompiler -> "self.foo._io if True else self.bar._io", | ||
| RubyCompiler -> "true ? foo._io : bar._io", | ||
| )) | ||
|
|
||
| full("(true ? foo : bar)._io", KaitaiStructType, KaitaiStreamType, ResultMap( | ||
| CppCompiler -> "(true ? foo() : bar())->_io()", | ||
| CSharpCompiler -> "(true ? Foo : Bar).M_Io", | ||
| GoCompiler -> | ||
| """var tmp1 interface{}; | ||
| |if (true) { | ||
| | tmp1 = this.Foo | ||
| |} else { | ||
| | tmp1 = this.Bar | ||
| |} | ||
| |tmp1._io""".stripMargin, | ||
|
Comment on lines
+313
to
+320
There was a problem hiding this comment. This too can hardly be approved as correct. Accessing
The code snippet on the linked page also demonstrates that func main() {
var i interface{}
describe(i) // => (<nil>, <nil>)
i = 42
describe(i) // => (42, int)
i = "hello"
describe(i) // => (hello, string)
}
func describe(i interface{}) {
fmt.Printf("(%v, %T)\n", i, i)
}To implement this correctly in Go, There was a problem hiding this comment. Thanks for catching this too, I was too naive to suppose it will just work. Looks like we indeed just don't have that functionality at all. As far as I can tell, there's no inheritance in Go, thus we can't have common ancestor KaitaiStruct which will implement storage of a stream, but we indeed can have a common interface promising that every KaitaiStruct type we define has to implement a method that will return what we can use for Let me actually give it a try separately. The hardest thing here, as you've pointed out, is to decide on a naming. I really don't like But, generally speaking, given that our Golang name rendition is UpperCamelCase, which guarantees no underscores, I believe putting in the underscore is a way to go. I wonder what do you folks think about There was a problem hiding this comment. As far as I'm concerned, both More interesting to me is another naming problem :), which is whether to pick
This makes me die inside a bit, but as they say "when in Rome, do as the Romans do". So There was a problem hiding this comment. The interesting bit here is that we actually already generate some "public" names for these as per this code. This generates Personally, I don't have a strong preference — very generally, sticking to prescribed recommendations per language makes sense, especially if they exist and are clear. |
||
| JavaCompiler -> "(true ? foo() : bar())._io()", | ||
| JavaScriptCompiler -> "(true ? this.foo : this.bar)._io", | ||
| LuaCompiler -> "utils.box_unwrap((true) and utils.box_wrap(self.foo) or (self.bar))._io", | ||
| PerlCompiler -> "(1 ? $self->foo() : $self->bar())->_io()", | ||
| PHPCompiler -> "(true ? $this->foo() : $this->bar())->_io()", | ||
| PythonCompiler -> "(self.foo if True else self.bar)._io", | ||
| RubyCompiler -> "(true ? foo : bar)._io", | ||
| )) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,7 @@ class NimTranslator(provider: TypeProvider, importList: ImportList) extends Base | |
| case Identifier.ROOT => s"${ksToNim(provider.determineType(Identifier.ROOT))}(this.${doName(s)})" | ||
| case _ => s"this.${doName(s)}" | ||
| } | ||
| override def doIfExp(condition: expr, ifTrue: expr, ifFalse: expr, extPrec: Int): String = | ||
| s"(if ${translate(condition)}: ${translate(ifTrue)} else: ${translate(ifFalse)})" | ||
|
There was a problem hiding this comment. Need to rewrite that part too. You can check result at https://play.nim-lang.org |
||
| override def arraySubscript(container: expr, idx: expr): String = | ||
| s"${translate(container)}[${translate(idx)}]" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,7 +47,7 @@ class RustTranslator(provider: TypeProvider, config: RuntimeConfig) extends Base | |
|
|
||
| override def arraySubscript(container: expr, idx: expr): String = | ||
| s"${translate(container)}[${translate(idx)}]" | ||
| override def doIfExp(condition: expr, ifTrue: expr, ifFalse: expr, extPrec: Int): String = | ||
| "if " + translate(condition) + | ||
| " { " + translate(ifTrue) + " } else { " + | ||
| translate(ifFalse) + "}" | ||
|
Comment on lines
51
to
53
There was a problem hiding this comment. Need to rewrite this part too. You can check result at https://play.rust-lang.org |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
456clearly doesn't fit intoint8, so I wouldn't approve this as the "expected" correct translation just to make the tests green and provide a false sense of "everything works fine" when it doesn't.This doesn't even compile, see https://go.dev/play/p/tZ81LHWP5bM:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually perfect catch, thanks for noticing it!
I don't think there's anything wrong with the expression, but actually we should fix GoTranslator implementation, so it should use more generic CalcIntType.