diff --git a/README.rst b/README.rst index c8a99a9d2..8486c9190 100644 --- a/README.rst +++ b/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 ^^^^^^^^^^^^^^^^ diff --git a/pydoctor/model.py b/pydoctor/model.py index fceaf21d9..720fffbf9 100644 --- a/pydoctor/model.py +++ b/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 @@ def is_exception(cls: 'Class') -> bool: 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 key-value pair correspond to a node and it's + predecessors, 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 @@ def init_finalbaseobjects(o: 'Class', path:Optional[List['Class']]=None) -> None 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 bogus + 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 @@ class Class(CanContainImportsDocumentable): # 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 @@ def setup(self) -> None: 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 @@ class Attribute(Inheritable): _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 @@ def fetchIntersphinxInventories(self, cache: CacheT) -> None: 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: diff --git a/pydoctor/mro.py b/pydoctor/mro.py index e8941e2aa..30997712f 100644 --- a/pydoctor/mro.py +++ b/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)) diff --git a/pydoctor/test/test_mro.py b/pydoctor/test/test_mro.py index 2c96dc1a3..1f2e5a41a 100644 --- a/pydoctor/test/test_mro.py +++ b/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: