-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Avoid using isinstance()
checks with FuncBase
for type narrowing
#13607
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
Conversation
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not aware of this problem, TIL! Thanks.
It looks like #3603 was fixed, shouldn't it just work now? Or is it just a new bug / case?
Maybe it should, but it doesn't :) there's still loads of code erroneously marked as unreachable if you run the self-check with
Perhaps, I'm not sure. Whatever the case, it seems mypy's reachability analysis could definitely still be improved upon when it comes to multiple inheritance. |
This isn't really a review, more of a context dump, but it looks like the problem has to do with narrowing unions containing None. Minimized repro: class SymbolTableNode: pass
class FuncBase: pass
def bad(node: SymbolTableNode | None) -> None:
assert isinstance(node, FuncBase)
reveal_type(node) # (No output, unreachable)
def good(node: SymbolTableNode) -> None:
assert isinstance(node, FuncBase)
reveal_type(node) # <subclass of SymbolTableNode and FuncBase> So, I think there are three reasonable options here:
Elaboration on why (3) might be reasonable: the problem with using # mypy: warn_unreachable
class SymbolTableNode: pass
class FuncBase: pass
class FuncDef(FuncBase, SymbolTableNode): pass
def awkward(node: SymbolTableNode) -> None:
assert isinstance(node, FuncBase)
reveal_type(node) # <subclass of SymbolTableNode and FuncBase>
assert isinstance(node, FuncDef) # E: Subclass of "SymbolTableNode", "FuncBase", and "FuncDef" cannot exist: would have inconsistent method resolution order
reveal_type(node) # (unreachable) The easiest way of side-stepping this potential footgun would be to just narrow directly to the leaf types. (Though I guess the downside with using SYMBOL_FUNCBASE_TYPE is that these functions will no longer handle LambdaExpr nodes? But maybe that's fine.) |
@Michael0x2a, thanks for the context! I've marked this PR as draft for now — personally, I think option (1) would be the ideal solution. The fundamental problem seems pretty similar to #12802 |
Closing this for now |
This PR improves mypy's understanding of the mypy codebase, and reduces the number of false positives if you run the selfcheck with
--warn-unreachable
.As stated in this docstring here, mypy doesn't understand type narrowing very well if you use an
isinstance()
check withFuncBase
:mypy/mypy/nodes.py
Lines 528 to 536 in 130e1a4
Instead, it tends to view blocks of code beneath an
if isinstance(foo, FuncBase)
guard as unreachable, leading to a lot of false positives if you run the selfcheck with--warn-unreachable
.