Fix: Issue #2307

[harshit-wadhwani]

closes #2307

Checklist

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

[github-actions]

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

CHANGELOG updated or no update needed, thanks! 😄

[harshit-wadhwani]

Annotated was introduced in Python 3.9.

[williballenthin]

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

[mr-tz]

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]

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]

I will take a look into those errors.

[mr-tz]

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

[harshit-wadhwani]

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

[mr-tz]

black isn't happy, are you using the same version running in CI (black==24.10.0) and the same config (pre-commit run --all-files black)?

[mr-tz]

use a string here (just for rendering)? (and same in vverbose?)

[mr-tz]

@williballenthin could you take a look if this is what you had in mind to close out the issue, please?

we should then squash and merge this PR

[williballenthin]

lgtm!

[mr-tz]

Thanks, @harshit-wadhwani!