Merge pull request #579 from twisted/10-c3linear

Correctly compute class method resolution order

Author: tristanlatr

README.rst

@@ -75,6 +75,7 @@ What's New?
 
 in development
 ^^^^^^^^^^^^^^
+* Fix long standing bugs in ``Class`` method resolution order.
 * Improve the extensibility of pydoctor (`more infos on extensions <https://pydoctor.readthedocs.io/en/latest/customize.html#use-a-custom-system-class>`_)
 * Fix line numbers in reStructuredText xref warnings.
 * Add support for `twisted.python.deprecated` (this was originally part of Twisted's customizations).

docs/tests/test.py

@@ -236,3 +236,19 @@ def getText(node: ET.Element) -> str:
                 privacy = getText(liobj.findall('./div[@class=\'privacy\']')[0])
                 # check that it's indeed private
                 assert privacy == 'HIDDEN'
+
+def test_missing_subclasses():
+    """
+    Test for missing subclasses of ParsedDocstring, issue https://github.com/twisted/pydoctor/issues/528.
+    """
+
+    infos = ('pydoctor.epydoc.markup._types.ParsedTypeDocstring', 
+        'pydoctor.epydoc.markup.epytext.ParsedEpytextDocstring', 
+        'pydoctor.epydoc.markup.plaintext.ParsedPlaintextDocstring', 
+        'pydoctor.epydoc.markup.restructuredtext.ParsedRstDocstring', 
+        'pydoctor.epydoc2stan.ParsedStanOnly')
+
+    with open(BASE_DIR / 'api' / 'pydoctor.epydoc.markup.ParsedDocstring.html', 'r', encoding='utf-8') as stream:
+        page = stream.read()
+        for i in infos:
+            assert i in page, page

mypy.ini

@@ -54,6 +54,10 @@ ignore_missing_imports=True
 [mypy-pydoctor.epydoc.sre_parse36]
 ignore_errors=True
 
+# Don't check the C3 lineartization code
+[mypy-pydoctor.mro.*]
+ignore_errors=True
+
 # Don't check our test data:
 
 [mypy-pydoctor.test.testpackages.*]

pydoctor/astbuilder.py

@@ -229,22 +229,19 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None:
             raise self.SkipNode()
 
         rawbases = []
-        bases = []
-        baseobjects = []
+        initialbases = []
+        initialbaseobjects = []
 
         for n in node.bases:
-            if isinstance(n, ast.Name):
-                str_base = n.id
-            else:
-                str_base = astor.to_source(n).strip()
-
+            # TODO: Handle generics in MRO
+            str_base = '.'.join(node2dottedname(n) or [astor.to_source(n).strip()])
             rawbases.append(str_base)
-            full_name = parent.expandName(str_base)
-            bases.append(full_name)
-            baseobj = self.system.objForFullName(full_name)
+            expandbase = parent.expandName(str_base)
+            baseobj = self.system.objForFullName(expandbase)
             if not isinstance(baseobj, model.Class):
                 baseobj = None
-            baseobjects.append(baseobj)
+            initialbases.append(expandbase)
+            initialbaseobjects.append(baseobj)
 
         lineno = node.lineno
         if node.decorator_list:
@@ -253,8 +250,8 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None:
         cls: model.Class = self.builder.pushClass(node.name, lineno)
         cls.decorators = []
         cls.rawbases = rawbases
-        cls.bases = bases
-        cls.baseobjects = baseobjects
+        cls._initialbaseobjects = initialbaseobjects
+        cls._initialbases = initialbases
 
         if len(node.body) > 0 and isinstance(node.body[0], ast.Expr) and isinstance(node.body[0].value, ast.Str):
             cls.setDocstring(node.body[0].value)
@@ -279,10 +276,10 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None:
                 else:
                     cls.decorators.append((base, args))
         cls.raw_decorators = node.decorator_list if node.decorator_list else []
-
-        for b in cls.baseobjects:
-            if b is not None:
-                b.subclasses.append(cls)
+        
+        # We're not resolving the subclasses at this point yet because all 
+        # modules might not have been processed, and since subclasses are only used in the presentation,
+        # it's better to resolve them in the post-processing instead.
 
     def depart_ClassDef(self, node: ast.ClassDef) -> None:
         self.builder.popClass()

pydoctor/extensions/zopeinterface.py

@@ -62,7 +62,7 @@ def _inheritedDocsources(obj: model.Documentable) -> Iterator[model.Documentable
         io = obj.system.objForFullName(interface)
         if io is not None:
             assert isinstance(io, ZopeInterfaceClass)
-            for io2 in io.allbases(include_self=True):
+            for io2 in io.mro():
                 if name in io2.contents:
                     yield io2.contents[name]
 
@@ -182,15 +182,13 @@ def _handleZopeInterfaceAssignmentInModule(self,
             return
         ob = self.visitor.system.objForFullName(funcName)
         if isinstance(ob, ZopeInterfaceClass) and ob.isinterfaceclass:
-            # TODO: Process 'bases' and '__doc__' arguments.
+            # TODO: Process 'rawbases' and '__doc__' arguments.
             # TODO: Currently, this implementation will create a duplicate class 
             # with the same name as the attribute, overriding it.
             interface = self.visitor.builder.pushClass(target, lineno)
             assert isinstance(interface, ZopeInterfaceClass)
             interface.isinterface = True
             interface.implementedby_directly = []
-            interface.bases = []
-            interface.baseobjects = []
             self.visitor.builder.popClass()
             self.visitor.builder.currentAttr = interface
 

pydoctor/model.py

@@ -25,7 +25,7 @@
 from urllib.parse import quote
 
 from pydoctor.options import Options
-from pydoctor import factory, qnmatch, utils, linker, astutils
+from pydoctor import factory, qnmatch, utils, linker, astutils, mro
 from pydoctor.epydoc.markup import ParsedDocstring
 from pydoctor.sphinx import CacheT, SphinxInventory
 
@@ -481,12 +481,69 @@ def submodules(self) -> Iterator['Module']:
 class Package(Module):
     kind = DocumentableKind.PACKAGE
 
+def compute_mro(cls:'Class') -> List[Union['Class', str]]:
+    """
+    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.
+    """
+    def init_finalbaseobjects(o: 'Class', path:Optional[List['Class']]=None) -> None:
+        if not path:
+            path = []
+        if o in path:
+            cycle_str = " -> ".join([o.fullName() for o in path[path.index(cls):] + [cls]])
+            raise ValueError(f"Cycle found while computing inheritance hierarchy: {cycle_str}")
+        path.append(o)
+        if o._finalbaseobjects is not None:
+            return
+        if o.rawbases:
+            finalbaseobjects: List[Optional[Class]] = []
+            finalbases: List[str] = []
+            for i,(str_base, base) in enumerate(zip(o.rawbases, o._initialbaseobjects)):
+                if base:
+                    finalbaseobjects.append(base)
+                    finalbases.append(base.fullName())
+                else:
+                    resolved_base = o.parent.resolveName(str_base)
+                    if isinstance(resolved_base, Class):
+                        base = resolved_base
+                        finalbaseobjects.append(base)
+                        finalbases.append(base.fullName())
+                    else:
+                        # the base object could not be resolved
+                        finalbaseobjects.append(None)
+                        finalbases.append(o._initialbases[i])
+                if base:
+                    init_finalbaseobjects(base, path.copy())
+            o._finalbaseobjects = finalbaseobjects
+            o._finalbases = finalbases
+    
+    def localbases(o:'Class') -> Iterator[Union['Class', str]]:
+        """
+        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 getbases(o:Union['Class', str]) -> List[Union['Class', str]]:
+        if isinstance(o, str):
+            return []
+        return list(localbases(o))
+
+    init_finalbaseobjects(cls)
+    return mro.mro(cls, getbases)
 
 class Class(CanContainImportsDocumentable):
     kind = DocumentableKind.CLASS
     parent: CanContainImportsDocumentable
-    bases: List[str]
-    baseobjects: List[Optional['Class']]
+    _initialbaseobjects: List[Optional['Class']]
+    _finalbaseobjects: Optional[List[Optional['Class']]] = None # set in post-processing
+    _initialbases: List[str]
+    _finalbases: Optional[List[str]] = None # set in post-processing
     decorators: Sequence[Tuple[str, Optional[Sequence[ast.expr]]]]
     # Note: While unused in pydoctor itself, raw_decorators is still in use
     #       by Twisted's custom System class, to find deprecations.
@@ -501,8 +558,68 @@ def setup(self) -> None:
         super().setup()
         self.rawbases: List[str] = []
         self.subclasses: List[Class] = []
+        self._initialbases = []
+        self._initialbaseobjects = []
+        self._mro: Optional[List[Union['Class', str]]] = None
+    
+    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) -> List[Union['Class', str]]:...
+    @overload
+    def mro(self, include_external:'Literal[False]'=False, include_self:bool=True) -> List['Class']:...
+    def mro(self, include_external:bool=False, include_self:bool=True) -> List[Union['Class', str]]: # type:ignore[misc]
+        """
+        Get the method resution order of this class. 
+
+        @note: The actual correct value is only set in post-processing, if L{mro()} is called
+            in the AST visitors, it will return the same as C{list(self.allbases(include_self))}.
+        """
+        if self._mro is None:
+            return list(self.allbases(include_self))
+        
+        _mro: List[Union[str, Class]]
+        if include_external is False:
+            _mro = [o for o in self._mro if not isinstance(o, str)]
+        else:
+            _mro = self._mro
+        if include_self is False:
+            _mro = _mro[1:]
+        return _mro
+
+    @property
+    def bases(self) -> List[str]:
+        """
+        Fully qualified names of the bases of this class.
+        """
+        return self._finalbases if \
+            self._finalbases is not None else self._initialbases
+    
+    @property
+    def baseobjects(self) -> List[Optional['Class']]:
+        """
+        Base objects, L{None} value is inserted when the base class could not be found in the system.
+        
+        @note: This property is currently computed two times, a first time when we're visiting the ClassDef and initially creating the object. 
+            It's computed another time in post-processing to try to resolve the names that could not be resolved the first time. This is needed when there are import cycles. 
+            
+            Meaning depending on the state of the system, this property can return either the initial objects or the final objects
+        """
+        return self._finalbaseobjects if \
+            self._finalbaseobjects is not None else self._initialbaseobjects
+    
     def allbases(self, include_self: bool = False) -> Iterator['Class']:
+        """
+        Iterate on all base objects of this class and it's super classes. Doesn't comply with MRO.
+        """
         if include_self:
             yield self
         for b in self.baseobjects:
@@ -514,7 +631,7 @@ def find(self, name: str) -> Optional[Documentable]:
 
         @return: the object with the given name, or L{None} if there isn't one
         """
-        for base in self.allbases(include_self=True):
+        for base in self.mro():
             obj: Optional[Documentable] = base.contents.get(name)
             if obj is not None:
                 return obj
@@ -554,7 +671,7 @@ def docsources(self) -> Iterator[Documentable]:
         yield self
         if not isinstance(self.parent, Class):
             return
-        for b in self.parent.allbases(include_self=False):
+        for b in self.parent.mro(include_self=False):
             if self.name in b.contents:
                 yield b.contents[self.name]
 
@@ -1006,8 +1123,6 @@ def _introspectThing(self, thing: object, parent: Documentable, parentMod: _Modu
                 self.addObject(f)
             elif isinstance(v, type):
                 c = self.Class(self, k, parent)
-                c.bases = []
-                c.baseobjects = []
                 c.rawbases = []
                 c.parentMod = parentMod
                 c.docstring = v.__doc__
@@ -1162,6 +1277,15 @@ def postProcess(self) -> None:
         without the risk of drawing incorrect conclusions because modules
         were not fully processed yet.
         """
+
+        # default post-processing includes processing of subclasses and MRO computing.
+        for cls in self.objectsOfType(Class):
+            cls._init_mro()
+            for b in cls.baseobjects:
+                if b is not None:
+                    b.subclasses.append(cls)
+
+
         for post_processor in self._post_processors:
             post_processor(self)