search

intel / cve-bin-tool

The CVE Binary Tool helps you determine if your system includes known vulnerabilities. You can scan binaries for over 200 common, vulnerable components (openssl, libpng, libxml2, expat and others), or if you know the components used, you can get a list of known vulnerabilities associated with an SBOM or a list of components and versions.
https://cve-bin-tool.readthedocs.io/en/latest/
GNU General Public License v3.0
1.17k stars 450 forks source link

Next Steps for Type Checking (discussion) #2316

Open terriko opened 1 year ago

terriko commented 1 year ago

We had lots of new contributors through hacktoberfest thanks to having such easy bugs, and I hope that was fun for everyone! Now that hacktoberfest is done and I'm not so swamped, let's talk about next steps for type checking!

Enabling Automatic Type Checking

I want to be able to enable mypy for type checking with our pre-commit linters. We'd previously looked at pyright but it crapped out on me at an inconvenient time during hacktoberfest so I'm making the executive decision to start with mypy. I'm not adverse to changing tools later, but I am adverse to using something that might stop working and break CI.

If anyone knows how to enable it so that we can exclude some files from checking, we might be able to do this now. otherwise, we can wait until we resolve the remaining issues.

The Type Checking Style Guide / Instructions for beginners

We currently support Python 3.7-3.10 (and I expect we'll enable 3.11 support soon) so we had been using some older types for backwards compatibility reasons. @Molkree kindly pointed out that we could be using from __future__ import annotations and use the new style of type annotations instead. We noodled around with it but it looks like using __future__ is going to be best with our existing pre-commit checks. I want to document that this is the preferred method in our style guide, and make sure we link that from any future (or still open) type checker bugs.

I think our type checking guide also needs some lists of custom types we use (and where to find them in the code) to help people figure that out and not just put Any or assume everything is a str.

Can anyone else think of stuff that need to go in there?

Timing

I've got some much more high-priority stuff that needs to happen before the 3.2 release, so I'm not intending to work on types much until after 3.2 is out. I'll probably set up another big round of easy beginner bugs to upgrade our type checking style and try to have those available to folk when Google Summer of Code interest starts to pick up in 2023. If anyone wants to work on type checking before then, though, feel free. I'm most interested in seeing the "harder" fixes where it's clear the types we chose initially aren't working. e.g. like these ones in output_engine/util.py

cve_bin_tool/output_engine/util.py:40: error: Item "str" of "Union[CVE, str]" has no attribute "severity"
cve_bin_tool/output_engine/util.py:139: error: Item "str" of "Union[CVE, str]" has no attribute "cve_number"
cve_bin_tool/output_engine/util.py:140: error: Item "str" of "Union[CVE, str]" has no attribute "severity"
cve_bin_tool/output_engine/util.py:141: error: Item "str" of "Union[CVE, str]" has no attribute "score"
cve_bin_tool/output_engine/util.py:142: error: Item "str" of "Union[CVE, str]" has no attribute "cvss_version"
cve_bin_tool/output_engine/util.py:143: error: Item "str" of "Union[CVE, str]" has no attribute "cvss_vector"
cve_bin_tool/output_engine/util.py:144: error: Argument 1 to "join" of "str" has incompatible type "Union[List[CVE], Set[str]]"; expected "Iterable[str]"
cve_bin_tool/output_engine/util.py:145: error: Item "str" of "Union[CVE, str]" has no attribute "remarks"
cve_bin_tool/output_engine/util.py:146: error: Item "str" of "Union[CVE, str]" has no attribute "comments"
cve_bin_tool/output_engine/util.py:149: error: Item "str" of "Union[CVE, str]" has no attribute "description"
cve_bin_tool/output_engine/util.py:152: error: Value of type "Optional[Dict[str, VersionInfo]]" is not indexable
cve_bin_tool/output_engine/util.py:152: error: Item "str" of "Union[CVE, str]" has no attribute "cve_number"
cve_bin_tool/output_engine/util.py:196: error: Dict entry 0 has incompatible type "str": "Dict[str, object]"; expected "Dict[str, Union[str, int]]": "List[Dict[str, str]]"
cve_bin_tool/output_engine/util.py:204: error: Dict entry 1 has incompatible type "str": "List[Dict[str, str]]"; expected "Dict[str, Union[str, int]]": "List[Dict[str, str]]"
DangerChamp commented 1 year ago

Hi there. I've looked into the pre-commit hooks for mypy and have found this. Pre-commit hook for mypy I've been able to test it out in the .pre-commit-config.yaml and everything looks fine. It has the option to exclude directories and files. If this is still needed I'll be happy to open a PR.

terriko commented 1 year ago

I think we're about due for an update in here, but it's kind of a non-update:

As you can see from the discussion above, we've got automatic checking on the files that were sufficiently complete.

We still don't have much in the way of a style guide, but we do at least have some links in the contributor docs: https://github.com/intel/cve-bin-tool/blob/main/CONTRIBUTING.md#running-mypy-by-itself

We're still at the point where the remaining mypy issues are mostly "hard" -- new contributors can't usually figure them out, and I don't always know enough about type checking best pratices to usefully guide them during code review. The remaining type checking issue PRs have been languishing a while.

It's been about half a year since I started this thread and I'm going to admit to myself that it's unlikely that I am going to be driving this any further for now.

We'll maintain what we have and try to be good about new code when we can, but I'm probably not going to be filing a bunch of mypy issues for hacktoberfest in the fall. I don't want to say this effort is "dead" so much as "hibernating" but I will say that it's in need of a new champion who isn't me if we want to keep moving through the last hurdles and get ourselves into full compliance.