Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust tests #96

Merged
merged 211 commits into from
Sep 7, 2024
Merged

Rust tests #96

merged 211 commits into from
Sep 7, 2024

Conversation

Agile86
Copy link
Contributor

@Agile86 Agile86 commented Mar 15, 2023

Hello everybody,

my name is Oleh Dolhov, my collegue name is Vitaly Reshetyuk,

we are from Stroz Friedberg (AON) providing this PR to bring Rust-language support to Kaitai.

Right now we can pass almost all tests, except (4) tests, that's using features like:

  1. parent rewrite

Rust doesn't support inheritance, so there is no common type.

  1. KStructUnit, tests like nav_parent_switch_cast.ksy, params_pass_array_struct.ksy, params_pass_struct.ksy.

  2. AnyType, test like process_coerce_switch.ksy.

Please, note, you need to clone compiler and runtime/rust exactly rust_basic_support_v2 branch.

Best regards,
Oleh & Vitaly.

@generalmimon
Copy link
Member

generalmimon commented Jun 25, 2024

@Agile86 @revitalyr I thank you for your work, but I will now take over and kindly ask you not to proceed with any changes. I have no doubt that you have done a lot of work (i.e. the 'quantity' is large), but across all your changes I come across a lot of nonsensical changes. You often only address the consequences of some problems in a completely wrong (logically flawed) way without looking for the cause of the problem and understanding it.

I will do my best to fix as many sins in your code as possible, but unfortunately I will not be able to fix all of them now. I don't have time to explain everything I don't like about your code, but I don't want you to fix anything anyway (I don't feel there's much chance I'd be happy with your fixes). Just by deciding to eventually accept your code, we are incurring a lot of technical debt going forward, which I personally don't like at all, but there is no other way. Hopefully I'll have time for some major rework in the future.

I would greatly appreciate it if you could take this statement professionally and not take offense. It's not personal, I don't know you, I'm just evaluating the code. But I think you should understand this assessment, because you yourself must be aware that your changes are often not well thought out.

See `builder/spec/rust/rust_builder_spec.rb` for more details.

This test currently fails:

```console
$ rspec builder/spec/rust/rust_builder_spec.rb
#### RustBuilder: initialized
F

Failures:

  1) RustBuilder macro_expansion #parse_failed_build parses failed build information
     Failure/Error:
       expect(@builder.parse_failed_build('test_out/rust/build-1.log')).to eq [
         test_nested_path,
         test_nested_path,
       ]

       expected: ["/mnt/c/temp/kaitai_struct/tests/builder/spec/rust/macro_expansion/spec/rust/tests/test_nested.rs",
                  "/mnt/c/temp/kaitai_struct/tests/builder/spec/rust/macro_expansion/spec/rust/tests/test_nested.rs"]
            got: ["/rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/macros/mod.rs",
                  "/rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/macros/mod.rs"]

       (compared using ==)
     # ./builder/spec/rust/rust_builder_spec.rb:70:in `block (4 levels) in <top (required)>'

Finished in 0.03102 seconds (files took 0.14634 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./builder/spec/rust/rust_builder_spec.rb:68 # RustBuilder macro_expansion #parse_failed_build parses failed build information
```

It will be fixed in the following commit by implementing the missing
logic in `builder/rust_builder.rb`.
This change fixes the test added in the previous commit:

```console
$ rspec builder/spec/rust/rust_builder_spec.rb
#### RustBuilder: initialized
.

Finished in 0.023 seconds (files took 0.49077 seconds to load)
1 example, 0 failures
```
This breaks the YamlInts test, which is correct because it should be
failing. All of the checked value instances (`test_{u4,u8}_{dec,hex}`)
have type `i32` in the `compiled/rust/yaml_ints.rs` file generated by
KSC, but that's wrong because none of the integer constants that these
value instances should contain according to `formats/yaml_ints.ksy` fit
within the `i32` range (which the Rust compiler, i.e. `rustc`, correctly
reports as long as the `overflowing_literals` lint is enabled, which is
the default). So the fact that this test now fails reflects reality.
Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Agile86 @revitalyr Thank you for this PR as well.

I've had quite a bit of work with this one to be happy with it. To some extent that's fine and understandable, because obviously you can't be as familiar with the project as I am, but I still don't understand why you did certain things. For example:

  1. I honestly don't understand the purpose of any of the changes to the KST translator that I reverted in 0e6254d...0ce1363. It looked like you didn't understand how to use the KST translator (in which case you could've asked), or you wanted to use it in a different workflow than what it's designed for, which is fine, but there wasn't any good reason to propose these changes in this PR because it has nothing to do with Rust specifically.

  2. I don't understand why you thought that generating assert_eq!($actStr, 0); for expected: 'null' in a KST spec was OK (RustSG.scala:100 at 905f1f2a):

    @@ -91,11 +84,12 @@ class RustSG(spec: TestSpec, provider: ClassTypeProvider) extends BaseGenerator(
       override def nullAssert(actual: Ast.expr): Unit = {
         val actStr = translateAct(actual)
         finish_panic()
    -    out.puts(s"assertNull($actStr);")
    +    out.puts(s"assert_eq!($actStr, 0);")
         // TODO: Figure out what's meant to happen here
       }

    Did you see in any other language that we would sweep nullable fields under the rug by just storing 0 in them if they have no value? I don't think so. Even the code we generate for C++ where nullable numeric fields are indistinguishable from non-nullable numeric types (unfortunately, std::optional is only available since C++17, while we target C++98 and C++11) provides null flags so that it's at least possible to distinguish 0 and null (although in a bit ugly way). In Rust, we should of course use Option<T> instead.

    I fixed the nullAssert method in 8bdfd0c so that it expects the generated code to actually use the Option<T> type for nullable fields. Currently it doesn't, so the corresponding tests will fail to build, but that's fine. Tests should accurrately represent the current state of what actually works and what doesn't. They should pass only if the features they test fully work, otherwise they should fail. There's no point in crippling the tests to make them "green" even though the features under test don't work properly - we'd just be lying to ourselves that everything is fine when it's not.

  3. Another example of crippling a test just to make it green was in ExprIntDiv - this is what it looked like before I finally regenerated it from the KST spec in 16d44c9c - https://github.com/kaitai-io/kaitai_struct_tests/blob/16d44c9c1177abd72fcfb8ffd9d17e0b86801397~/spec/rust/tests/test_expr_int_div.rs#L24-L27:

        assert_eq!(*r.div_pos_const().expect("error reading"), 756);
        assert_eq!(*r.div_neg_const().expect("error reading"), -756);
        assert_eq!(*r.div_pos_seq().expect("error reading"), 97130679);
        assert_eq!(*r.div_neg_seq().expect("error reading"), -4072);

    Again, did you see -756 and -4072 in any other language? I don't think so. I would think expr_int_div.kst:8-15 is clear enough:

      - actual: div_pos_const
        expected: 756
      - actual: div_neg_const
        expected: -757
      - actual: div_pos_seq
        expected: 97130679
      - actual: div_neg_seq
        expected: -4073

    I'm really missing what the thought process was ("no one will notice and I'll be able to say that my implementation passes more tests?"), but please don't do this. A failing test is better than faked passing one.

  4. Yet another example of crippling a test, this time using #![allow(overflowing_literals)] to make the YamlInts test pass when it absolutely should not pass and the Rust compiler knows this (bf66bbe). This was actually in the compiler as well (Rust support kaitai_struct_compiler#250 (comment)) and even the runtime library had something similar (#![allow(unused)], which suppressed a bunch of warnings that had no good reason to be suppressed, see kaitai-io/kaitai_struct_rust_runtime@c7ce843).

    Come on, suppressing lints you don't like is not the solution.

  5. I'm generally very skeptical about string manipulation on translated expressions (as I also mentioned in Rust support kaitai_struct_compiler#250 (review)). I'm convinced that we should always treat translated expressions as opaque strings - i.e. never attempt to "see what's inside" in any way. The reason is that string operations can't possibly understand the syntax and semantics of the expression inside the string, so it's highly unreliable, see Ruby: enum.to_i implementation is buggy due to string replacement kaitai_struct#1125 for instance. Whenever it's needed to "see what's inside the expression", pattern matching with AST objects (or perhaps data types) should be used.

    So I wasn't happy to see the string replacements that used to be in RustSG.scala in the KST translator. But surprisingly, I tried removing them and found out that they don't actually do anything (b0bd33e#diff-a6bcd12fc56efe3c133df903294fadf9330b65f2374740fa685a8946a45fc0b7). So yeah, I don't understand why they were there, but it doesn't matter now.

  6. The method you used to "expect an exception" didn't really make much sense (test_eof_exception_bytes.rs:18-22):

        if let Err(err) = res {
            println!("expected err: {:?}, exception: EndOfStreamError", err);
        } else {
            panic!("no expected exception: EndOfStreamError");
        }

    First of all, this doesn't actually test the type of the error as it should (and as it does in all other languages). Second, the println! is actually useless, because Rust by default doesn't display stdout of passed tests (see https://blog.ssanj.net/posts/2023-09-24-how-to-see-stdout-when-running-tests-in-rust.html). So the above is equivalent to:

    assert!(res.is_err(), "no expected exception: EndOfStreamError");

    Which makes it even more visible that it doesn't test or indicate (so that it could be checked manually, which I don't think would be good enough, but would at least make some sense) the error type whatsoever.

    Testing the error type is actually very important - if you don't do that, you're just blindly accepting any error that may arise for any reason (which is very prone to producing false negative test results, because the test might as well just crash with some exception before it even starts, but the test result will be "all good").

    I've seen this before in Lua - some time ago, I decided to replace luaunit.assertError (which doesn't check the error type) with luaunit.assertErrorMsgMatches in bb20b5a#diff-335bd0c62964dced90d2d661679316bb2ab4be1e36b80db89f4eaee7ed94bc83. I didn't expect this change to reveal any bug, but it did - the unable to decide endianness error could actually never be thrown (such case was treated as big-endian), which I fixed in kaitai-io/kaitai_struct_compiler@f79666f. And luaunit.assertError(DefaultEndianExprException.from_file, "src/endian_expr.bin") didn't fail, because it caught an error that occurred because the arguments to DefaultEndianExprException.from_file did not match, so no parsing actually took place - the invocation was supposed to be luaunit.assertError(DefaultEndianExprException.from_file, DefaultEndianExprException, "src/endian_expr.bin").


However, I managed to correct (almost) all the wrong things I found, so I'm happy with the result. I think it's quite robust now and there should be few problems with it in the future.

@generalmimon generalmimon merged commit a0bd64e into kaitai-io:master Sep 7, 2024
@revitalyr
Copy link
Contributor

revitalyr commented Sep 10, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants