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

Fix: Issue #2307 #2439

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

harshit-wadhwani
Copy link

@harshit-wadhwani harshit-wadhwani commented Oct 4, 2024

Checklist

  • No CHANGELOG update needed
  • No new tests needed
  • No documentation update needed

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add bug fixes, new features, breaking changes and anything else you think is worthwhile mentioning to the master (unreleased) section of CHANGELOG.md. If no CHANGELOG update is needed add the following to the PR description: [x] No CHANGELOG update needed

@github-actions github-actions bot dismissed their stale review October 4, 2024 16:55

CHANGELOG updated or no update needed, thanks! 😄

@harshit-wadhwani
Copy link
Author

Annotated was introduced in Python 3.9.

@williballenthin
Copy link
Collaborator

we'll remove py3.8 in the next couple days and then we can retry the CI runs. thanks @harshit-wadhwani !

@mr-tz
Copy link
Collaborator

mr-tz commented Oct 23, 2024

mypy fails, could you take another look, please, @harshit-wadhwani?

capa/render/vverbose.py:202: error: Incompatible types in assignment (expression has type "str", variable has type "Literal['os', 'arch', 'format', 'match', 'characteristic', 'export', 'import', 'section', 'function name', 'substring', 'regex', 'string', 'class', 'namespace', 'api', 'property', 'number', 'bytes', 'offset', 'mnemonic', 'operand number', 'operand offset', 'basic block']")  [assignment]
capa/ida/plugin/model.py:545: error: Incompatible types in assignment (expression has type "str", variable has type "Literal['os', 'arch', 'format', 'match', 'characteristic', 'export', 'import', 'section', 'function name', 'substring', 'regex', 'string', 'class', 'namespace', 'api', 'property', 'number', 'bytes', 'offset', 'mnemonic', 'operand number', 'operand offset', 'basic block']")  [assignment]
capa/ida/plugin/model.py:547: error: Incompatible types in assignment (expression has type "str", variable has type "Literal['os', 'arch', 'format', 'match', 'characteristic', 'export', 'import', 'section', 'function name', 'substring', 'regex', 'string', 'class', 'namespace', 'api', 'property', 'number', 'bytes', 'offset', 'mnemonic', 'operand number', 'operand offset', 'basic block']")  [assignment]
capa/ida/plugin/model.py:549: error: Incompatible types in assignment (expression has type "str", variable has type "Literal['os', 'arch', 'format', 'match', 'characteristic', 'export', 'import', 'section', 'function name', 'substring', 'regex', 'string', 'class', 'namespace', 'api', 'property', 'number', 'bytes', 'offset', 'mnemonic', 'operand number', 'operand offset', 'basic block']")  [assignment]
Found 4 errors in 2 files (checked [20](https://github.com/mandiant/capa/actions/runs/11463966323/job/31899069300?pr=2439#step:9:21)9 source files)

@williballenthin
Copy link
Collaborator

williballenthin commented Oct 23, 2024

those mypy errors are outside of the code that @harshit-wadhwani has touched, though i agree it would be great if they can fix the errors.

it seems to be due to stricter checking that mypy can now do: mypy now knows that only some strings can be put into the type field, rather than any string, so it complains when a string is assigned there. to fix, we should assert that the string that we want to put there is one of the acceptable strings. something like assert type_name in {"os", "arch", ...}. except i'm not sure we have this list yet today, so it may be a new global constant to add and maintain.

to initialize this list, we could use the list of features in capa.rules.SUPPORTED_FEATURES and extract and deduplicate all the feature.name properties. something like:

VALID_FEATURE_NAMES = set()
for features in capa.rules.SUPPORTED_FEATURES.values():
    for feature in features:
        VALID_FEATURE_NAMES.add(feature.name)

...

assert type_name in VALID_FEATURE_NAMES

hm, but this won't satisfy mypy, cause mypy can't look into this list. so instead i think we need to hardcode the list, like:

VALID_FEATURE_NAMES = ("os", "arch", ...)
...
assert type_name in VALID_FEATURE_NAMES

the downside is that we need to maintain this list, keeping it up to date whenever we add a new feature. but i dont think that will be common or require much work.

@harshit-wadhwani
Copy link
Author

I will take a look into those errors.

@mr-tz mr-tz added this to the v8.0 milestone Nov 27, 2024
@mr-tz
Copy link
Collaborator

mr-tz commented Nov 27, 2024

@harshit-wadhwani have you had a chance to take a look here, yet?

@harshit-wadhwani
Copy link
Author

harshit-wadhwani commented Nov 29, 2024

I ran pre-commit with dev container and it passes everything, but not on the github actions, any suggestions ?
@mr-tz @williballenthin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants