Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

AlexWaygood
Copy link
Member

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 with FuncBase:

mypy/mypy/nodes.py

Lines 528 to 536 in 130e1a4

N.B: Although this has SymbolNode subclasses (FuncDef,
OverloadedFuncDef), avoid calling isinstance(..., FuncBase) on
something that is typed as SymbolNode. This is to work around
mypy bug #3603, in which mypy doesn't understand multiple
inheritance very well, and will assume that a SymbolNode
cannot be a FuncBase.
Instead, test against SYMBOL_FUNCBASE_TYPES, which enumerates
SymbolNode subclasses that are also FuncBase subclasses.

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.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@sobolevn sobolevn left a 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?

@AlexWaygood
Copy link
Member Author

It looks like #3603 was fixed, shouldn't it just work now?

Maybe it should, but it doesn't :) there's still loads of code erroneously marked as unreachable if you run the self-check with --warn-unreachable, and this PR significantly reduces the number of false positives.

Or is it just a new bug / case?

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.

@Michael0x2a
Copy link
Collaborator

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:

  1. Fix the final loop in checker.py's conditional_types_with_intersection so it doesn't give up if the union contains a non-Instance
  2. Instead of using SYMBOL_FUNCBASE_TYPES, make mypy use None a little less frequently/explicitly check for None
  3. Merge this PR after updating the comments explaining why SYMBOL_FUNCBASE_TYPE is a thing

Elaboration on why (3) might be reasonable: the problem with using isinstance(node, FuncBase) and ad-hoc intersections is that it prevents you from further narrowing the type using checks like isinstance(node, FuncDef). Granted, it doesn't look like this is something we're doing now, but it's a potential footgun:

# 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.)

@AlexWaygood AlexWaygood marked this pull request as draft September 4, 2022 16:31
@AlexWaygood
Copy link
Member Author

@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

@AlexWaygood
Copy link
Member Author

Closing this for now

@AlexWaygood AlexWaygood deleted the funcbase-unreachable branch January 26, 2025 09:03
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

Successfully merging this pull request may close these issues.

3 participants