Handle typing.Generic case in MRO

README.rst

@@ -74,6 +74,10 @@ in development
 ^^^^^^^^^^^^^^
 
 * Drop support for Python 3.8.
+* Fix a bug in the MRO computing code that would result in an incorrect  
+  ``Cannot compute linearization of the class inheritance hierarchy`` message 
+  for valid types extending ``typing.Generic`` as well as other generic classes.
+
 
 pydoctor 24.11.1
 ^^^^^^^^^^^^^^^^

pydoctor/model.py

@@ -20,9 +20,10 @@
 from inspect import signature, Signature
 from pathlib import Path
 from typing import (
-    TYPE_CHECKING, Any, Collection, Dict, Iterator, List, Mapping, Callable, 
+    TYPE_CHECKING, Any, Collection, Dict, Iterable, Iterator, List, Mapping, Callable, 
     Optional, Sequence, Set, Tuple, Type, TypeVar, Union, cast, overload
 )
+from graphlib import TopologicalSorter
 from urllib.parse import quote
 
 import attr
@@ -33,12 +34,13 @@
 from pydoctor.sphinx import CacheT, SphinxInventory
 
 if TYPE_CHECKING:
-    from typing_extensions import Literal, Protocol
+    from typing import Literal, Protocol, TypeAlias
     from pydoctor.astbuilder import ASTBuilder, DocumentableT
 else:
     Literal = {True: bool, False: bool}
     ASTBuilder = Protocol = object
 
+T = TypeVar('T')
 
 # originally when I started to write pydoctor I had this idea of a big
 # tree of Documentables arranged in an almost arbitrary tree.
@@ -575,21 +577,36 @@
             return True
     return False
 
-def compute_mro(cls:'Class') -> Sequence[Union['Class', str]]:
+def topsort(graph: Mapping[Any, Sequence[T]]) -> Iterable[T]:
+    """
+    Given a mapping where each keys corespond to a node 
+    and keys the predecessors of the node, return the topological order of the nodes.
+
+    This is a simpple wrapper for L{graphlib.TopologicalSorter.static_order}.
+    """
+    return TopologicalSorter(graph).static_order()
+
+ClassOrStr: TypeAlias = 'Class | str'
+
+class ClassHierarchyFinalizer:
     """
-    Compute the method resolution order for this class.
-    This function will also set the 
-    C{_finalbaseobjects} and C{_finalbases} attributes on 
-    this class and all it's superclasses.
+    Encapsulate code related to class hierarchies post-processing.
     """
-    def init_finalbaseobjects(o: 'Class', path:Optional[List['Class']]=None) -> None:
+
+    @staticmethod
+    def _init_finalbaseobjects(o: Class, path:list[Class] | None = None) -> None:
+        """
+        The base objects are computed in two passes, first the ast visitor sets C{_initialbaseobjects}, 
+        then we set C{_finalbaseobjects} from this function which should be called during post-processing.
+        """
         if not path:
             path = []
         if o in path:
-            cycle_str = " -> ".join([o.fullName() for o in path[path.index(cls):] + [cls]])
+            cycle_str = " -> ".join([c.fullName() for c in path[path.index(o):] + [o]])
             raise ValueError(f"Cycle found while computing inheritance hierarchy: {cycle_str}")
         path.append(o)
         if o._finalbaseobjects is not None:
+            # we already computed these, so skip.
             return
         if o.rawbases:
             finalbaseobjects: List[Optional[Class]] = []
@@ -611,27 +628,89 @@
                         finalbases.append(o._initialbases[i])
                 if base:
                     # Recurse on super classes
-                    init_finalbaseobjects(base, path.copy())
+                    ClassHierarchyFinalizer._init_finalbaseobjects(base, path.copy())
             o._finalbaseobjects = finalbaseobjects
             o._finalbases = finalbases
-
-    def localbases(o:'Class') -> Iterator[Union['Class', str]]:
+
+    @staticmethod
+    def _getbases(o: Class) -> Iterator[ClassOrStr]:
         """
-        Like L{Class.baseobjects} but fallback to the expanded name if the base is not resolved to a L{Class} object.
+        Like L{Class.baseobjects} but fallback to the expanded 
+        name if the base is not resolved to a L{Class} object.
         """
         for s,b in zip(o.bases, o.baseobjects):
             if isinstance(b, Class):
                 yield b
             else:
                 yield s
+
+    def __init__(self, classes: Iterable[Class]) -> None:
+        # this calls _init_finalbaseobjects for every class and 
+        # create the graph object for the ones that did not raised
+        # a cycle-error.
+        self.graph: dict[Class, list[ClassOrStr]]  = {}
+        self.computed_mros: dict[ClassOrStr, list[ClassOrStr]] = {}
+
+        for cls in classes:
+            try:
+                self._init_finalbaseobjects(cls)
+            except ValueError as e:
+                # Set the MRO right away in case of cycles. 
+                # They should not be in the graph though!
+                cls.report(str(e), 'mro')
+                self.computed_mros[cls] = cls._mro = list(cls.allbases(True))
+            else:
+                self.graph[cls] = bases = []
+                for b in self._getbases(cls):
+                    bases.append(b)
+                    # strings are not part of the graph
+                    # because they always have the same MRO: empty list.
+
+    def compute_mros(self) -> None:
+
+        # If this raises a CycleError, our code is boggus since we already
+        # checked for cycles ourself.
+        static_order = topsort(self.graph)
+
+        for cls in static_order:
+            if cls in self.computed_mros or isinstance(cls, str):
+                # If it's already computed, it means it's boggus like with cycle or something.
+                continue
+            self.computed_mros[cls] = cls._mro = self._compute_mro(cls)
 
-    def getbases(o:Union['Class', str]) -> List[Union['Class', str]]:
-        if isinstance(o, str):
-            return []
-        return list(localbases(o))
+    def _compute_mro(self, cls: Class) -> list[ClassOrStr]:
+        """
+        Compute the method resolution order for this class.
+        This assumes that the MRO of the bases of the class 
+        have already been computed and stored in C{self.computed_mros}.
+        """
+        if cls not in self.graph:
+            raise ValueError
+
+        result: list[ClassOrStr] = [cls]
 
-    init_finalbaseobjects(cls)
-    return mro.mro(cls, getbases)
+        if not (bases:=self.graph[cls]):
+            return result
+
+        # since we compute all MRO in topological order, we can safely assume
+        # that self.computed_mros contains all the MROs of the bases of this class.
+        bases_mros = [self.computed_mros.get(kls, []) for kls in bases]
+
+        # handle multiple typing.Generic case, 
+        # see https://github.com/twisted/pydoctor/issues/846.
+        # support documenting typing.py module by using allobject.get.
+        generic = cls.system.allobjects.get(_d:='typing.Generic', _d)
+        if generic in bases and any(generic in _mro for _mro in bases_mros):
+            # this is safe since we checked 'generic in bases'.
+            bases.remove(generic) # type: ignore[arg-type]
+
+        try:
+            return result + mro.c3_merge(*bases_mros, bases)
+
+        except ValueError as e:
+            cls.report(f'{e} of {cls.fullName()!r}', 'mro')
+            return list(cls.allbases(True))
+
 
 def _find_dunder_constructor(cls:'Class') -> Optional['Function']:
     """
@@ -691,7 +770,7 @@
     # set in post-processing:
     _finalbaseobjects: Optional[List[Optional['Class']]] = None 
     _finalbases: Optional[List[str]] = None
-    _mro: Optional[Sequence[Union['Class', str]]] = None
+    _mro: Optional[Sequence[Class | str]] = None
 
     def setup(self) -> None:
         super().setup()
@@ -701,21 +780,11 @@
         self._initialbases: List[str] = []
         self._initialbaseobjects: List[Optional['Class']] = []
 
-    def _init_mro(self) -> None:
-        """
-        Compute the correct value of the method resolution order returned by L{mro()}.
-        """
-        try:
-            self._mro = compute_mro(self)
-        except ValueError as e:
-            self.report(str(e), 'mro')
-            self._mro = list(self.allbases(True))
-
     @overload
-    def mro(self, include_external:'Literal[True]', include_self:bool=True) -> Sequence[Union['Class', str]]:...
+    def mro(self, include_external:'Literal[True]', include_self:bool=True) -> Sequence[Class | str]:...
     @overload
     def mro(self, include_external:'Literal[False]'=False, include_self:bool=True) -> Sequence['Class']:...
-    def mro(self, include_external:bool=False, include_self:bool=True) -> Sequence[Union['Class', str]]:
+    def mro(self, include_external:bool=False, include_self:bool=True) -> Sequence[Class | str]:
         """
         Get the method resution order of this class. 
 
@@ -886,8 +955,6 @@
 _ModuleT = Module
 _PackageT = Package
 
-T = TypeVar('T')
-
 def import_mod_from_file_location(module_full_name:str, path: Path) -> types.ModuleType:
     spec = importlib.util.spec_from_file_location(module_full_name, path)
     if spec is None: 
@@ -1495,10 +1562,15 @@
             self.intersphinx.update(cache, url)
 
 def defaultPostProcess(system:'System') -> None:
-    for cls in system.objectsOfType(Class):
-        # Initiate the MROs
-        cls._init_mro()
 
+    # Init class graph and final bases.
+    class_finalizer = ClassHierarchyFinalizer(
+        system.objectsOfType(Class))
+
+    # Compute MROs
+    class_finalizer.compute_mros()
+
+    for cls in system.objectsOfType(Class):
         # Compute subclasses
         for b in cls.baseobjects:
             if b is not None:

pydoctor/mro.py

@@ -27,7 +27,7 @@
 
 from collections import deque
 from itertools import islice
-from typing import Callable, List, Tuple, Optional, TypeVar
+from typing import List, Tuple, Optional, TypeVar
 
 T = TypeVar('T')
 
@@ -103,7 +103,7 @@ def remove(self, item: Optional[T]) -> None:
                 i.popleft()
 
 
-def _merge(*lists) -> list:
+def c3_merge(*lists) -> list:
     result: List[Optional[T]] = []
     linearizations = DependencyList(*lists)
 
@@ -123,14 +123,3 @@ def _merge(*lists) -> list:
             # Loop never broke, no linearization could possibly be found
             raise ValueError('Cannot compute linearization of the class inheritance hierarchy')
 
-
-def mro(cls: T, getbases: Callable[[T], List[T]]) -> List[T]:
-    """
-    Return a list of classes in order corresponding to Python's MRO.
-    """
-    result = [cls]
-
-    if not getbases(cls):
-        return result
-    else:
-        return result + _merge(*[mro(kls, getbases) for kls in getbases(cls)], getbases(cls))

pydoctor/test/test_mro.py

@@ -1,5 +1,4 @@
 from typing import List, Optional, Type
-import pytest
 
 from pydoctor import model, stanutils
 from pydoctor.templatewriter import pages, util
@@ -11,7 +10,7 @@ def assert_mro_equals(klass: Optional[model.Documentable], expected_mro: List[st
     assert [member.fullName() if isinstance(member, model.Documentable) else member for member in klass.mro(True)] == expected_mro
 
 @systemcls_param
-def test_mro(systemcls: Type[model.System],) -> None:
+def test_mro(systemcls: Type[model.System], capsys:CapSys) -> None:
     mod = fromText("""\
     from mod import External
     class C: pass
@@ -111,12 +110,11 @@ class GenericPedalo(MyGeneric[ast.AST], Pedalo):...
         'mro.WheelBoat',
         'mro.Boat'])
 
-    with pytest.raises(ValueError, match="Cannot compute linearization"):
-        model.compute_mro(mod.contents["F1"]) # type:ignore
-    with pytest.raises(ValueError, match="Cannot compute linearization"):
-        model.compute_mro(mod.contents["G1"]) # type:ignore
-    with pytest.raises(ValueError, match="Cannot compute linearization"):
-        model.compute_mro(mod.contents["Duplicates"]) # type:ignore
+    assert capsys.readouterr().out == '''mro:31: Cannot compute linearization of the class inheritance hierarchy of 'mro.Duplicates'
+mro:9: Cannot compute linearization of the class inheritance hierarchy of 'mro.F1'
+mro:10: Cannot compute linearization of the class inheritance hierarchy of 'mro.G1'
+'''
+
 
 def test_mro_cycle(capsys:CapSys) -> None:
     fromText("""\
@@ -130,6 +128,76 @@ class D(C):...
 cycle:4: Cycle found while computing inheritance hierarchy: cycle.D -> cycle.C -> cycle.A -> cycle.D
 '''
 
+def test_mro_generic(capsys:CapSys) -> None:
+    src = '''
+    from typing import Generic, TypeVar
+    T = TypeVar('T')
+    class A: ...
+    class B(Generic[T]): ...
+    class C(A, Generic[T], B[T]): ...
+    '''
+    mod = fromText(src, modname='t')
+    assert not capsys.readouterr().out
+    assert_mro_equals(mod.contents['C'], 
+                      ["t.C", "t.A", "t.B", "typing.Generic"])
+
+def test_mro_generic_in_system(capsys:CapSys) -> None:
+    src = '''
+    class TypeVar:...
+    class Generic: ...
+    T = TypeVar('T')
+    class A: ...
+    class B(Generic[T]): ...
+    class C(A, Generic[T], B[T]): ...
+    '''
+    mod = fromText(src, modname='typing')
+    assert not capsys.readouterr().out
+    assert_mro_equals(mod.contents['C'], 
+                      ["typing.C", "typing.A", "typing.B", "typing.Generic"])
+
+
+def test_mro_generic_4(capsys:CapSys) -> None:
+    src = '''
+    from typing import Generic, TypeVar
+    T = TypeVar('T')
+    class A: ...
+    class Z: ...
+    class B(Generic[T], Z): ...
+    class C(A, Generic[T], B[T]): ...
+    '''
+    mod = fromText(src, modname='t')
+    assert not capsys.readouterr().out
+    assert_mro_equals(mod.contents['C'], 
+                      ["t.C", "t.A", "t.B", "typing.Generic", "t.Z"])
+
+def test_mro_generic_2(capsys:CapSys) -> None:
+    src = '''
+    from typing import Generic, TypeVar
+    T1 = TypeVar('T1')
+    T2 = TypeVar('T2')
+    class A(Generic[T1]): ...
+    class B(Generic[T2]): ...
+    class C(A[T1], B[T2]): ...
+    '''
+    mod = fromText(src, modname='t')
+    assert not capsys.readouterr().out
+    assert_mro_equals(mod.contents['C'], 
+                      ["t.C", "t.A", "t.B", "typing.Generic"])
+
+def test_mro_generic_3(capsys:CapSys) -> None:
+    src = '''
+    from typing import Generic as TGeneric, TypeVar
+    T = TypeVar('T')
+    class Generic: ...
+    class A(Generic): ...
+    class B(TGeneric[T]): ...
+    class C(A, B[T]): ...
+    '''
+    mod = fromText(src, modname='t')
+    assert not capsys.readouterr().out
+    assert_mro_equals(mod.contents['C'], 
+                      ["t.C", "t.A", "t.Generic", "t.B", "typing.Generic"])
+
 def test_inherited_docsources()-> None:
     simple = fromText("""\
     class A: