Avoid using isinstance() checks with FuncBase for type narrowing

[AlexWaygood]

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]

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

[sobolevn]

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]

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]

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]

@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]

Closing this for now