Add type hints - #611
Conversation
eliben
left a comment
eliben
left a comment
There was a problem hiding this comment.
Just a couple of initial questions
In general, I'd really prefer someone with Python type checking experience to take a careful look at this
eliben
left a comment
eliben
left a comment
There was a problem hiding this comment.
One question and general comment: would it be possible to split this PR to multiple? Even if just for the sake of review - could we start with a small-medium PR with a representative set of changes? It's OK if the intermediate steps don't fully type check until everything has landed
Yes, I can do that. Any advise on how to split best? Internel / low-level / high-level? |
Yes, by layers could be a great way to slice it. Starting at the lowest possible and then progressing upwards |
|
Where do we stand with this PR? How much of it has already been added? |
Sorry for the delay, I'm currently busy otherwise. Hope to find some time next weekend. |
|
What's the status here? |
b8d0cab to
c5f4dc8
Compare
The bad news: Sadly I'm busy otherwise .
I have been experimenting with typeguard, which turns those type-hints into runtime checks. This allows validating those hints when running the test-suite. Sadly that drastically increases the runtime and – even more sad – shows several errors in my type annotations. |
Timmmm
left a comment
Timmmm
left a comment
There was a problem hiding this comment.
This looks great to me. It would be good to split into smaller PRs - maybe you could do one with all the basic str, int, None types that are pretty obvious and then a second one with everything else?
On the other hand that's super tedious to do and nobody has time for that. If this were my project I think I would just merge it as-is and improve it in future PRs.
If this passes Pyright that's already a huge achievement and improvement.
|
I have a hunch this will never see the light of day. Would it be feasible to scale back the mission - type-annotate the user facing part of the API and mark the private stuff as off limits for the type checker? |
|
Yeah this should just be merged IMO. It's pretty much impossible to take an untyped Python project and add correct type hints to it all in one go. Once this is merged you can gradually fix the errors until it all type checks, and then enable type checking with Pyright in CI (or Pyrefly/Ty maybe by the time that actually happens!). |
|
Awesome work! I would love to see this merged, we got bitten by it recently here: pwndbg/pwndbg#3470 (comment) |
|
+1 for just merging. Anything helps really. |
|
Thanks for all the work! I'll have to think about my strategy here overall... |
|
Ty and Pyrefly are not really ready for production yet. Pyright is definitely the way to go for now. IMO Mypy is not worth thinking about. It is strictly worse than Pyright. |
YMMV: They all have their pros and cons and currently find different things. I'm running fine with I've spent some more time on this and have been able to reduce the number of issues:
|
Any updates on this? Merging this PR is not going to break any end users because there are no functional changes. The PR is +1,686 -1,088 which is not small but honestly not that big either, I've gone through it personally. pyelftools typing is currently broken for end users, this will only have a positive impact. If there are any issues, we will fix them when we get to them (I will gladly send a PR to fix such an issue if I encounter it), and again, they will not be functional, only part of peoples type checking pipeline. Most people have probably disabled type checking for pyelftools at this point (pwndbg has, for instance) since, again, it is currently flashing red due to the typed marker and there being no types. By delaying merging this you are only creating more work for @pmhahn who has to rebase and resolve conflicts. Could you elaborate what your thinking on this situation is? |
|
|
Only the near final bits remain: This are now some special type hints:
The last (extra) bits as in #657 |
I've closed it because it's not really useful and we've been coordinating the work here. For all practical purposes, type hints have already been added; we're now working through remaining details and enhancements. Is this PR (611) now ready for a review and merge? |
Okay.
Yes, these are the final bits. |
| from elftools.elf.elffile import ELFFile | ||
|
|
||
| try: | ||
| from typeguard import suppress_type_checks |
There was a problem hiding this comment.
typeguard turns those type-annotations into runtime checks: I've been using this to validate, that my type-annotations actually match what is used at runtime. It found many errors in my initial typing and I've been running with it since.
As the commit message documents, 4e4ef81 changed the signature of the loader to use str only. But the test test_relative_loader_rejects_bytes_paths() calls ELFFile.make_relative_loader(bytes), which violates that type-hint: typeguard will detect this and raise an exception, which breaks the test. Thus typeguard must be disabled for that specific test (only):
$ python3 -m unittest discover . test_debuglink.py
..E.
======================================================================
ERROR: test_relative_loader_rejects_bytes_paths (test.test_debuglink.TestDebuglink.test_relative_loader_rejects_bytes_paths)
----------------------------------------------------------------------
Traceback (most recent call last):
File "test/test_debuglink.py", line 124, in test_relative_loader_rejects_bytes_paths
ELFFile.make_relative_loader(b'sample.elf')
File "elftools/elf/elffile.py", line 117, in make_relative_loader
def make_relative_loader(base_path: str) -> Callable[[str], IO[bytes]]:
File ".venv/lib/python3.12/site-packages/typeguard/_functions.py", line 180, in check_argument_types_internal
check_type_internal(value, annotation, memo)
File ".venv/lib/python3.12/site-packages/typeguard/_checkers.py", line 994, in check_type_internal
raise TypeCheckError(f"is not an instance of {qualified_name(origin_type)}")
typeguard.TypeCheckError: argument "base_path" (bytes) is not an instance of str
----------------------------------------------------------------------
Ran 4 tests in 0.216s
FAILED (errors=1)If you drop this, no-body will be able to run the test-suite with typeguard unless you re-add it manually.
Alternative: drop the test or broaden the exception to also accept typeguard.TypeCheckError(Exception)
|
|
||
| from elftools.elf.elffile import ELFFile | ||
| from elftools.elf.hash import ELFHashTable, GNUHashTable | ||
| from elftools.elf.hash import ELFHashTable, GNUHashTable, _SymbolTable |
There was a problem hiding this comment.
I wonder: can we just leave type checking out of these tests?
It's a shame to convolute the test code for this; I'm not sure the value is worth it.
There was a problem hiding this comment.
Again typeguard: The tests pass None instead of some real-types, which typeguard does not like. As such unitest.mock.Mock must be used to at least fake enough to silence typeguard.
Being able to run the unit-test with typeguard enabled was a big win, so I prefer to keep this, but YMMV.
There was a problem hiding this comment.
I don't get it.... I don't find any mentions of typeguard in the repo right now (other than pyproject.toml).
Generally, I'd like to avoid this extra dependency at all, if possible. Our goals may be different here -- my main interest is ensure that pyelftools's public API has type information so clients / users can benefit from it for their own code and documentation purposes. I care much less about the library's internals being deeply type checked, and tests even less so. So please let's simplify this as much as possible.
There was a problem hiding this comment.
typeguard is no hard-dependency; that's why I've added that try: import typeguard; except ImportError: fallback thing so you can just use elftools as before.
It's similar to all those if TYPE_CHECKING: import …s which only add internal dependencies, when you run any type-checker. typeguard is just another one, but in contrast to mypy, pyright, pyrefly, ty being static – they only look at the code but do not execute it – typeguard is dynamic and checks types while executing the code. That's a great way to validate the type hints as there were many cases, where I started with – for example – bytes but than had it to change it to list[int] or add a | None, as reality was different from reading just the code and guessing things.
As soon as you extend pyelftools and add new functions/methods, you again will have the problem to add and validate new type hints; running the test suite with typeguard is a great way to get this almost for free – actually with (only) a drastic performance drop as all argument and return-values are validated, which takes time.
Now that ELFHashTable.__init__() is typed, your (or anybodies) editor will tell you that passing None to elffile (1st) and symboltable (4th argument) is invalid from a type-perspective. In this case its okay, but you don't want to add | None to those 2 argument just to have the tests clean as that would required checking for those Nones in may other locations.
While you could use elffile = symboltable = cast('Any', None) or add a # type: ignore there to silence the type-checkers, sadly that's not sufficient for typeguard as it explicitly checks, if the Protocol is implemented:
$ python3 -m unittest discover . -k test.test_hash.TestELFHash.test_empty_table_without_header
E
======================================================================
ERROR: test_empty_table_without_header (test.test_hash.TestELFHash.test_empty_table_without_header)
Verify we can handle an empty (0 byte) ELF hash section.
----------------------------------------------------------------------
Traceback (most recent call last):
File "test/test_hash.py", line 67, in test_empty_table_without_header
empty_hash_section = ELFHashTable(elffile, 0, 0, symboltable)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "elftools/elf/hash.py", line 44, in __init__
def __init__(
File ".venv/lib/python3.12/site-packages/typeguard/_functions.py", line 180, in check_argument_types_internal
check_type_internal(value, annotation, memo)
File ".venv/lib/python3.12/site-packages/typeguard/_checkers.py", line 989, in check_type_internal
checker(value, origin_type, args, memo)
File ".venv/lib/python3.12/site-packages/typeguard/_checkers.py", line 862, in check_protocol
raise TypeCheckError(
typeguard.TypeCheckError: argument "symboltable" (None) is not compatible with the _SymbolTable protocol because it has no method named 'get_symbol'
----------------------------------------------------------------------
Ran 1 test in 0.001s
FAILED (errors=1)As unittest.mock.MagicMock is part of any standard Python since several years and it only needs to prototype to fake, importing _SymbolTable here to satisfy both static and dynamic type checkers here seems like a minor price to pay.
key may either be an `int` or `EnumInt`. Declare a type requiring minimum an `int`. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Declare a protocol for the relocation functions, which all implementations must follow. Callable is insufficient as some of those functions are called with named arguments. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Since 4e4ef81 ("Make make_relative_loader expect strings, not bytes") `path` is of type `str`. Type it like that. But there is one test, which explicitly passes `bytes` to test, if the wrong type is rejected. This wrong type is detected by typeguard and causes the test to fail. As such disable typeguard for that single test. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Type helper function _reverse_dict() so that we get the correct type automatically in every place it is used. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Segment is similar to dict[Any, Any], which requires many explicit type-casts in many locations. To improve this, add some @Overloads to get better types similar to TypedDict. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Section is similar to dict[Any, Any], which requires many explicit type-casts in many locations. To improve this, add some @Overloads to get better types similar to TypedDict. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
We know precisely, when describe_reg_name() will return a `str` and not `None`. So save us from having to check for None in many locations, add some @Overloads to get better types. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Introduce TypedDict for RelocationTables to get better typing for get_relocation_tables(). Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
There are multiple classes, which can function as a StringTable. They do not share a common super-class, which could be used for typing. As such declare a Protocol, which captures the signature each of those implementations must follow. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
There are multiple classes, which can function as a SymbolTable. They do not share a common super-class, which could be used for typing. As such declare a Protocol, which captures the signature each of those implementations must follow. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
GNUHashTable and ELFHashTable to not share a common base class. As such assigning one or the other to the same variable results in a type error. Rename one instance. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
Assert that the fetched section is of type DynamicSection. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
For typing mirror the elf_assert() as assert. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
AttributesSubsubsection needs some type hints. Signed-off-by: Philipp Hahn <phahn-oss@avm.de>
|
🥳 |
|
could we get a new release cut with these changes? |
|
We can cut a new release if this effort is done. @pmhahn anything else remaining? |
|
Please follow #660 for a new release, and use it to report any issues |
7 participants
This is the mayor PR to add Python type hints #514 – without #609 this will not be complete as
elftools.construct.Containeris used in many places, which is a container forAnything: retrieving values from it will be typedAny, which basically means untyped: without manually type-hinting every such use case those values do propagate further and even spill into the public API.Because of missing 6f99ce0 running
mypywill find the following errors:Similar missing db4fb21 is responsible for
These I do not know how to fix - their type is not static and depends dynamically on the opened ELF file:
And finally the last group of issues, which are also caused by missing cc7b1ea:
Please have a 1st look.
Then we can decide on how to proceed, e.g. just merge it or try to extract a subset for only some public API files.