Visit Try 'orelse', 'finalbody' and 'handlers' and If 'orelse' (#589)

Co-authored-by: Glyph <glyph@twistedmatrix.com>
Co-authored-by: tristanlatr <tristanlatr@users.noreply.github.com>

README.rst

@@ -74,6 +74,11 @@ in development
 ^^^^^^^^^^^^^^
 
 * Drop Python 3.7 and support Python 3.13.
+* Improve collection of objects:
+   - Document objects declared in the ``else`` block of 'if' statements (previously they were ignored).
+   - Document objects declared in ``finalbody`` and ``else`` block of 'try' statements (previously they were ignored).
+   - Objects declared in the ``else`` block of if statements and in the ``handlers`` of 'try' statements
+     are ignored if a concurrent object is declared before (`more infos on branch priorities <https://pydoctor.readthedocs.io/en/latest/codedoc.html#branch-priorities>`_).
 * Trigger a warning when several docstrings are detected for the same object.
 * Improve typing of docutils related code.
 * Run unit tests on all supported combinations of Python versions and platforms, including PyPy for Windows. Previously, tests where ran on all supported Python version for Linux, but not for MacOS and Windows.

docs/source/codedoc.rst

@@ -333,3 +333,54 @@ The content of ``my_project/__init__.py`` includes::
     from .core._impl import MyClass
 
     __all__ = ("MyClass",)
+
+Branch priorities
+-----------------
+
+When pydoctor deals with try/except/else or if/else block, it makes sure that the names defined in 
+the main flow has precedence over the definitions in ``except`` handlers or ``else`` blocks.
+
+Meaning that in the context of the code below, ``ssl`` would resolve to ``twisted.internet.ssl``:
+
+.. code:: python
+
+    try:
+        # main flow
+        from twisted.internet import ssl as _ssl
+    except ImportError:
+        # exceptional flow
+        ssl = None # ignored since 'ssl' is defined in the main flow below.
+        var = True # not ignored since 'var' is not defined anywhere else.
+    else:
+        # main flow
+        ssl = _ssl
+
+Similarly, in the context of the code below, the ``CapSys`` protocol under the ``TYPE_CHECKING`` block will be 
+documented and the runtime version will be ignored.
+
+.. code:: python
+
+    from typing import TYPE_CHECKING
+    if TYPE_CHECKING:
+        # main flow
+        from typing import Protocol
+        class CapSys(Protocol):
+            def readouterr() -> Any:
+                ...
+    else:
+        # secondary flow
+        class CapSys(object): # ignored since 'CapSys' is defined in the main flow above.
+            ...
+
+But sometimes pydoctor can be better off analysing the ``TYPE_CHECKING`` blocks and should 
+stick to the runtime version of the code instead. 
+For these case, you might want to inverse the condition of if statement: 
+
+ .. code:: python
+    
+    if not TYPE_CHECKING:
+        # main flow
+        from ._implementation import Thing
+    else:
+        # secondary flow
+        from ._typing import Thing # ignored since 'Thing' is defined in the main flow above.

pydoctor/astbuilder.py

@@ -2,15 +2,15 @@
 from __future__ import annotations
 
 import ast
+import contextlib
 import sys
 
 from functools import partial
 from inspect import Parameter, Signature
-from itertools import chain
 from pathlib import Path
 from typing import (
     Any, Callable, Collection, Dict, Iterable, Iterator, List, Mapping, Optional, Sequence, Tuple,
-    Type, TypeVar, Union, cast
+    Type, TypeVar, Union, Set, cast
 )
 
 from pydoctor import epydoc2stan, model, node2stan, extensions, linker
@@ -176,6 +176,35 @@ def __init__(self, builder: 'ASTBuilder', module: model.Module):
         self.builder = builder
         self.system = builder.system
         self.module = module
+        self._override_guard_state: Tuple[Optional[model.Documentable], Set[str]] = (None, set())
+
+    @contextlib.contextmanager
+    def override_guard(self) -> Iterator[None]:
+        """
+        Returns a context manager that will make the builder ignore any new 
+        assigments to existing names within the same context.  Currently used to visit C{If.orelse} and C{Try.handlers}.
+        
+        @note: The list of existing names is generated at the moment of
+            calling the function, such that new names defined inside these blocks follows the usual override rules.
+        """
+        ctx = self.builder.current
+        while not isinstance(ctx, model.CanContainImportsDocumentable):
+            assert ctx.parent
+            ctx = ctx.parent
+        ignore_override_init = self._override_guard_state
+        # we list names only once to ignore new names added inside the block,
+        # they should be overriden as usual.
+        self._override_guard_state = (ctx, set(ctx.localNames()))
+        yield
+        self._override_guard_state = ignore_override_init
+
+    def _ignore_name(self, ob: model.Documentable, name:str) -> bool:
+        """
+        Should this C{name} be ignored because it matches 
+        the override guard in the context of C{ob}?
+        """
+        ctx, names = self._override_guard_state
+        return ctx is ob and name in names
 
     def _infer_attr_annotations(self, scope: model.Documentable) -> None:
         # Infer annotation when leaving scope so explicit
@@ -195,7 +224,29 @@ def visit_If(self, node: ast.If) -> None:
                 # skip if __name__ == '__main__': blocks since
                 # whatever is declared in them cannot be imported
                 # and thus is not part of the API
-                raise self.SkipNode()
+                raise self.SkipChildren()
+
+    def depart_If(self, node: ast.If) -> None:
+        # At this point the body of the If node has already been visited
+        # Visit the 'orelse' block of the If node, with override guard
+        with self.override_guard():
+            for n in node.orelse:
+                self.walkabout(n)
+
+    def depart_Try(self, node: ast.Try) -> None:
+        # At this point the body of the Try node has already been visited
+        # Visit the 'orelse' and 'finalbody' blocks of the Try node.
+
+        for n in node.orelse:
+            self.walkabout(n)
+        for n in node.finalbody:
+            self.walkabout(n)
+
+        # Visit the handlers with override guard 
+        with self.override_guard():
+            for h in node.handlers:
+                for n in h.body:
+                    self.walkabout(n)
 
     def visit_Module(self, node: ast.Module) -> None:
         assert self.module.docstring is None
@@ -216,6 +267,9 @@ def visit_ClassDef(self, node: ast.ClassDef) -> None:
         parent = self.builder.current
         if isinstance(parent, model.Function):
             raise self.SkipNode()
+        # Ignore in override guard
+        if self._ignore_name(parent, node.name):
+            raise self.IgnoreNode()
 
         rawbases = []
         initialbases = []
@@ -337,35 +391,37 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None:
     def _importAll(self, modname: str) -> None:
         """Handle a C{from <modname> import *} statement."""
 
+        current = self.builder.current
+
         mod = self.system.getProcessedModule(modname)
         if mod is None:
             # We don't have any information about the module, so we don't know
             # what names to import.
-            self.builder.current.report(f"import * from unknown {modname}", thresh=1)
+            current.report(f"import * from unknown {modname}", thresh=1)
             return
 
-        self.builder.current.report(f"import * from {modname}", thresh=1)
+        current.report(f"import * from {modname}", thresh=1)
 
         # Get names to import: use __all__ if available, otherwise take all
         # names that are not private.
         names = mod.all
         if names is None:
-            names = [
-                name
-                for name in chain(mod.contents.keys(),
-                                  mod._localNameToFullName_map.keys())
-                if not name.startswith('_')
-                ]
+            names = [ name for name in mod.localNames() 
+                     if not name.startswith('_') ]
 
         # Fetch names to export.
         exports = self._getCurrentModuleExports()
 
         # Add imported names to our module namespace.
-        assert isinstance(self.builder.current, model.CanContainImportsDocumentable)
-        _localNameToFullName = self.builder.current._localNameToFullName_map
+        assert isinstance(current, model.CanContainImportsDocumentable)
+        _localNameToFullName = current._localNameToFullName_map
         expandName = mod.expandName
         for name in names:
 
+            # # Ignore in override guard
+            if self._ignore_name(current, name):
+                continue
+
             if self._handleReExport(exports, name, name, mod) is True:
                 continue
 
@@ -429,6 +485,11 @@ def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None:
             orgname, asname = al.name, al.asname
             if asname is None:
                 asname = orgname
+
+            # Ignore in override guard
+            if self._ignore_name(current, asname):
+                continue
+
             # If we're importing from a package, make sure imported modules
             # are processed (getProcessedModule() ignores non-modules).
             if isinstance(mod, model.Package):
@@ -451,15 +512,20 @@ def visit_Import(self, node: ast.Import) -> None:
         (dotted_name, as_name) where as_name is None if there was no 'as foo'
         part of the statement.
         """
-        if not isinstance(self.builder.current, model.CanContainImportsDocumentable):
+        current = self.builder.current
+        if not isinstance(current, model.CanContainImportsDocumentable):
             # processing import statement in odd context
             return
-        _localNameToFullName = self.builder.current._localNameToFullName_map
+        _localNameToFullName = current._localNameToFullName_map
+
         for al in node.names:
             targetname, asname = al.name, al.asname
             if asname is None:
                 # we're keeping track of all defined names
                 asname = targetname = targetname.split('.')[0]
+            # Ignore in override guard
+            if self._ignore_name(current, asname):
+                continue
             _localNameToFullName[asname] = targetname
 
     def _handleOldSchoolMethodDecoration(self, target: str, expr: Optional[ast.expr]) -> bool:
@@ -644,6 +710,8 @@ def _handleInstanceVar(self,
             raise IgnoreAssignment()
         if not _maybeAttribute(cls, name):
             raise IgnoreAssignment()
+        if self._ignore_name(cls, name):
+            raise IgnoreAssignment()
 
         # Class variables can only be Attribute, so it's OK to cast because we used _maybeAttribute() above.
         obj = cast(Optional[model.Attribute], cls.contents.get(name))
@@ -732,6 +800,8 @@ def _handleAssignment(self,
         if isinstance(targetNode, ast.Name):
             target = targetNode.id
             scope = self.builder.current
+            if self._ignore_name(scope, target):
+                raise IgnoreAssignment()
             if isinstance(scope, model.Module):
                 self._handleAssignmentInModule(target, annotation, expr, lineno, augassign=augassign)
             elif isinstance(scope, model.Class):
@@ -863,6 +933,9 @@ def _handleFunctionDef(self,
         parent = self.builder.current
         if isinstance(parent, model.Function):
             raise self.SkipNode()
+        # Ignore in override guard
+        if self._ignore_name(parent, node.name):
+            raise self.IgnoreNode()
 
         lineno = node.lineno
 
@@ -909,7 +982,7 @@ def _handleFunctionDef(self,
                 attr.report(f'{attr.fullName()} is both property and classmethod')
             if is_staticmethod:
                 attr.report(f'{attr.fullName()} is both property and staticmethod')
-            raise self.SkipNode()
+            raise self.SkipNode() # visitor extensions will still be called.
 
         # Check if it's a new func or exists with an overload
         existing_func = parent.contents.get(func_name)
@@ -920,7 +993,7 @@ def _handleFunctionDef(self,
             # properties set for the primary function and not overloads.
             if existing_func.signature and is_overload_func:
                 existing_func.report(f'{existing_func.fullName()} overload appeared after primary function', lineno_offset=lineno-existing_func.linenumber)
-                raise self.SkipNode()
+                raise self.IgnoreNode()
             # Do not recreate function object, just re-push it
             self.builder.push(existing_func, lineno)
             func = existing_func

pydoctor/extensions/deprecate.py

@@ -55,25 +55,25 @@ def depart_ClassDef(self, node:ast.ClassDef) -> None:
         """
         Called after a class definition is visited.
         """
+        current = self.visitor.builder.current
         try:
-            cls = self.visitor.builder.current.contents[node.name]
+            cls = current.contents[node.name]
         except KeyError:
             # Classes inside functions are ignored.
             return
-        assert isinstance(cls, model.Class)
         getDeprecated(cls, node.decorator_list)
 
     def depart_FunctionDef(self, node:ast.FunctionDef) -> None:
         """
         Called after a function definition is visited.
         """
+        current = self.visitor.builder.current
         try:
             # Property or Function
-            func = self.visitor.builder.current.contents[node.name]
+            func = current.contents[node.name]
         except KeyError:
             # Inner functions are ignored.
             return
-        assert isinstance(func, (model.Function, model.Attribute))
         getDeprecated(func, node.decorator_list)
 
 _incremental_Version_signature = inspect.signature(Version)

pydoctor/model.py

@@ -9,7 +9,7 @@
 
 import abc
 import ast
-import attr
+from itertools import chain
 from collections import defaultdict
 import datetime
 import importlib
@@ -26,6 +26,8 @@
 )
 from urllib.parse import quote
 
+import attr
+
 from pydoctor.options import Options
 from pydoctor import factory, qnmatch, utils, linker, astutils, mro
 from pydoctor.epydoc.markup import ParsedDocstring
@@ -468,6 +470,9 @@ def isNameDefined(self, name: str) -> bool:
         else:
             return False
 
+    def localNames(self) -> Iterator[str]:
+        return chain(self.contents.keys(),
+                     self._localNameToFullName_map.keys())
 
 class Module(CanContainImportsDocumentable):
     kind = DocumentableKind.MODULE