Use stricter verifications before marking an attribute as constant (#710

)

* Use stricter verifications before marking an attribute as constant. Do not trigger any warnings when pydoctor cannot make sense of a potential constant attribute.

Fix #623

* Say that pydoctor is not a checker.

README.rst

@@ -82,6 +82,12 @@ in development
   scope when possible, when impossible, the theoretical runtime scopes are used. A warning can
   be reported when an annotation name is ambiguous (can be resolved to different names 
   depending on the scope context) with option ``-v``.
+* Use stricter verification before marking an attribute as constant: 
+   - instance variables are never marked as constant
+   - a variable that has several definitions will not be marked as constant
+   - a variable declaration under any kind of control flow block will not be marked as constant
+* Do not trigger warnings when pydoctor cannot make sense of a potential constant attribute 
+  (pydoctor is not a static checker).
 * Fix presentation of type aliases in string form.
 * Improve the AST colorizer to output less parenthesis when it's not required.
 * Fix colorization of dictionary unpacking.

pydoctor/astbuilder.py

@@ -16,8 +16,8 @@
 from pydoctor import epydoc2stan, model, node2stan, extensions, linker
 from pydoctor.epydoc.markup._pyval_repr import colorize_inline_pyval
 from pydoctor.astutils import (is_none_literal, is_typing_annotation, is_using_annotations, is_using_typing_final, node2dottedname, node2fullname, 
-                               is__name__equals__main__, unstring_annotation, iterassign, extract_docstring_linenum,  
-                               NodeVisitor)
+                               is__name__equals__main__, unstring_annotation, iterassign, extract_docstring_linenum, get_parents, 
+                               NodeVisitor, Parentage)
 
 def parseFile(path: Path) -> ast.Module:
     """Parse the contents of a Python source file."""
@@ -60,18 +60,34 @@ def _handleAliasing(
     ctx._localNameToFullName_map[target] = full_name
     return True
 
-def is_constant(obj: model.Attribute) -> bool:
+
+_CONTROL_FLOW_BLOCKS:Tuple[Type[ast.stmt],...] = (ast.If, ast.While, ast.For, ast.Try, 
+                                            ast.AsyncFor, ast.With, ast.AsyncWith)
+"""
+AST types that introduces a new control flow block, potentially conditionnal.
+"""
+if sys.version_info >= (3, 10):
+    _CONTROL_FLOW_BLOCKS += (ast.Match,)
+if sys.version_info >= (3, 11):
+    _CONTROL_FLOW_BLOCKS += (ast.TryStar,)
+
+def is_constant(obj: model.Attribute, 
+                annotation:Optional[ast.expr], 
+                value:Optional[ast.expr]) -> bool:
     """
     Detect if the given assignment is a constant. 
 
-    To detect whether a assignment is a constant, this checks two things:
-        - all-caps variable name
-        - typing.Final annotation
+    For an assignment to be detected as constant, it should: 
+        - have all-caps variable name or using L{typing.Final} annotation
+        - not be overriden
+        - not be defined in a conditionnal block or any other kind of control flow blocks
     
     @note: Must be called after setting obj.annotation to detect variables using Final.
     """
-
-    return obj.name.isupper() or is_using_typing_final(obj.annotation, obj)
+    if not is_attribute_overridden(obj, value) and value:
+        if not any(isinstance(n, _CONTROL_FLOW_BLOCKS) for n in get_parents(value)):
+            return obj.name.isupper() or is_using_typing_final(annotation, obj)
+    return False
 
 class TypeAliasVisitorExt(extensions.ModuleVisitorExt):
     """
@@ -168,6 +184,7 @@ def visit_If(self, node: ast.If) -> None:
 
     def visit_Module(self, node: ast.Module) -> None:
         assert self.module.docstring is None
+        Parentage().visit(node)
 
         self.builder.push(self.module, 0)
         if len(node.body) > 0 and isinstance(node.body[0], ast.Expr) and isinstance(node.body[0].value, ast.Str):
@@ -454,39 +471,28 @@ def _handleOldSchoolMethodDecoration(self, target: str, expr: Optional[ast.expr]
                     target_obj.kind = model.DocumentableKind.CLASS_METHOD
                 return True
         return False
-
-    def _warnsConstantAssigmentOverride(self, obj: model.Attribute, lineno_offset: int) -> None:
-        obj.report(f'Assignment to constant "{obj.name}" overrides previous assignment '
-                    f'at line {obj.linenumber}, the original value will not be part of the docs.', 
-                            section='ast', lineno_offset=lineno_offset)
-
-    def _warnsConstantReAssigmentInInstance(self, obj: model.Attribute, lineno_offset: int = 0) -> None:
-        obj.report(f'Assignment to constant "{obj.name}" inside an instance is ignored, this value will not be part of the docs.', 
-                        section='ast', lineno_offset=lineno_offset)
-
-    def _handleConstant(self, obj: model.Attribute, value: Optional[ast.expr], lineno: int) -> None:
-        """Must be called after obj.setLineNumber() to have the right line number in the warning."""
-
-        if is_attribute_overridden(obj, value):
-
-            if obj.kind in (model.DocumentableKind.CONSTANT, 
-                                model.DocumentableKind.VARIABLE, 
-                                model.DocumentableKind.CLASS_VARIABLE):
-                # Module/Class level warning, regular override.
-                self._warnsConstantAssigmentOverride(obj=obj, lineno_offset=lineno-obj.linenumber)
-            else:
-                # Instance level warning caught at the time of the constant detection.
-                self._warnsConstantReAssigmentInInstance(obj)
-
-        obj.value = value
-
-        obj.kind = model.DocumentableKind.CONSTANT
 
-        # A hack to to display variables annotated with Final with the real type instead.
-        if is_using_typing_final(obj.annotation, obj):
-            if isinstance(obj.annotation, ast.Subscript):
+    @classmethod
+    def _handleConstant(cls, obj:model.Attribute, 
+                             annotation:Optional[ast.expr], 
+                             value:Optional[ast.expr],
+                             lineno:int, 
+                             defaultKind:model.DocumentableKind) -> None:
+        if is_constant(obj, annotation=annotation, value=value):
+            obj.kind = model.DocumentableKind.CONSTANT
+            cls._tweakConstantAnnotation(obj=obj, annotation=annotation, 
+                                value=value, lineno=lineno)
+        elif obj.kind is model.DocumentableKind.CONSTANT:
+            obj.kind = defaultKind
+
+    @staticmethod
+    def _tweakConstantAnnotation(obj: model.Attribute, annotation:Optional[ast.expr], 
+                        value: Optional[ast.expr], lineno: int) -> None:
+        # Display variables annotated with Final with the real type instead.
+        if is_using_typing_final(annotation, obj):
+            if isinstance(annotation, ast.Subscript):
                 try:
-                    annotation = extract_final_subscript(obj.annotation)
+                    annotation = extract_final_subscript(annotation)
                 except ValueError as e:
                     obj.report(str(e), section='ast', lineno_offset=lineno-obj.linenumber)
                     obj.annotation = _infer_type(value) if value else None
@@ -498,21 +504,46 @@ def _handleConstant(self, obj: model.Attribute, value: Optional[ast.expr], linen
                 # Simply ignore it because it's duplication of information.
                 obj.annotation = _infer_type(value) if value else None
 
+    @staticmethod
+    def _storeAttrValue(obj:model.Attribute, new_value:Optional[ast.expr], 
+                        augassign:Optional[ast.operator]=None) -> None:
+        if new_value:
+            if augassign: 
+                if obj.value:
+                    # We're storing the value of augmented assignemnt value as binop for the sake 
+                    # of correctness, but we're not doing anything special with it at the
+                    # moment, nonethless this could be useful for future developments.
+                    # We don't bother reporting warnings, pydoctor is not a checker.
+                    obj.value = ast.BinOp(left=obj.value, op=augassign, right=new_value)
+            else:
+                obj.value = new_value
+
+    def _storeCurrentAttr(self, obj:model.Attribute, 
+                          augassign:Optional[object]=None) -> None:
+        if not augassign:
+            self.builder.currentAttr = obj
+        else:
+            self.builder.currentAttr = None
+
     def _handleModuleVar(self,
             target: str,
             annotation: Optional[ast.expr],
             expr: Optional[ast.expr],
-            lineno: int
+            lineno: int,
+            augassign:Optional[ast.operator],
             ) -> None:
         if target in MODULE_VARIABLES_META_PARSERS:
             # This is metadata, not a variable that needs to be documented,
             # and therefore doesn't need an Attribute instance.
             return
         parent = self.builder.current
         obj = parent.contents.get(target)
-
         if obj is None:
-            obj = self.builder.addAttribute(name=target, kind=None, parent=parent)
+            if augassign:
+                return
+            obj = self.builder.addAttribute(name=target, 
+                                            kind=model.DocumentableKind.VARIABLE, 
+                                            parent=parent)
 
         # If it's not an attribute it means that the name is already denifed as function/class 
         # probably meaning that this attribute is a bound callable. 
@@ -534,32 +565,29 @@ def _handleModuleVar(self,
         obj.annotation = annotation
         obj.setLineNumber(lineno)
 
-        if is_constant(obj):
-            self._handleConstant(obj=obj, value=expr, lineno=lineno)
-        else:
-            obj.kind = model.DocumentableKind.VARIABLE
-            # We store the expr value for all Attribute in order to be able to 
-            # check if they have been initialized or not.
-            obj.value = expr
-
-        self.builder.currentAttr = obj
+        self._handleConstant(obj, annotation, expr, lineno, 
+                                  model.DocumentableKind.VARIABLE)
+        self._storeAttrValue(obj, expr, augassign)
+        self._storeCurrentAttr(obj, augassign)
 
     def _handleAssignmentInModule(self,
             target: str,
             annotation: Optional[ast.expr],
             expr: Optional[ast.expr],
-            lineno: int
+            lineno: int,
+            augassign:Optional[ast.operator],
             ) -> None:
         module = self.builder.current
         assert isinstance(module, model.Module)
         if not _handleAliasing(module, target, expr):
-            self._handleModuleVar(target, annotation, expr, lineno)
+            self._handleModuleVar(target, annotation, expr, lineno, augassign=augassign)
 
     def _handleClassVar(self,
             name: str,
             annotation: Optional[ast.expr],
             expr: Optional[ast.expr],
-            lineno: int
+            lineno: int,
+            augassign:Optional[ast.operator],
             ) -> None:
         cls = self.builder.current
         assert isinstance(cls, model.Class)
@@ -570,6 +598,8 @@ def _handleClassVar(self,
         obj = cast(Optional[model.Attribute], cls.contents.get(name))
 
         if obj is None:
+            if augassign:
+                return
             obj = self.builder.addAttribute(name=name, kind=None, parent=cls)
 
         if obj.kind is None:
@@ -582,12 +612,10 @@ def _handleClassVar(self,
         obj.annotation = annotation
         obj.setLineNumber(lineno)
 
-        if is_constant(obj):
-            self._handleConstant(obj=obj, value=expr, lineno=lineno)
-        else:
-            obj.value = expr
-
-        self.builder.currentAttr = obj
+        self._handleConstant(obj, annotation, expr, lineno, 
+                                  model.DocumentableKind.CLASS_VARIABLE)
+        self._storeAttrValue(obj, expr, augassign)
+        self._storeCurrentAttr(obj, augassign)
 
     def _handleInstanceVar(self,
             name: str,
@@ -607,35 +635,29 @@ def _handleInstanceVar(self,
         # 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))
         if obj is None:
-
             obj = self.builder.addAttribute(name=name, kind=None, parent=cls)
 
         if annotation is None and expr is not None:
             annotation = _infer_type(expr)
 
         obj.annotation = annotation
         obj.setLineNumber(lineno)
-
-        # Maybe an instance variable overrides a constant, 
-        # so we check before setting the kind to INSTANCE_VARIABLE.
-        if obj.kind is model.DocumentableKind.CONSTANT:
-            self._warnsConstantReAssigmentInInstance(obj, lineno_offset=lineno-obj.linenumber)
-        else:
-            obj.kind = model.DocumentableKind.INSTANCE_VARIABLE
-            obj.value = expr
-
-        self.builder.currentAttr = obj
+        # undonditionnaly set the kind to ivar
+        obj.kind = model.DocumentableKind.INSTANCE_VARIABLE
+        self._storeAttrValue(obj, expr)
+        self._storeCurrentAttr(obj)
 
     def _handleAssignmentInClass(self,
             target: str,
             annotation: Optional[ast.expr],
             expr: Optional[ast.expr],
-            lineno: int
+            lineno: int,
+            augassign:Optional[ast.operator],
             ) -> None:
         cls = self.builder.current
         assert isinstance(cls, model.Class)
         if not _handleAliasing(cls, target, expr):
-            self._handleClassVar(target, annotation, expr, lineno)
+            self._handleClassVar(target, annotation, expr, lineno, augassign=augassign)
 
     def _handleDocstringUpdate(self,
             targetNode: ast.expr,
@@ -689,17 +711,18 @@ def _handleAssignment(self,
             targetNode: ast.expr,
             annotation: Optional[ast.expr],
             expr: Optional[ast.expr],
-            lineno: int
+            lineno: int,
+            augassign:Optional[ast.operator]=None,
             ) -> None:
         if isinstance(targetNode, ast.Name):
             target = targetNode.id
             scope = self.builder.current
             if isinstance(scope, model.Module):
-                self._handleAssignmentInModule(target, annotation, expr, lineno)
+                self._handleAssignmentInModule(target, annotation, expr, lineno, augassign=augassign)
             elif isinstance(scope, model.Class):
-                if not self._handleOldSchoolMethodDecoration(target, expr):
-                    self._handleAssignmentInClass(target, annotation, expr, lineno)
-        elif isinstance(targetNode, ast.Attribute):
+                if augassign or not self._handleOldSchoolMethodDecoration(target, expr):
+                    self._handleAssignmentInClass(target, annotation, expr, lineno, augassign=augassign)
+        elif isinstance(targetNode, ast.Attribute) and not augassign:
             value = targetNode.value
             if targetNode.attr == '__doc__':
                 self._handleDocstringUpdate(value, expr, lineno)
@@ -728,6 +751,10 @@ def visit_Assign(self, node: ast.Assign) -> None:
     def visit_AnnAssign(self, node: ast.AnnAssign) -> None:
         annotation = unstring_annotation(node.annotation, self.builder.current)
         self._handleAssignment(node.target, annotation, node.value, node.lineno)
+
+    def visit_AugAssign(self, node:ast.AugAssign) -> None:
+        self._handleAssignment(node.target, None, node.value, 
+                               node.lineno, augassign=node.op)
 
     def visit_Expr(self, node: ast.Expr) -> None:
         value = node.value
@@ -904,7 +931,9 @@ def _handlePropertyDef(self,
             lineno: int
             ) -> model.Attribute:
 
-        attr = self.builder.addAttribute(name=node.name, kind=model.DocumentableKind.PROPERTY, parent=self.builder.current)
+        attr = self.builder.addAttribute(name=node.name, 
+                                         kind=model.DocumentableKind.PROPERTY, 
+                                         parent=self.builder.current)
         attr.setLineNumber(lineno)
 
         if docstring is not None:

pydoctor/astutils.py

@@ -6,7 +6,7 @@
 import platform
 import sys
 from numbers import Number
-from typing import Iterator, Optional, List, Iterable, Sequence, TYPE_CHECKING, Tuple, Union
+from typing import Iterator, Optional, List, Iterable, Sequence, TYPE_CHECKING, Tuple, Union, cast
 from inspect import BoundArguments, Signature
 import ast
 
@@ -402,4 +402,31 @@ def extract_docstring(node: ast.Str) -> Tuple[int, str]:
         - The docstring to be parsed, cleaned by L{inspect.cleandoc}.
     """
     lineno = extract_docstring_linenum(node)
-    return lineno, inspect.cleandoc(node.s)
+    return lineno, inspect.cleandoc(node.s)
+
+class Parentage(ast.NodeTransformer):
+    """
+    Add C{parent} attribute to ast nodes instances.
+    """
+    # stolen from https://stackoverflow.com/a/68845448
+    parent: Optional[ast.AST] = None
+
+    def visit(self, node: ast.AST) -> ast.AST:
+        setattr(node, 'parent', self.parent)
+        self.parent = node
+        node = super().visit(node)
+        if isinstance(node, ast.AST):
+            self.parent = getattr(node, 'parent')
+        return node
+
+def get_parents(node:ast.AST) -> Iterator[ast.AST]:
+    """
+    Once nodes have the C{.parent} attribute with {Parentage}, use this function
+    to get a iterator on all parents of the given node up to the root module.
+    """
+    def _yield_parents(n:Optional[ast.AST]) -> Iterator[ast.AST]:
+        if n:
+            yield n
+            p = cast(ast.AST, getattr(n, 'parent', None))
+            yield from _yield_parents(p)
+    yield from _yield_parents(getattr(node, 'parent', None))