Repository navigation
Update mypy and run it in strict mode - #302
Merged
waskyo merged 16 commits intoOct 29, 2026
Merged
Conversation
sipma
reviewed
Oct 25, 2026
sipma
reviewed
Oct 25, 2026
sipma
approved these changes
Oct 25, 2026
sipma
left a comment
sipma
left a comment
Contributor
There was a problem hiding this comment.
Looks great! I especially appreciate the bugs you found and fixed, like in XXpr.py and MIPSInstruction and some others. Thank you very much for bringing the code up to a higher standard.
this also updates the version of the checkout action to the latest one
also add type annotations to a couple of index parameters that were causing mypy to think we were return Any
A number of type annotations assumed we were doing 'import chb'
- get_relocation_tables: sh.index is a string, self.sections is a dict with int keys. move the converstion to int outside the if check so the check and subsequent access are valid - as_dictionary: fix possible typo, using s from a previous loop instead of ss from current loop
mypy is still unhappy though. We're combining ASTStmtLabels with ASTStmts which are not compatible. There doesn't seem to be a good solution that doesn't rock the boat on this poorly understood function so we're doing type:ignore for the time being.
mostly missing type annotations and wrong imports
waskyo
force-pushed
the
run_mypy_in_strict_mode
branch
from
October 29, 2026 14:06
1aec8dc to
c3e8787
Compare
The code was comparing CallTarget to strings. This commit changes various locations to use `CallTarget.name` for the comparison. is_call_to_app_function required a bigger change: first, we need to check that the returned call target is an AppTarget (which is what the function is looking for) and then we need to compare its address since that is what the passed function argument represents. Address is an attribute that only some of the CallTarget classes provide so we cannot access it willy-nilly. Hat tip to the overlord for catching this.
Mostly to make mypy happy
Switch to using left shift instead. That could break if shiftamount is negative but I don't think that's possible?
This was making mypy really mad and fixing it while keeping the generic would have required a lot of refactoring. Instead, we now have a FileHeader ABC, which ELFHeader and PEHeader inherit from. AppAccess and its users switch to that. AppAccess also gets two new methods to get the ELF/PE-typed headers so that code that only supports one or the other can be explicit about that. The methods in the ABC represent what the current code calls generically. I had to add a is_big_endian method to PEHeader which always returns False, according to the internet PE binaries are always little endian. Along the way, fix some issues where we check for None being returned, print an error, but then continued to use the invalid value. For the archaelogists: AppAccess was originally made generic in 28bb704
We raise an exception at constructor time if given an AppAccess wrapping a non-ELF binary. Also, compare_executable_content and relational_compare_elfdata were accessing an ELF only field in their app's header causing mypy to complain. But since the code doesn't use the sectionheaders we can just remove the access
It was the whole point of this set of commits and I forgot to do it :-)
waskyo
force-pushed
the
run_mypy_in_strict_mode
branch
from
October 29, 2026 14:35
c3e8787 to
4df61b1
Compare
- Narrow the mypy error that we're ignoring in chb/app/Cfg.py. As suggested by the overlord, instead of ignoring all type issues we only ignore the error about returning the wrong thing (Incompatible return value type) - Use `1 << shiftamount` instead of `2 << (shiftamount - 1)` in fix for mypy issue with `**` operator (see efbe1aa for details). - Fix doc typo
waskyo
force-pushed
the
run_mypy_in_strict_mode
branch
from
October 29, 2026 15:08
4df61b1 to
caf299f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
And fix all the issues that popped up from the additional checks.
Changes:
is_big_endianmethod that always returns False, internet told me PE files are always little-endiantype: ignorethat we're not sure how to fix (f60e85f)None, print an error, but then continue to use the invalid value.