Bug in __subclasscheck__ causes isinstance() to fail

[JamesGuthrie]

From the discussions in #135 and #136 it looks as though __instancecheck__ and __subclasscheck__ are here to stay for the collections.abc subclasses.
There is a bug in the implementation of __subclasscheck__ for GenericMeta. The following sample code exposes the bug:

from typing import Iterable

assert isinstance([], Iterable)
assert isinstance([], Iterable)

The second call of isinstance([], Iterable) returns False when it should return True.

The reason for this appears to be the an incorrect implementation of __subclasscheck__() for GenericMeta. super().__subclasscheck__(cls) on line 1024 of typing.py results in the list type being placed in the negative cache of typing.Iterable, afterwards issubclass(cls, self.__extra__) evaluates to True, but the negative cache is not invalidated. On the next check of isinstance([], Iterable) the negative cache is consulted and returns False.

One possible fix is to invalidate typing.Iterable's negative cache if issubclass(cls, self.__extra__) evaluates to True, as in the following diff:

diff --git a/prototyping/typing.py b/prototyping/typing.py
index ddaec3e..f576ce1 100644
--- a/prototyping/typing.py
+++ b/prototyping/typing.py
@@ -1025,7 +1025,10 @@ class GenericMeta(TypingMeta, abc.ABCMeta):
             return True
         if self.__extra__ is None or isinstance(cls, GenericMeta):
             return False
-        return issubclass(cls, self.__extra__)
+        if issubclass(cls, self.__extra__):
+            abc.ABCMeta._abc_invalidation_counter += 1
+            return True
+        return False


 class Generic(metaclass=GenericMeta):

If you think my proposed change is good, I'll submit a PR with the change.

[gvanrossum]

Wow, thanks for finding that one! It's pretty terrible. But I don't think your fix is right -- wouldn't it bump the invalidation counter each time __subclasscheck__ is called?

[JamesGuthrie]

Yes you're right, I've taken another look at the code in order to better determine where the fix should be. I assume this issue cropped up when __instancecheck__ was removed from GenericMeta.

The call graph for isinstance([], Iterable) is:

1. ABCMeta.__instancecheck__(cls=typing.Iterable, instance=[])
   Initially the caches aren't populated so cls.__subclasscheck__(<class 'list'>) is called:
2. GenericMeta.__subclasscheck__(self=typing.Iterable, cls=<class 'list'>)
   Next the supertype of typing.Iterable is consulted super().__subclasscheck__(cls)
3. ABCMeta.__subclasscheck__(cls=typing.Iterable, subclass=<class 'list'>)
   Here it is determined that <class 'list'> is not a subclass of typing.Iterable and the negative cache of typing.Iterable is set, returning back to GenericMeta.__subclasscheck__(self=typing.Iterable, cls=<class 'list'>).
   Finally, the check for issubclass(cls, self.__extra__) is done:
4. ABCMeta.__subclasscheck__(cls=collections.abc.Iterable, subclass=<class 'list'>)
   Which returns True.

The next pass through, the first call is ABCMeta.__instancecheck__(cls=typing.Iterable, instance=[]), which has a negative cache set, so it returns False.

I see two issues and potential paths to a fix:

1. Handing control of __instancecheck__ to the superclass is breaking the expected behaviour, __instancecheck__ should be implemented in GenericMeta (if this is the solution, we need to figure out how to reconcile this with Kill __instancecheck__ #135. Maybe by providing an implementation in GenericMeta and overriding that with an exception in the Dict, List etc. subclasses.
2. The logic in GenericMeta.__subclasscheck__ needs to be fixed.

• Reorder the checks, first consulting issubclass(cls, self.__extra__), and only then consulting super().__subclasscheck__(cls). I'm not sure if the ordering matters much here, the test suite passes with that ordering.
• We could fiddle around in typing.Iterable's caches, removing the negative cache entry and putting the correct entry in the cache. This seems awfully messy though.

[gvanrossum]

Thanks so much for the detailed analysis! I'm focusing on this today, to get it in before Larry tags rc3. Alas, none of the alternatives look attractive. I believe that reordering the checks simply postpones the problem (the tests don't have enough coverage for this).

[gvanrossum]

I decided that overriding __instancecheck__ was the right choice. For now there are still some bugs (e.g. isinstance([42], Iterable[str]) returns True) but those aren't regressions, and I ran out of time.