Add endian keyword argument to type reads#144
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 22 22
Lines 2726 2773 +47
=====================================
- Misses 2726 2773 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
15ab019 to
8a6b8b6
Compare
8a6b8b6 to
1cad01c
Compare
Merging this PR will not alter performance
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
c8419f6 to
df8bbb9
Compare
|
I rebased on #140 and added the context manager. |
Oh oops, you added it in the PR message:
anyway, my comment about the risks of using **kwargs still stand. In which case would it break backwards compatibility? |
f8fb6b1 to
fd1fe6d
Compare
|
Improved the docstrings and replied to your comment 😄. |
|
If there is a case where someone uses c_def = """
struct test {
char[4] endian;
};
"""
c_test = cstruct().load(c_def)
>>> c_test.test(endian='help')
<test endian='help'>
>>> c_test.test(b'help', endian='@')
<test endian=b'h'>
>>> c_test.test(b'', endian=b'help')
*** ValueError: Invalid endianness: 'help', expected one of <, >, !, @, =, network, little, bigIs it an idea to prefix it with |
fd1fe6d to
b6540a0
Compare
|
@Miauwkeru what exactly is the weird behavior you're speaking of? I added a unit test without checking the behavior first, with how I would expect it to react, and it passed on the first try. So I'm not sure in what sense it's behaving strangely. Perhaps your strange behavior is because you didn't type your field correctly? It should be |
b6540a0 to
26943c5
Compare
The malformed struct was indeed valid, that fixed some things and due to it I found a different bug There seems to be an issue with a struct only containing ipdb> c_test.test(b"help", endian='<')
*** TypeError: __init__() got multiple values for argument 'endian'
ipdb> c_test.test(b"help\0", endian='<')
<test endian=b'help'> |
|
I'd call this an invalid invocation and not a bug. That invocation would in my opinion be the equivalent of this Python code: In [1]: class A:
...: def __init__(self, a):
...: self.a = a
...:
In [2]: A(1, a=2)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[2], line 1
----> 1 A(1, a=2)
TypeError: A.__init__() got multiple values for argument 'a'You hit this micro optimization: dissect.cstruct/dissect/cstruct/types/structure.py Lines 74 to 82 in 91f2347 Which treats structure initialization with exactly one char/bytes field of exactly the same size to be a "new structure invocation", instead of a "parse these bytes" invocation. |
26943c5 to
5a24c01
Compare
0496f2a to
4f5f8e1
Compare
4f5f8e1 to
21416dd
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends dissect.cstruct’s type read/write API to support overriding endianness on a per-call basis (including structs, unions, and pointers), and updates the compiler-generated readers and tests accordingly. It also adds a convenience context-manager API to obtain a copied cstruct instance with a different endianness.
Changes:
- Add
endian=keyword support throughoutBaseTyperead/write/dump paths and propagate it into generated structure/union parsing and the compiler output. - Update core builtin types (ints, packed, enum, char/wchar, void, leb128, pointer, structures/unions) to accept and use the passed endianness.
- Add/adjust tests for endian overrides, pointer behavior, compiler output, and bitbuffer endian semantics; add warnings when registering custom types lacking the new signature.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_types_union.py | Adds coverage for endian override when reading unions and dumping with explicit endian. |
| tests/test_types_structure.py | Adds coverage for endian override for structs and an endian field name conflict case. |
| tests/test_types_pointer.py | Updates pointer construction expectations and adds coverage for endian override + dereference behavior. |
| tests/test_types_custom.py | Updates custom type signatures for new endian kw-only API and adds tests for warning behavior. |
| tests/test_types_base.py | Updates test custom types to accept endian kw-only and propagate it when delegating. |
| tests/test_compiler.py | Updates expected generated source to use passed endian and pass it into nested reads/pointers. |
| tests/test_bitbuffer.py | Updates BitBuffer API usage (endian= kw-only) and adjusts expected bitfield read results. |
| tests/test_basic.py | Updates test custom type signatures and adds tests for the new endianness context manager helpers. |
| dissect/cstruct/util.py | Introduces a normalized endianness→byteorder map and validates pack/unpack endianness inputs. |
| dissect/cstruct/types/wchar.py | Threads endian through wchar read/write encoding decisions. |
| dissect/cstruct/types/void.py | Updates void read/write signatures to accept endian. |
| dissect/cstruct/types/structure.py | Propagates endian into structure and union parsing/writing and stores union endian for rebuild/update. |
| dissect/cstruct/types/pointer.py | Stores pointer parse endian, propagates endian into pointer reads/dereference, and adds per-instance caching. |
| dissect/cstruct/types/packed.py | Uses passed endian for struct packing/unpacking and updates method signatures. |
| dissect/cstruct/types/leb128.py | Updates signatures to accept endian and threads it through _read_0 calls. |
| dissect/cstruct/types/int.py | Switches to endian→byteorder map and updates signatures to accept endian. |
| dissect/cstruct/types/enum.py | Propagates endian through enum read/write and array helpers. |
| dissect/cstruct/types/char.py | Updates char read/write signatures to accept endian and threads it through array helpers. |
| dissect/cstruct/types/base.py | Adds endian= override support for read/write/dumps and introduces normalize_endianness(). |
| dissect/cstruct/cstruct.py | Normalizes instance endianness, adds custom type signature warning checks, and adds endianness context managers. |
| dissect/cstruct/compiler.py | Updates generated _read signature and emitted code to use the passed endian. |
| dissect/cstruct/bitbuffer.py | Makes endian kw-only and propagates it into type reads/writes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cb77d7a to
941293d
Compare
|
Copilot review remarked this comment: #144 (comment) Which on second thought is actually fair. I think it's nicer and a bit more predicable if the existing instance is modified, instead of a fresh copy created. It'd also be a bit faster. |
941293d to
c3e00e8
Compare
c3e00e8 to
e627a24
Compare
e627a24 to
b2bf53a
Compare
i agree on this, and it's a contextmanager so i would also expect this. |
i believe this is a "known" issue right? Some keyword arguments can conflict with the structure field names. I'm sure i've encountered it before. I'm not sure if that is also the case here, but it does look like it from the example. I'm not sure if there is a solid solution for that, other than using positional arguments. |
|
For the record, the following works: In [12]: cs.load("""
...: struct test {
...: char endian;
...: };
...: """)
Out[12]: <dissect.cstruct.cstruct.cstruct at 0x1076d4230>
In [13]: cs.test(endian=b"\x01")
Out[13]: <test endian=b'\x01'>It's specifically this case that breaks right now (the input bytes matches the length of a structure that consists solely out of a single field type that map to bytes): In [14]: cs.test(b"\x01", endian="<")
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[14], line 1
----> 1 cs.test(b"\x01", endian="<")
File ~/Shares/src/dissect/dissect.cstruct/dissect/cstruct/types/structure.py:87, in StructureMetaType.__call__(cls, *args, **kwargs)
78 def __call__(cls, *args, **kwargs) -> Self: # type: ignore
79 if (
80 cls.__fields__
81 and len(args) == len(cls.__fields__) == 1
(...) 85 ):
86 # Shortcut for single char/bytes type
---> 87 return type.__call__(cls, *args, **kwargs)
88 if not args and not kwargs:
89 obj = type.__call__(cls)
TypeError: __init__() got multiple values for argument 'endian'Removing the following "micro optimization" resolves this: diff --git i/dissect/cstruct/types/structure.py w/dissect/cstruct/types/structure.py
index 1ba1a88..e8b1342 100644
--- i/dissect/cstruct/types/structure.py
+++ w/dissect/cstruct/types/structure.py
@@ -76,15 +76,6 @@ class StructureMetaType(MetaType):
return super().__new__(metacls, name, bases, classdict)
def __call__(cls, *args, **kwargs) -> Self: # type: ignore
- if (
- cls.__fields__
- and len(args) == len(cls.__fields__) == 1
- and isinstance(args[0], bytes)
- and issubclass(cls.__fields__[0].type, bytes)
- and len(args[0]) == cls.__fields__[0].type.size
- ):
- # Shortcut for single char/bytes type
- return type.__call__(cls, *args, **kwargs)
if not args and not kwargs:
obj = type.__call__(cls)
object.__setattr__(obj, "__dynamic_sizes__", {})After: In [5]: cs.test(b"\x01", endian="<")
Out[5]: <test endian=b'\x01'>I'm fine with removing this micro-optimization too, as it probably doesn't gain us that much anyway. |
|
what about this: from dissect.cstruct import cstruct
cs = cstruct("""
struct test1 {
uint16 foo;
uint16 bar;
uint16 endian;
};
struct test2 {
uint16 endian;
};
""")and then: $ uv run python -i t.py
>>> print(cs.test1(0x1337, endian=">"))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/workspaces/dissect.cstruct/dissect/cstruct/types/structure.py", line 508, in __repr__
value = hex(value)
^^^^^^^^^^
TypeError: 'str' object cannot be interpreted as an integer
>>> print(cs.test2(0x1337, endian=">"))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/workspaces/dissect.cstruct/dissect/cstruct/types/structure.py", line 93, in __call__
return super().__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/workspaces/dissect.cstruct/dissect/cstruct/types/base.py", line 54, in __call__
return type.__call__(cls, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: __init__() got multiple values for argument 'endian'UPDATE: ok talked with Schamper, i'm a dummy and don't mind the usecase above. The |
fc45e08 to
cbfe298
Compare
cbfe298 to
35def67
Compare
| if "endian" not in signature.parameters: | ||
| warnings.warn( | ||
| f"Custom type {type_name} is missing the 'endian' keyword-only parameter in its {method} method. " # noqa: E501 | ||
| "Please refer to the changelog of dissect.cstruct 5.0 for more information.", |
There was a problem hiding this comment.
don't forget this when creating the changelog. Or do we already keep track of a changelog in the docs?
There was a problem hiding this comment.
No not yet, I plan to make a big changelog when we start preparing to release 5.0.
Partially solves #143, but I'd still like the context manager approach to be in there too. But that depends on #140.
Adds the ability to override the endianness on single type reads. So for example:
This should work on any type, including structs, unions and pointers.
Because the function signature for type reading and writing changes because of this, I also added a check when adding a custom type to ensure it's signature is correct. If it's not, it will either throw a (descriptive) error or issue a warning. I also intentionally added a
**kwargsto the function signature to allow for future expansion without breaking backwards compatibility again.I'm not 100% if the behavior of pointers is as expected. I implemented it so that:
.dereference(endian=).dumps()will fall back to the cstruct instance endianness, unless overridden