Check for compatibility when inheriting from multiple classes for fields

[fangyi-zhou]

This diff is an attempt to address the issue in #943.

When a class inherits from multiple base classes, we check whether the intersection of the field types is never. If so, we raise an error.

Next step: Handle method overrides / overloads.

[connernilsen]

Hey @fangyi-zhou, thanks for working on this!

It looks like there are a few broken tests, would you be able to take a look at those? In the meantime, I'll tag @stroxler and @yangdanny97, since they were active on the initial issue.

[fangyi-zhou]

It looks like there are a few broken tests, would you be able to take a look at those?

This is the tricky part (and hence why I marked this diff as RFC). This change surfaces some issues with standard library in vendored typeshed.

Error messages are re-formatted for readability

(1) There are some issues with name mismatches in importlib. I raised a PR to typeshed to reconcile the differences python/typeshed#14809. These errors are due to we require parameter names in function to match when subtyping.

ERROR _frozen_importlib_external.pyi:132:7-27: Inconsistent types for field `load_module` inherited from multiple base classes: 
 `(self: Self@FileLoader, name: str | None = None) -> ModuleType` from `FileLoader`, 
 `(self: Self@_LoaderBasics, fullname: str) -> ModuleType` from `_LoaderBasics` [inconsistent-overload]
ERROR _frozen_importlib_external.pyi:136:7-26: Inconsistent types for field `load_module` inherited from multiple base classes: 
 `(self: Self@FileLoader, name: str | None = None) -> ModuleType` from `FileLoader`, 
 `(self: Self@_LoaderBasics, fullname: str) -> ModuleType` from `_LoaderBasics` [inconsistent-overload]
ERROR _frozen_importlib_external.pyi:188:11-31: Inconsistent types for field `__init__` inherited from multiple base classes: 
 `(self: Self@ExtensionFileLoader, name: str, path: str) -> None` from `ExtensionFileLoader`, 
 `(self: Self@FileLoader, fullname: str, path: str) -> None` from `FileLoader` [inconsistent-overload]
ERROR _frozen_importlib_external.pyi:188:11-31: Inconsistent types for field `load_module` inherited from multiple base classes: 
 `(self: Self@FileLoader, name: str | None = None) -> ModuleType` from `FileLoader`, 
 `(self: Self@_LoaderBasics, fullname: str) -> ModuleType` from `_LoaderBasics` [inconsistent-overload]

(2) There are some issues with subtyping generic function (?)

ERROR typing.pyi:583:7-25: Inconsistent types for field `__next__` inherited from multiple base classes: 
 `(self: Self@Generator) -> _YieldT_co` from `Generator`, 
 `(self: Self@Iterator) -> _T_co` from `Iterator` [inconsistent-overload]
ERROR typing.pyi:583:7-25: Inconsistent types for field `__iter__` inherited from multiple base classes: 
 `(self: Self@Generator) -> Generator[_YieldT_co, _SendT_contra, _ReturnT_co]` from `Generator`, 
 `(self: Self@Iterator) -> Iterator[_T_co]` from `Iterator`, 
 `(self: Self@Iterable) -> Iterator[_T_co]` from `Iterable` [inconsistent-overload]

(3) Handling overloads (?)

ERROR _typeshed/__init__.pyi:294:7-28: Inconsistent types for field `__getitem__` inherited from multiple base classes: 
 `(self: Self@SliceableBuffer, slice: slice[Any, Any, Any], /) -> Sequence[int]` from `SliceableBuffer`, 
 `(self: Self@IndexableBuffer, i: int, /) -> int` from `IndexableBuffer` [inconsistent-overload]

[yangdanny97]

Nice work on the typeshed PR!

The overall approach makes sense, I'll do a more detailed review soon.

Our intersection operation is a bit shaky, so I'm not surprised if there's a bug or two buried there that affects this PR. If it turns out to be very difficult to resolve, we could merge this with the check restricted to TypedDicts (or restricted to non-function types), and iterate till we eventually can run this for everything.

[yangdanny97]

we might need a new error code here

[yangdanny97]

we can split this into multiple lines,

pyrefly/pyrefly/lib/error/collector.rs

Line 121 in fb552b7

pub fn add(&self, range: TextRange, info: ErrorInfo, mut msg: Vec1<String>) {

takes a vec1 of lines

[yangdanny97]

Is there some extra work being done here? For example given the class hierarchy

class A:
  x: T1
class B:
  x: T2
class C(A, B):
  x: T3
class D:
  x: T4
class E(C, D): ...

Also, when doing the check for E, we're also checking whether A and B's impls are compatible with each other, but that should have already been checked and the error raised in C's definition, so we'd be raising an extra error.

Maybe that's not a bad thing, it does affect E as well, but just something to consider.

Given this snippet

class A:
  x: int
class B:
  x: str
class C(A, B): pass
class D:
  x: int
class E(C, D): pass

mypy errors on both C and E, pyright only errors on C

This logic also doesn't seem to account for overrides. For example,

class A:
  x: int
class B:
  x: str
class C(A, B):
  x: int
class D:
  x: int
class E(C, D): pass

Here we would check the impl of x from A and B, even though they're both overridden by the impl from C. Mypy only errors on C in this case, but I suspect we would error on E as well.

I wonder if this means we could get away with looking up a single copy of the field from each base class, using the MRO. That would be fewer things to check than looking up every occurrence of that field from anywhere in the class hierarchy.

[yangdanny97]

For the 3rd example you gave

ERROR _typeshed/__init__.pyi:294:7-28: Inconsistent types for field `__getitem__` inherited from multiple base classes: 
 `(self: Self@SliceableBuffer, slice: slice[Any, Any, Any], /) -> Sequence[int]` from `SliceableBuffer`, 
 `(self: Self@IndexableBuffer, i: int, /) -> int` from `IndexableBuffer` [inconsistent-overload]

it's from here

https://github.com/python/typeshed/blob/bee1e1f551c1c4b74b1cbb250ffd1152290b40a5/stdlib/_typeshed/__init__.pyi#L305

The parent implementations are definitely not compatible with each other, but as long as the child's overload is compatible with both I think it's fine.

I'm not sure what the fix here should be - do we skip methods entirely? (that seems excessive) or could we make an intersection of two incompatible methods generate an overload with both signatures?

[yangdanny97]

For the second issue you raised, maybe @samwgoldman has an opinion on this.

Maybe we need to do some sort of type var substitution (replacing all the parent class's type vars with the child class's type vars so that these checks are all using the same type vars).

[fangyi-zhou]

I'm made some changes according to the comments, but this stack is not ready for another review yet. I'll investigate another day how to deal with the overloads.

[fangyi-zhou]

ERROR _frozen_importlib_external.pyi:188:11-31: Inconsistent types for field __init__ inherited from multiple base classes:
(self: Self@ExtensionFileLoader, name: str, path: str) -> None from ExtensionFileLoader,
(self: Self@FileLoader, fullname: str, path: str) -> None from FileLoader [inconsistent-overload]

This seems to be a problem in the standard library.

For FileLoader:
https://github.com/python/cpython/blob/d1ca001d357400d3f1f64e7fa48ace99a59c558f/Lib/importlib/_bootstrap_external.py#L921

For ExtensionFileLoader:
https://github.com/python/cpython/blob/d1ca001d357400d3f1f64e7fa48ace99a59c558f/Lib/importlib/_bootstrap_external.py#L1044

Maybe we should exclude constructors from this check?

[yangdanny97]

If the overloads ends up being tricky or affecting a lot of other behaviors, we could merge a version of this that excludes overloads & implkement that part separately.

Re: the constructor stuff, i think that makes sense to skip it. Oftentimes classes will override constructors with completely different signatures, and our override consistency check also skips it. The override consistency check implementation may be a useful reference for what should/should not be skipped for this analysis

https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs#L1751

[fangyi-zhou]

Let's get the field check merged in first, and deal with methods in a follow up diff.