Various mypy fixes - #5350
Various mypy fixes#5350
Conversation
b47225a to
80ee3a3
Compare
Remove code for detecting a virtualenv. This has been broken since ~3.7.
Invalid type annotations confuse mypy. When it sees a type annotation that doesn't conflict with other annotations, it is trusted. This means that future attempts to conditionally check a variable's type will cause mypy to produce an unreachable code path warning.
202c0ba to
a3e9409
Compare
Later use of the same variable name leads mypy to report unreachable code paths.
Mypy infers an unannotated variable's type from its initial value. When a variable's type may change (such as from None to non-None), the variable's type needs to be annotated, otherwise mypy may warn of unreachable code paths.
Python's provides type annotation features which allow validating types throughout the code base. Use, rather than ignore, these features. Note that since a dict's .get() may return None, using this method when the only possible return value is a literal will confuse mypy.
Enable positional arguments for various environments.
Add missing deps for tip-mypy.
The -tip envs were broken when a non-tip env existed. In these cases,
{[testenv:mypy]commands} resolved to the non-tip environment. Fix it.
Fix the current network v2 schema as well.
a3e9409 to
c4924a2
Compare
Python's provides type annotation features which allow validating types throughout the code base. Use, rather than ignore, these features. Note that since a dict's .get() may return None, using this method when the only possible return value is a literal will confuse mypy.
Enable positional arguments for various environments.
Add missing deps for tip-mypy.
The -tip envs were broken when a non-tip env existed. In these cases,
{[testenv:mypy]commands} resolved to the non-tip environment. Fix it.
Fix the current network v2 schema as well.
d4ad08f to
e13fbec
Compare
e13fbec to
544754d
Compare
891684b to
c685400
Compare
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
I don't have time to do a full review today, but I realized I have a stale "Pending" comment that is still relevant.
|
|
||
|
|
||
| def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: | ||
| def handle(name: str, cfg: Config, cloud: Cloud, _) -> None: |
There was a problem hiding this comment.
This is part of every single handler...
Let's not update this in this PR. If we do, lets update all handlers in a separate PR. Also, I'd prefer _args: List rather than _. A single underscore doesn't tell you what it's supposed to represent.
There was a problem hiding this comment.
This is part of every single handler...
This is the only one that has an argument name which later has a different type. If you'd rather we didn't fix it this way, then I'll change the shadowed variable name.
There was a problem hiding this comment.
Ah, I see now. I thought you were doing something else. Yeah, I'm ok changing this one here. I think I'd still prefer the name change though.
|
|
||
| # Apply json formatting | ||
| 04c82eafd8fe34c5f02ffecfdb5dba888f3baa3f | ||
| e13fbec78315f535b3fccb22727e5035201b6929 |
There was a problem hiding this comment.
I don't see this commit anywhere.
|
|
||
| @property | ||
| def contents(self) -> bytes: | ||
| if self._response.content is None: |
There was a problem hiding this comment.
Do you know why mypy is yelling about this one? content can definitely be None, so I think the check here is valid.
There was a problem hiding this comment.
I'll need to dig in further about this one
There was a problem hiding this comment.
@TheRealFalcon Thanks for finding that. It turns out that this was due to a bug in typeshed. I just submitted a PR to fix it. I'll change this PR to silence this warning until a fix is released.
|
@TheRealFalcon I think I've addressed all comments. If tests pass, I think this is ready for review. |
| bae9b11da9ed7dd0b16fe5adeaf4774b7cc628cf | ||
|
|
||
| # Apply json formatting | ||
| 04c82eafd8fe34c5f02ffecfdb5dba888f3baa3f |
There was a problem hiding this comment.
These changed again. Think it might make sense to update these after the fact?
| types-setuptools=={[format_deps]types-setuptools} | ||
| typing-extensions=={[format_deps]typing-extensions} | ||
| commands = {envpython} -m mypy cloudinit/ tests/ tools/ | ||
| # TODO: add --check-untyped-defs to allow checking untyped function bodies |
Remove code for detecting a virtualenv. This has been broken since ~3.7.
Invalid type annotations confuse mypy. When it sees a type annotation that doesn't conflict with other annotations, it is trusted. This means that future attempts to conditionally check a variable's type will cause mypy to produce an unreachable code path warning.
Later use of the same variable name leads mypy to report unreachable code paths.
Mypy infers an unannotated variable's type from its initial value. When a variable's type may change (such as from None to non-None), the variable's type needs to be annotated, otherwise mypy may warn of unreachable code paths.
cb493bd to
3c85e40
Compare
Enable mypy's "warn_unreachable" checks. Remove adjacent unused mocks. Note: mypy can incorrectly report typing errors as "unused code paths"
Python's provides type annotation features which allow validating types throughout the code base. Use, rather than ignore, these features. Note that since a dict's .get() may return None, using this method when the only possible return value is a literal will confuse mypy.
Enable positional arguments for various environments.
Add missing deps for tip-mypy.
The -tip envs were broken when a non-tip env existed. In these cases,
{[testenv:mypy]commands} resolved to the non-tip environment. Fix it.
Fix the current network v2 schema as well.
This doesn't add anything useful on its own, but it defines a custom type which can easily be overridden. See canonicalGH-5398 for more details.
|
Looks like the recent addition to mypy features expanded the scope slightly, so I had to update some test code to make it pass. I think I just need to fix the merge conflict before this is ready to merge @TheRealFalcon. |
TheRealFalcon
left a comment
TheRealFalcon
left a comment
There was a problem hiding this comment.
2 questions inline
| self, | ||
| cloud: "IntegrationCloud", | ||
| instance: BaseInstance, | ||
| instance, |
There was a problem hiding this comment.
This was fairly useful when it comes to editor autocomplete. Why does mypy not like it?
There was a problem hiding this comment.
Because instance doesn't contain BaseInstance. It contains a child class of BaseInstance which has other attributes and methods, so when we use those attributes mypy (rightfully) warns of errors like this:
tests/integration_tests/modules/test_combined.py:465: error: "BaseInstance" has no attribute "availability_zone"
tests/integration_tests/modules/test_combined.py:486: error: "BaseInstance" has no attribute "zone"
tests/integration_tests/modules/test_combined.py:487: error: "BaseInstance" has no attribute "instance_id"
We can use type narrowing to preserve autocomplete suggestions while satisfying mypy. I'll cook something up.
| f"Failed to update packages: {response.stderr}" | ||
| ) | ||
|
|
||
| def ip(self) -> str: |
There was a problem hiding this comment.
This is used in __exit__ (though the implementation is buggy). Why is it being removed?
There was a problem hiding this comment.
The whole purpose of that was to to make it easier for someone to ssh into an instance on a public cloud after a test, however since only lxd has execute_via_ssh, its purpose is defeated. I've been meaning to clean this up for a while, but hadn't gotten to it yet. Mypy is complaining about the types in this code which is why I finally made the change.
There was a problem hiding this comment.
So can we fix it instead of dropping it? It's valid if we don't want to fix it here, but we can use the # typing: ignore directives in that case.
There was a problem hiding this comment.
Sure, that's fair. Looking at this again I think the fix might be easier than I previously thought.
Remove code for detecting a virtualenv. This has been broken since ~3.7.
Invalid type annotations confuse mypy. When it sees a type annotation that doesn't conflict with other annotations, it is trusted. This means that future attempts to conditionally check a variable's type will cause mypy to produce an unreachable code path warning.
Later use of the same variable name leads mypy to report unreachable code paths.
Mypy infers an unannotated variable's type from its initial value. When a variable's type may change (such as from None to non-None), the variable's type needs to be annotated, otherwise mypy may warn of unreachable code paths.
Enable mypy's "warn_unreachable" checks. Remove adjacent unused mocks. Note: mypy can incorrectly report typing errors as "unused code paths"
Python's provides type annotation features which allow validating types throughout the code base. Use, rather than ignore, these features. Note that since a dict's .get() may return None, using this method when the only possible return value is a literal will confuse mypy.
Enable positional arguments for various environments.
Add missing deps for tip-mypy.
The -tip envs were broken when a non-tip env existed. In these cases,
{[testenv:mypy]commands} resolved to the non-tip environment. Fix it.
Fix the current network v2 schema as well.
This doesn't add anything useful on its own, but it defines a custom type which can easily be overridden. See canonicalGH-5398 for more details.
This also satisfies mypy via type narrowing, which hasattr() does not accomplish.
|
@TheRealFalcon Just force pushed to fix the merge conflict, and while I was at it I squashed the fixup commits and updated commit messages with PR number. Assuming tests pass this one is ready for re-review. |
2 participants
Additional Context
See individual commits.
This work enables using more of mypy's static analysis features.
This PR does various cleanups including:
Note to reviewers:
Most of the changes here are no-ops. That said, please pay special attention to the commit titled
refactor(typing): Remove unused code paths. Invalid type annotations can lead mypy to warn incorrectly that a code path is unused (this is how I found many of the type errors in this commit series). I do believe that I was thorough in verifying this commit, but this one carries the most risk of the series.