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

Python: to_i on a enum may fail, if a enum is not parsed as known #815

Closed
KOLANICH opened this issue Oct 2, 2020 · 2 comments
Closed

Python: to_i on a enum may fail, if a enum is not parsed as known #815

KOLANICH opened this issue Oct 2, 2020 · 2 comments

Comments

@KOLANICH
Copy link

KOLANICH commented Oct 2, 2020

Proposed fix is to replace Enum to IntEnum in

https://github.com/kaitai-io/kaitai_struct_compiler/blob/8d5308dc6591f82cd7e1bd0233af2772b5f68db8/shared/src/main/scala/io/kaitai/struct/languages/PythonCompiler.scala#L462

https://github.com/kaitai-io/kaitai_struct_compiler/blob/8d5308dc6591f82cd7e1bd0233af2772b5f68db8/shared/src/main/scala/io/kaitai/struct/languages/PythonCompiler.scala#L465

and use

override def enumToInt(v: Ast.expr, et: EnumType): String = s"int(${translate(v)})"

in

https://github.com/kaitai-io/kaitai_struct_compiler/blob/61a577cb67e1fb7e3d9c6603554c36e7c6f47849/shared/src/main/scala/io/kaitai/struct/translators/PythonTranslator.scala#L84L85

Also, IntEnums can be interacted to numbers without explicit casting at all - they inherit int. Should we allow such for all the targets and determine if casting is needed automatically and apply it implicitly?

UPD: thanks for fixing this in a timely fashion ;) I had to implement a workaround. https://github.com/kaitaiStructCompile/kaitaiStructCompile.py/blob/16eef312efea66593b5a1e1b07bab6fd33b1b3b8/kaitaiStructCompile/postprocessors.py#L12-L132

@generalmimon
Copy link
Member

I've just hit this issue when porting serialization support to Python. If an enum value cannot be converted to an integer, it cannot be serialized. And the current enumToInt implementation only works for known values from the enum definition:

PythonTranslator.scala:87-88

  override def enumToInt(v: Ast.expr, et: EnumType): String =
    s"${translate(v)}.value"

When the KS-generated code parses an enum field, it first reads the integer value, which goes through the doEnumById translator method:

PythonTranslator.scala:60-61

  override def doEnumById(enumTypeAbs: List[String], id: String): String =
    s"${PythonCompiler.kstreamName}.resolve_enum(${PythonCompiler.types2class(enumTypeAbs)}, $id)"

So in case of Python, the resolve_enum runtime method is used:

kaitaistruct.py:436-445

    @staticmethod
    def resolve_enum(enum_obj, value):
        """Resolves value using enum: if the value is not found in the map,
        we'll just use literal value per se. Works around problem with Python
        enums throwing an exception when encountering unknown value.
        """
        try:
            return enum_obj(value)
        except ValueError:
            return value

As a result, the value of an "enum attribute" in KS terms can in Python be represented either as an object instance of the enum class, or a plain integer (int). But integers don't have any value property:

ERROR: test_read_write_roundtrip (specwrite.test_enum_for_unknown_id.TestEnumForUnknownId)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\temp\kaitai_struct\tests\spec\python\specwrite\common_spec.py", line 26, in test_read_write_roundtrip
    orig_ks._write(new_io)
  File "C:\temp\kaitai_struct\runtime\python\kaitaistruct.py", line 67, in _write
    self._write__seq(io)
  File "C:\temp\kaitai_struct\tests\compiled\python\testwrite\enum_for_unknown_id.py", line 32, in _write__seq
    self._io.write_u1(self.one.value)
AttributeError: 'int' object has no attribute 'value'
class EnumForUnknownId(ReadWriteKaitaiStruct):
    # ...
    def _read(self):
        self.one = KaitaiStream.resolve_enum(EnumForUnknownId.Animal, self._io.read_u1())

    def _write__seq(self, io=None):
        # ...
        self._io.write_u1(self.one.value)

@KOLANICH
Copy link
Author

IntEnum doesn't require using .value. It is as god as an int. Also there is an IntFlag, which can be used, if an enum has a certain structure (I mean athe compiler can try to detect this, but IMHO such int fields should be converted into structs of bit-sized ints where it makes sense, so maybe also emit warnings with the replacement KS code).

generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Oct 9, 2023
I'm going to change the base class for Python enums to
[`enum.IntEnum`](https://docs.python.org/3/library/enum.html#enum.IntEnum),
as suggested in kaitai-io/kaitai_struct#815.
An `enum.IntEnum` object behaves almost like an `int` in many cases, but
not all. For example, if you have a enum def like this:

```python
class Animal(enum.IntEnum):
    DOG = 4
    CAT = 7
```

...then `Animal.DOG == 4` is `True`. Consequently, if you do
`self.assertEqual(Animal.DOG, 4)` in a unit test, it will pass. However,
the result of `str(Animal.DOG)` depends on the version of Python:

- before Python 3.11: `str(Animal.DOG) == 'Animal.DOG'`
- since Python 3.11: `str(Animal.DOG) == '4'`

So at least as long as we support older Pythons than 3.11, a KS
expression `animal::dog.to_i.to_s` has to be translated as
`str(int(Animal.DOG))` (not just `str(Animal.DOG)`, because that won't
yield the correct result `'4'` in Python 3.10 and older). Therefore,
this commit adds an assertion to protect against this potential mistake.
generalmimon added a commit to kaitai-io/kaitai_struct_tests that referenced this issue Oct 9, 2023
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

2 participants