• xarray: was a false positive,Incompatible definition of generic field on multiple base classes #9031
• pymongo: don't see what's wrong there, but looks unrelated?
• steam: weird and unsafe, done for runtime hints usage.
• ibis: good (inheritsclass FrozenDict(dict, Mapping[K, V]) - dict is missing type params, so should be compatible)
• homeassistant: very long hierarchy, but looks correct: the default for the omitted typevar isDataUpdateCoordinator[dict[str, Any]], coordinator type args are incompatible
• kornia: correct, silencing transitive errors

Diff frommypy_primer, showing the effect of this PR on open source code:

xarray (https://github.com/pydata/xarray)+ xarray/core/groupby.py:1610: error: Unused "type: ignore" comment  [unused-ignore]+ xarray/core/groupby.py:1778: error: Unused "type: ignore" comment  [unused-ignore]+ xarray/core/resample.py:236: error: Unused "type: ignore" comment  [unused-ignore]+ xarray/core/resample.py:379: error: Unused "type: ignore" comment  [unused-ignore]mongo-python-driver (https://github.com/mongodb/mongo-python-driver)+ pymongo/helpers_shared.py:131: error: Unused "type: ignore" comment  [unused-ignore]steam.py (https://github.com/Gobot1234/steam.py)+ steam/ext/commands/converters.py:273: error: Definition of "converter_for" in base class "Converter" is incompatible with definition in base class "Converter"  [misc]+ steam/ext/commands/converters.py:294: error: Definition of "converter_for" in base class "Converter" is incompatible with definition in base class "Converter"  [misc]+ steam/ext/commands/converters.py:302: error: Definition of "converter_for" in base class "Converter" is incompatible with definition in base class "Converter"  [misc]+ steam/ext/commands/converters.py:389: error: Definition of "converter_for" in base class "Converter" is incompatible with definition in base class "Converter"  [misc]ibis (https://github.com/ibis-project/ibis)- ibis/common/collections.py:280: error: Definition of "get" in base class "Mapping" is incompatible with definition in base class "Mapping"  [misc]core (https://github.com/home-assistant/core)+ homeassistant/components/ring/entity.py:176: error: Definition of "coordinator" in base class "BaseCoordinatorEntity" is incompatible with definition in base class "BaseCoordinatorEntity"  [misc]+ homeassistant/components/tesla_fleet/entity.py:157: error: Definition of "coordinator" in base class "BaseCoordinatorEntity" is incompatible with definition in base class "BaseCoordinatorEntity"  [misc]+ homeassistant/components/tomorrowio/weather.py:97: error: Definition of "coordinator" in base class "BaseCoordinatorEntity" is incompatible with definition in base class "BaseCoordinatorEntity"  [misc]kornia (https://github.com/kornia/kornia)+ kornia/augmentation/_3d/mix/transplantation.py:7: error: Unused "type: ignore" comment  [unused-ignore]

Yes, I am very much interested in this topic (i.e. issues similar to#6184). I vaguely remember that last time I thought about this it seemed the best way to address all this is check derivation paths compatibility (seemap_instance_to_supertypes()). I will try to take a proper look on weekend (or maybe early next week).

@ilevkivskyi just a gentle ping if you forgot about this:)

Hi! No, I didn't forget to look at it, but just finally found energy to post my findings. Looking at the tests and issues closed, it seems like you are trying to solve three separate problems with multiple inheritance in this PR:

1. Only the most derived definition of an attribute should be checked to be a subtype w.r.t. to definitions in further MRO entries,not each definitions pair in the MRO.
2. Map/expand step is missing when comparing attributes with non-callable types.
3. Mypy has various inconsistent behaviours when a generic class (sayB) comes to the MRO from two bases with different parameters (sayB[int] andB[str]).

While first two are just simple obvious bugs, the last one is actually a non-trivial thing and any attempts towards it should be excluded from this PR. Now my general comments on the first two issues:

1. I think you can simplify your implementation and make the logic more obvious, if instead of usingseen_names (which hints at some kind of caching or similar) you will:
   • have one (outer) loop over attribute name
   • find the most derived definition for given attribute, if it is in current class, skip this attribute
   • in the second (inner) loop go over classes that appear after the index where you found most derive definition
2. I think you can simply add missingmap_type_from_supertype() call in the non-callable branch (the one that starts withelif first_type and second_type:), seebind_and_map_method() for how to call it. All the "typed MRO" stuff is not needed for this (and I am not even sure it is correct).

After you will update the PR accordingly, I will look again and leave more detailed comments (if needed).

Thank you for this thorough review,@ilevkivskyi!

I must confess I hardly understand some of your points. This PR is trying to solve one primary problem. The problem is: when checking compatibility of parents in a multiple inheritance scenario,mypy fails to understand the relationship between generics in those. This issue is related to yourproblem 3, but doesn't directly correspond to it.

I agree that problem 1 is solved here (seen_names check).

On the other hand, problem 2 just ceases to exist in the proposed implementation. I'm reasonably confident that simplifying some fragile checks by obtaining already parametrized callables from corresponding base classes is somewhat more obviously correct.

#18268 and#9031 share a common source:mypy considers baseTypeInfos (MRO entries) instead of instances with appropriately mapped generics. "Typed MRO" approach I propose fixes exactly that: we just build an MRO-like structure with all generics propagated from current definition.#14279 and#9319 are indeed both caused by theproblem 1 from your list.

So yes, now there are two problems solved here: generics mapping and avoiding cross-checks for each pair of MRO entries. If you'd prefer, I can submit the second part as a separate PR, leaving only the "typed MRO" here and removingseen_names check.

Could you explain a bit further your concerns regarding using this "typed MRO" structure? It looks like a very obvious solution to me, so I would really prefer to stick to it unless you see some hidden deficiencies I'm missing.

Regarding your specific suggestions, no. 1 turned out to be more complicated and less efficient (because we have to build a list of all attributes first, and then traverse the MRO or typed MRO again to find the source of that name).seen_names contains all attributes that were already checked against - if any of preceding bases defines that name, all following bases don't have to define it compatibly with each other). No. 2 assumes (AFAIC) that we ditch the "typed MRO" which I'd prefer to keep.

The problem is: when checking compatibility of parents in a multiple inheritance scenario, mypy fails to understand the relationship between generics in those.

The real problem is that this statement is factually wrong. There is nothing fundamental missing in mypy. As I already explained, mypy, as written now, only applies the correct map/expand steps when comparing attribute types only when those types are callable types. For example, your testtestMultipleInheritanceCompatibleTypeVar already passes on master (modulo error message order) if I replacex: T withx: Callable[[], T] everywhere.

No. 2 assumes (AFAIC) that we ditch the "typed MRO" which I'd prefer to keep.

Yes, we don't need all that. Because in best case it would be re-inventing the wheel (and I don't think it is even correct as written). This whole generic handling problem can be fixed with just a two-line diff:

         elif first_type and second_type:             if isinstance(first.node, Var):+                first_type = map_type_from_supertype(first_type, ctx, base1)                 first_type = expand_self_type(first.node, first_type, fill_typevars(ctx))             if isinstance(second.node, Var):+                second_type = map_type_from_supertype(second_type, ctx, base2)                 second_type = expand_self_type(second.node, second_type, fill_typevars(ctx))             ok = is_equivalent(first_type, second_type)

Regarding your specific suggestions, no. 1 turned out to be more complicated and less efficient

I doubt there will be any measurable difference. Also premature optimization is the root of all evil. To be clear, I mean something as simple as this:

all_names= {nameforbaseinmrofornameinbase.names}fornameinall_names-typ.names.keys():ifis_private(name):continuei,base=next((i,base)fori,baseinenumerate(mro)ifnameinbase.names)forbase2inmro[i+1 :]:ifnameinbase2.namesandbase2notinbase.mro:self.check_compatibility(name,base,base2,typ)

which is a double-nested loop instead of a triple-nested loop, and IMO reads self-explanatory.

Wow, your first paragraph was the missing bit, thanks! I get your point now.

This approach is similar to my very first attempt that didn't make it into this PR. I replaced it with current approach for the following reason:

fromcollections.abcimportIterable,MappingfromtypingimportTypeVar_T=TypeVar("_T")_U=TypeVar("_U")classBad(Mapping[_T,int],Iterable[_U]):pass

This snippet should obviously (? perhaps I'm misunderstanding something again?) generate a diagnostic as_T and_U are unrelated, and__iter__ isn't overridden. This snippet is accepted by currentmypy master and can't be rejected by any algorithm relying on MRO alone -Iterable is present in MRO only once, and so will be parameterized with the same type.__iter__ is only defined inIterable, so won't be checked as aMapping attribute, and then we won't attempt to compareIterable[_T] vsIterable[_U] since both map to a singleIterable TypeInfo in the MRO.

There's an old testcase testSubtypingMappingUnpacking3 commenting that behaviour, but no explanation is provided there. That case was added in#11151. This applies to any scenario when impossible parameter combination is requested for derived and parent class, both included in bases.

To be clear, I mean something as simple as this:

And that's nice, thanks! This does read better indeed.And it's true thatis_subtype was an overkill here, MRO check should suffice (not MRO exactly - only "typed MRO" again, otherwise incompatible type parameters won't be detected again).

Wrapping up

To avoid any heated discussions here, I'm ready to close this PR and open a replacement according to your suggestions above, but I still believe that existing MRO-based checks are inferior to traversing the "typed MRO" (essentially a copy of MRO with generic entries duplicated with different params from their corresponding bases) as they fail to detect a typing error by design.

I do trust your maintainer's knowledge and expertise and don't question them, but sometimes being focused on a problem for a long time can make one's judgement slightly biased. My code is not my child, I don't mind iterating and throwing away ideas proven unsuccessful, and I'm really committed to improvingmypy in whatever way I can. Please consider asking someone else to compare both of the proposed solutions if possible, or at least coming back to my proposal in a day or two with a fresh pair of eyes. Thank you for your time! Please ping here if you insist on proceeding with your proposal, I'll open a superseding PR with that.

I replaced it with current approach for the following reason: [...]

But this is exactly the problem 3 that I mentioned above. (Which is a trickier problem than you think and should not be attempted here)

and can't be rejected by any algorithm relying on MRO alone

This is again not true, for abase in MRO fortyp simply writemap_instance_to_supertypes(fill_typevars(typ), base) (note plural in the name) and you will get a list like[typing.Iterable[_T], typing.Iterable[_U]] in your example.

I'm ready to close this PR and open a replacement

I don't really care whether you open a new PR of clean-up this one, as soon as my comments are implemented.