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

Enable strict_optional in mypy config #3317

Open
dae opened this issue Jul 22, 2024 · 2 comments
Open

Enable strict_optional in mypy config #3317

dae opened this issue Jul 22, 2024 · 2 comments

Comments

@dae
Copy link
Member

dae commented Jul 22, 2024

Our mypy configuration uses no_strict_optional = true. This tells mypy to assume nullable variables will not be none, so code like the following will pass a check, and fail at runtime instead:

foo: int | None = None
foo + 5

If we adjust the mypy config, we can get it to throw errors instead:

diff --git a/.mypy.ini b/.mypy.ini
index ef32ed7a2..1207b6aa3 100644
--- a/.mypy.ini
+++ b/.mypy.ini
@@ -22,6 +22,7 @@ exclude = (qt/bundle/PyOxidizer|pylib/anki/_vendor)
 
 [mypy-anki.*]
 disallow_untyped_defs = True
+no_strict_optional = false
 [mypy-anki.importing.*]
 disallow_untyped_defs = False
 [mypy-anki.exporting]

There's quite a few in pylib that need correcting, e.g.

pylib/anki/importing/anki2.py:316: error: Item "None" of "Optional[DBProxy]" has no attribute "execute"  [union-attr]
pylib/anki/importing/csvfile.py:114: error: Value of type "Union[str, list[str], None]" is not indexable  [index]
pylib/anki/importing/csvfile.py:116: error: Value of type "Union[str, list[str], None]" is not indexable  [index]
pylib/anki/importing/csvfile.py:118: error: Value of type "Union[str, list[str], None]" is not indexable  [index]
pylib/anki/importing/csvfile.py:122: error: Argument 1 to "reader" has incompatible type "Union[str, list[str], None]"; expected "Iterable[str]"  [arg-type]
pylib/anki/importing/apkg.py:62: error: Item "None" of "Optional[ZipFile]" has no attribute "read"  [union-attr]
Found 181 errors in 24 files (checked 336 source files)

The first error is caused by doing 'col.db.execute()' when col.db is DBProxy | None. Adding an assert col.db will tell mypy that we are confident db will not be null in that case, and the error will go away. In other cases where we're not 100% sure if the value will be valid, the routine should check for null / exit early / etc.

The work on this can be done incrementally, as you can enable no_strict_optional for individual submodules at a time (e.g. pylib.importing).

Many of the errors are in legacy code (old stats, old importing code) that will eventually be removed. In such cases, sprinkling asserts above the problem lines is probably all that is required. Our goal here is not really to change the way the errors are handled at run time. The goal is to get our code working with the new setting, so we can benefit from its increased protection in other parts of the codebase.

Another upside of doing this is that pyright (the type checker used by VS Code) does not support no_strict_optional. That causes our current codebase to show a lot errors when checked with pyright.

I suspect sorting out pylib first will bring the most value. aqt would benefit from being tackled too in the future, but there are 1000+ errors at the moment. Sometimes a single assert at the top of a function will fix a bunch of errors at once, as all the subsequent code can assume some variable is non-null. So the total issues to fix are likely smaller than that. But better to start with the low-hanging fruit.

@davidculley
Copy link
Contributor

I'll gladly look into it. 🙂

@davidculley
Copy link
Contributor

I've begun with the pylib directory (see #3355).

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

No branches or pull requests

2 participants