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

Ruby, Lua: missing requires not detected by tests due to require leaks #1099

Open
generalmimon opened this issue Mar 31, 2024 · 1 comment
Open

Comments

@generalmimon
Copy link
Member

generalmimon commented Mar 31, 2024

I was wondering how it is possible that the EnumImport test passes in Ruby (see https://ci.kaitai.io/ or ci.json:239-243 of the ruby/3.3-linux-x86_64 target).

The enum_import.ksy spec imports two other .ksy specs (enum_0 and enum_deep) and defines two seq fields, each of an enum type defined in one of the imported specs:

meta:
  id: enum_import
  endian: le
  imports:
    - enum_0
    - enum_deep
seq:
  - id: pet_1
    type: u4
    enum: enum_0::animal
  - id: pet_2
    type: u4
    enum: enum_deep::container1::container2::animal

The problem is that the enum_import.rb (still generated by the latest KSC at the time of writing, i.e. 29f7a592) looks like this:

# This is a generated file! Please edit source .ksy file and use kaitai-struct-compiler to rebuild

require 'kaitai/struct/struct'

unless Gem::Version.new(Kaitai::Struct::VERSION) >= Gem::Version.new('0.11')
  raise "Incompatible Kaitai Struct Ruby API: 0.11 or later is required, but you have #{Kaitai::Struct::VERSION}"
end

class EnumImport < Kaitai::Struct::Struct
  def initialize(_io, _parent = nil, _root = self)
    super(_io, _parent, _root)
    _read
  end

  def _read
    @pet_1 = Kaitai::Struct::Stream::resolve_enum(Enum0::ANIMAL, @_io.read_u4le)
    @pet_2 = Kaitai::Struct::Stream::resolve_enum(EnumDeep::Container1::Container2::ANIMAL, @_io.read_u4le)
    self
  end
  attr_reader :pet_1
  attr_reader :pet_2
end

Notice that it refers to classes Enum0 and EnumDeep on these lines:

    @pet_1 = Kaitai::Struct::Stream::resolve_enum(Enum0::ANIMAL, @_io.read_u4le)
    #                                             ^^^^^
    @pet_2 = Kaitai::Struct::Stream::resolve_enum(EnumDeep::Container1::Container2::ANIMAL, @_io.read_u4le)
    #                                             ^^^^^^^^

... but there are no corresponding require statements, so the Ruby interpreter should have no idea what these names are. And yet this test passes in Ruby.

It turns out that the requires in Ruby are "leaky", i.e. they pollute the global namespace. So if any format or test Ruby files running before the above enum_import.rb contain require 'enum_0' and require 'enum_deep' (which is what the EnumImport class needs to run successfully) in the same Ruby interpreter process, the fact that the enum_import.rb file doesn't itself have the requires it needs is not detected by tests.

And if you search for require 'enum_0' and for require 'enum_deep', you can see that both requires have 2 occurrences in test specs.

If you disable the enum_0 in spec/ruby/enum_import_spec.rb:

RSpec.describe 'EnumImport' do
  it 'parses test properly' do
    require 'enum_import'
    # require 'enum_0'
    require 'enum_deep'

... it's not enough (i.e. the EnumImport test will still pass), because enum_0 is also tested directly, so require 'enum_0' is also in spec/ruby/enum_0_spec.rb:

RSpec.describe 'Enum0' do
  it 'parses test properly' do
    # require 'enum_0'

EnumImport will finally fail (as it should have all this time) only after you disable both these requires:

$ ./run-ruby

  ...

  3) EnumImport parses test properly
     Failure/Error: r = EnumImport.from_file('src/enum_0.bin')

     NameError:
       uninitialized constant EnumImport::Enum0
     # ./compiled/ruby/enum_import.rb:16:in `_read'
     # ./compiled/ruby/enum_import.rb:12:in `initialize'
     # C:/temp/kaitai_struct/runtime/ruby/lib/kaitai/struct/struct.rb:26:in `new'
     # C:/temp/kaitai_struct/runtime/ruby/lib/kaitai/struct/struct.rb:26:in `from_file'
     # ./spec/ruby/enum_import_spec.rb:8:in `block (2 levels) in <top (required)>'
generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Mar 31, 2024
See the source code of the KST translator - the `imports` list in .kst
specs is copied to the `extraImports` property of the `TestSpec` case
class, and `extraImports` are only used to generate `import` statements
into generated test specs in C++, Java, Nim and Ruby. This means
`import` statements in test specs don't need to duplicate
`meta/imports`, it's enough to have extra imports only for specs that
are referenced from expressions in `asserts` (for example, `imports:
enum_0` is needed in `enum_import.kst` because there is an assertion
with `expected: enum_0::animal::cat`).

Since `enum_import.kst` is the only spec that refers from `asserts` to
any of the specs listed in `imports`, this commit removes the
unnecessary `imports` from all other .kst specs.

This partly addresses
<kaitai-io/kaitai_struct#1099>. While this
doesn't solve the root of the problem, at least now tests that import
specs that are not tested directly and also not imported from any other
spec (or if they are, then only from a test that comes later in the
alphabet and thus has not run yet) no longer pass. This means that with
the current state of KSC, this breaks 5 tests in Ruby:

* CastToImported
* EnumToIClassBorder1
* ImportsAbs
* ImportsCircularA
* ImportsRel1

The fact that these tests now fail is a good thing, because ultimately,
we want to ensure that all generated Ruby format modules contain the
`require` statements they themselves need to function, which is not the
case right now, so they *should* be failing.

Ironically, I believe that the `imports` in KST specs removed in this
commit were in place specifically to make Ruby tests pass (because
AFAIK, they are unnecessary from the perspective of any other language).
However, this reliance of Ruby format modules on external `require`s is
incorrect.
@generalmimon
Copy link
Member Author

generalmimon commented Apr 1, 2024

The same problem occurs in Lua - enum_import.lua also has no require("enum_0") or require("enum_deep") but refers to Enum0 and EnumDeep:

-- This is a generated file! Please edit source .ksy file and use kaitai-struct-compiler to rebuild
--
-- This file is compatible with Lua 5.3

local class = require("class")
require("kaitaistruct")

EnumImport = class.class(KaitaiStruct)

function EnumImport:_init(io, parent, root)
  KaitaiStruct._init(self, io)
  self._parent = parent
  self._root = root or self
  self:_read()
end

function EnumImport:_read()
  self.pet_1 = Enum0.Animal(self._io:read_u4le())
  self.pet_2 = EnumDeep.Container1.Container2.Animal(self._io:read_u4le())
end

Unlike Ruby, test_enum_import.lua only contains require("enum_import") and not require("enum_0") (which is correct since none of the asserts refer to Enum0, see kaitai-io/kaitai_struct_tests#109). But like in Ruby, the require("enum_0") is obviously present in test_enum_0.lua:

-- Autogenerated from KST: please remove this line if doing any edits by hand!

local luaunit = require("luaunit")

-- require("enum_0")

TestEnum0 = {}

function TestEnum0:test_enum_0()

... and as long as it present, EnumImport passes in Lua despite the missing require("enum_0") in compiled/lua/enum_import.lua. Only if you disable it as above, then the EnumImport test finally breaks:

  • 🔴 2 tests broken:
    • Enum0: (...)
    • EnumImport:
      - {"status"=>"passed"}
      + {"status"=>"failed",
      +  "failure"=>
      +   {"file_name"=>nil,
      +    "line_num"=>nil,
      +    "message"=>
      +     "compiled/lua/enum_import.lua:18: attempt to index a nil value (global 'Enum0')",
      +    "trace"=>
      +     "stack traceback:\n" +
      +     "\tcompiled/lua/enum_import.lua:18: in method '_read'\n" +
      +     "\tcompiled/lua/enum_import.lua:14: in local 'init'\n" +
      +     "\t../runtime/lua/class.lua:70: in function <../runtime/lua/class.lua:66>\n" +
      +     "\t(...tail calls...)\n" +
      +     "\tspec/lua/test_enum_import.lua:10: in upvalue 'TestEnumImport.test_enum_import'"}}

@generalmimon generalmimon changed the title Ruby: missing requires not detected by tests due to require leaks Ruby, Lua: missing requires not detected by tests due to require leaks Apr 1, 2024
generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Apr 1, 2024
See the source code of the KST translator - the `imports` list in .kst
specs is copied to the `extraImports` property of the `TestSpec` case
class, and `extraImports` are only used to generate `import` statements
into generated test specs in C++, Java, Nim and Ruby. This means
`import` statements in test specs don't need to duplicate
`meta/imports`, it's enough to have extra imports only for specs that
are referenced from expressions in `asserts` (for example, `imports:
enum_0` is needed in `enum_import.kst` because there is an assertion
with `expected: enum_0::animal::cat`).

Since `enum_import.kst` is the only spec that refers from `asserts` to
any of the specs listed in `imports`, this commit removes the
unnecessary `imports` from all other .kst specs.

This partly addresses
<kaitai-io/kaitai_struct#1099>. While this
doesn't solve the root of the problem, at least now tests that import
specs that are not tested directly and also not imported from any other
spec (or if they are, then only from a test that comes later in the
alphabet and thus has not run yet) no longer pass. This means that with
the current state of KSC, this breaks 4 tests in Ruby:

* EnumToIClassBorder1
* ImportsAbs
* ImportsCircularA
* ImportsRel1

The fact that these tests now fail is a good thing, because ultimately,
we want to ensure that all generated Ruby format modules contain the
`require` statements they themselves need to function, which is not the
case right now, so they *should* be failing.

Ironically, I believe that the `imports` in KST specs removed in this
commit were in place specifically to make Ruby tests pass (because
AFAIK, they are unnecessary from the perspective of any other language).
However, this reliance of Ruby format modules on external `require`s is
incorrect.
generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Apr 1, 2024
See the source code of the KST translator - the `imports` list in .kst
specs is copied to the `extraImports` property of the `TestSpec` case
class, and `extraImports` are only used to generate `import` statements
into generated test specs in C++, Java, Nim and Ruby. This means
`import` statements in test specs don't need to duplicate
`meta/imports`, it's enough to have extra imports only for specs that
are referenced from expressions in `asserts` (for example, `imports:
enum_0` is needed in `enum_import.kst` because there is an assertion
with `expected: enum_0::animal::cat`).

Since `enum_import.kst` is the only spec that refers from `asserts` to
any of the specs listed in `imports`, this commit removes the
unnecessary `imports` from all other .kst specs.

This partly addresses
<kaitai-io/kaitai_struct#1099>. While this
doesn't solve the root of the problem, at least now tests that import
specs that are not tested directly and also not imported from any other
spec (or if they are, then only from a test that comes later in the
alphabet and thus has not run yet) no longer pass. This means that with
the current state of KSC, this breaks 4 tests in Ruby:

* EnumToIClassBorder1
* ImportsAbs
* ImportsCircularA
* ImportsRel1

The fact that these tests now fail is a good thing, because ultimately,
we want to ensure that all generated Ruby format modules contain the
`require` statements they themselves need to function, which is not the
case right now, so they *should* be failing.

Ironically, I believe that the `imports` in KST specs removed in this
commit were in place specifically to make Ruby tests pass (because
AFAIK, they are unnecessary from the perspective of any other language).
However, this reliance of Ruby format modules on external `require`s is
incorrect.
generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Apr 11, 2024
See the source code of the KST translator - the `imports` list in .kst
specs is copied to the `extraImports` property of the `TestSpec` case
class, and `extraImports` are only used to generate `import` statements
into generated test specs in C++, Java, Nim and Ruby. This means
`import` statements in test specs don't need to duplicate
`meta/imports`, it's enough to have extra imports only for specs that
are referenced from expressions in `asserts` (for example, `imports:
enum_0` is needed in `enum_import.kst` because there is an assertion
with `expected: enum_0::animal::cat`).

Since `enum_import.kst` is the only spec that refers from `asserts` to
any of the specs listed in `imports`, this commit removes the
unnecessary `imports` from all other .kst specs.

This partly addresses
<kaitai-io/kaitai_struct#1099>. While this
doesn't solve the root of the problem, at least now tests that import
specs that are not tested directly and also not imported from any other
spec (or if they are, then only from a test that comes later in the
alphabet and thus has not run yet) no longer pass. This means that with
the current state of KSC, this breaks 4 tests in Ruby:

* EnumToIClassBorder1
* ImportsAbs
* ImportsCircularA
* ImportsRel1

The fact that these tests now fail is a good thing, because ultimately,
we want to ensure that all generated Ruby format modules contain the
`require` statements they themselves need to function, which is not the
case right now, so they *should* be failing.

Ironically, I believe that the `imports` in KST specs removed in this
commit were in place specifically to make Ruby tests pass (because
AFAIK, they are unnecessary from the perspective of any other language).
However, this reliance of Ruby format modules on external `require`s is
incorrect.
generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Apr 11, 2024
See the source code of the KST translator - the `imports` list in .kst
specs is copied to the `extraImports` property of the `TestSpec` case
class, and `extraImports` are only used to generate `import` statements
into generated test specs in C++, Java, Nim and Ruby. This means
`import` statements in test specs don't need to duplicate
`meta/imports`, it's enough to have extra imports only for specs that
are referenced from expressions in `asserts` (for example, `imports:
enum_0` is needed in `enum_import.kst` because there is an assertion
with `expected: enum_0::animal::cat`).

Since `enum_import.kst` is the only spec that refers from `asserts` to
any of the specs listed in `imports`, this commit removes the
unnecessary `imports` from all other .kst specs.

This partly addresses
<kaitai-io/kaitai_struct#1099>. While this
doesn't solve the root of the problem, at least now tests that import
specs that are not tested directly and also not imported from any other
spec (or if they are, then only from a test that comes later in the
alphabet and thus has not run yet) no longer pass. This means that with
the current state of KSC, this breaks 4 tests in Ruby:

* EnumToIClassBorder1
* ImportsAbs
* ImportsCircularA
* ImportsRel1

The fact that these tests now fail is a good thing, because ultimately,
we want to ensure that all generated Ruby format modules contain the
`require` statements they themselves need to function, which is not the
case right now, so they *should* be failing.

Ironically, I believe that the `imports` in KST specs removed in this
commit were in place specifically to make Ruby tests pass (because
AFAIK, they are unnecessary from the perspective of any other language).
However, this reliance of Ruby format modules on external `require`s is
incorrect.
generalmimon added a commit to kaitai-io/kaitai_struct_compiler that referenced this issue Apr 15, 2024
... instead of expecting the calling code to load everything that the
generated modules need.

Fixes these tests for Ruby:

* ImportsCastToImported
* ImportsCastToImported2
* ImportsParamsDefEnumImported
* ImportsParamsDefUsertypeImported

We intentionally use `require_relative` (instead of `require` as usual)
and only for imported types (not opaque types) - see the added code
comment in `RubyCompiler`'s implementation of `externalTypeDeclaration`
for more details.

Until now, we've been kind of living under the impression that the
generated Ruby files don't need explicit imports, because until
recently, all of our import-related tests in
https://github.com/kaitai-io/kaitai_struct_tests were passing despite
the fact that KSC has never generated any `require`s in Ruby. However,
it turned out that this was just a coincidence described in
kaitai-io/kaitai_struct#1099, caused by the
fact that the Ruby's `require` pollutes the global namespace and that
the imported specs for which we were missing `require`s were already
required by some other test. This was not the case in the 4 tests that
this commit fixes as mentioned above, which were added recently and all
import a format spec that no other test uses.

This "missing `require`s" problem would have been an issue in
[KSV](https://github.com/kaitai-io/kaitai_struct_visualizer) too, but
KSV works around it by requiring every single `*.rb` file that KSC has
generated (which will include any imported .ksy specs), and opaque types
must be explicitly imported using the `-r` command-line option. This
should still work after this commit, although calling `require` with
each `*.rb` file in the directory of generated format modules becomes
unnecessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant