From cf9df5294f3cb250c38c6e8202095a294416ff9e Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Tue, 11 Jul 2023 11:41:59 -0400 Subject: [PATCH 01/15] Fix #604 --- pydoctor/astbuilder.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index e1fc18004..6edbc9524 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -394,13 +394,13 @@ def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None: if asname is None: asname = orgname - if mod is not None and self._handleReExport(exports, orgname, asname, mod) is True: - continue - # If we're importing from a package, make sure imported modules # are processed (getProcessedModule() ignores non-modules). if isinstance(mod, model.Package): self.system.getProcessedModule(f'{modname}.{orgname}') + + if mod is not None and self._handleReExport(exports, orgname, asname, mod) is True: + continue _localNameToFullName[asname] = f'{modname}.{orgname}' From 893645958a3c28ebe7a542a334df729527f734c7 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 18:32:12 -0400 Subject: [PATCH 02/15] Rework reparenting in post-processing. Introduces the Module.imports attribue holding a list of resolved imports, this is required by the re-exporting process since only imported names should be reparented. Introduces the System.modules attribute that is a dict from the module full names to the Module instances. This dict is not changed during reparenting. Switch from using of a actual dict to hold values of System.allobjects attribute; instead allobject is a proxy that lazily get the documentable having the requested fullname based on system's rootobjects and modules attributes. It simplifies drastically the reparenting code since the only thing left to handle is the new name of object and it's parent.contents links. This commit should not introduce any changes in behavior. --- pydoctor/astbuilder.py | 166 +++++++++++++++----------- pydoctor/model.py | 176 +++++++++++++++++++++------- pydoctor/templatewriter/__init__.py | 2 +- pydoctor/test/test_astbuilder.py | 2 +- pydoctor/test/test_packages.py | 12 +- 5 files changed, 243 insertions(+), 115 deletions(-) diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index 6edbc9524..0c5ebc68b 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -149,6 +149,83 @@ def extract_final_subscript(annotation: ast.Subscript) -> ast.expr: assert isinstance(ann_slice, ast.expr) return ann_slice +def _handleReExport(mod:'model.Module', + origin_name:str, as_name:str, + origin_module:model.Module) -> None: + """ + Move re-exported objects into current module. + """ + # Move re-exported objects into current module. + current = mod + modname = origin_module.fullName() + + # In case of duplicates names, we can't rely on resolveName, + # So we use content.get first to resolve non-alias names. + ob = origin_module.contents.get(origin_name) or origin_module.resolveName(origin_name) + if ob is None: + current.report("cannot resolve re-exported name :" + f'{modname}.{origin_name}', thresh=1) + else: + if origin_module.all is None or origin_name not in origin_module.all: + if as_name != ob.name: + current.system.msg( + "astbuilder", + "moving %r into %r as %r" % (ob.fullName(), current.fullName(), as_name) + ) + else: + current.system.msg( + "astbuilder", + "moving %r into %r" % (ob.fullName(), current.fullName()) + ) + ob.reparent(current, as_name) + else: + current.system.msg( + "astbuilder", + f"not moving %r into %r, because {origin_name} is present in {modname}.__all__" % (ob.fullName(), current.fullName()), + thresh=1 + ) + +def getModuleExports(mod:'model.Module') -> Collection[str]: + # Fetch names to export. + exports = mod.all + if exports is None: + exports = [] + return exports + +def getPublicNames(mod:'model.Module') -> Collection[str]: + names = mod.all + if names is None: + names = [ + name + for name in chain(mod.contents.keys(), + mod._localNameToFullName_map.keys()) + if not name.startswith('_') + ] + return names + +def processReExports(system:'model.System') -> None: + for mod in system.objectsOfType(model.Module): + exports = getModuleExports(mod) + for imported_name in mod.imports: + local_name = imported_name.name + orgname = imported_name.orgname + orgmodule = imported_name.orgmodule + if local_name != '*' and (not orgname or local_name not in exports): + continue + origin = system.modules.get(orgmodule) or system.allobjects.get(orgmodule) + if isinstance(origin, model.Module): + if local_name != '*': + if orgname: + # only 'import from' statements can be used in re-exporting currently. + _handleReExport(mod, orgname, local_name, origin) + else: + for n in getPublicNames(origin): + if n in exports: + _handleReExport(mod, n, n, origin) + else: + mod.report("cannot resolve origin module of re-exported name :" + f"{'.'.join(imported_name.orgmodule)}.{local_name}", thresh=1) + class ModuleVistor(NodeVisitor): def __init__(self, builder: 'ASTBuilder', module: model.Module): @@ -300,95 +377,41 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None: def _importAll(self, modname: str) -> None: """Handle a C{from import *} statement.""" + ctx = self.builder.current + + if isinstance(ctx, model.Module): + ctx.imports.append(model.Import('*', modname)) 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) + ctx.report(f"import * from unknown {modname}", thresh=1) return - self.builder.current.report(f"import * from {modname}", thresh=1) + ctx.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('_') - ] - - # Fetch names to export. - exports = self._getCurrentModuleExports() + names = getPublicNames(mod) # Add imported names to our module namespace. - assert isinstance(self.builder.current, model.CanContainImportsDocumentable) - _localNameToFullName = self.builder.current._localNameToFullName_map + assert isinstance(ctx, model.CanContainImportsDocumentable) + _localNameToFullName = ctx._localNameToFullName_map expandName = mod.expandName for name in names: - - if self._handleReExport(exports, name, name, mod) is True: - continue - _localNameToFullName[name] = expandName(name) - def _getCurrentModuleExports(self) -> Collection[str]: - # Fetch names to export. - current = self.builder.current - if isinstance(current, model.Module): - exports = current.all - if exports is None: - exports = [] - else: - # Don't export names imported inside classes or functions. - exports = [] - return exports - - def _handleReExport(self, curr_mod_exports:Collection[str], - origin_name:str, as_name:str, - origin_module:model.Module) -> bool: - """ - Move re-exported objects into current module. - - @returns: True if the imported name has been sucessfully re-exported. - """ - # Move re-exported objects into current module. - current = self.builder.current - modname = origin_module.fullName() - if as_name in curr_mod_exports: - # In case of duplicates names, we can't rely on resolveName, - # So we use content.get first to resolve non-alias names. - ob = origin_module.contents.get(origin_name) or origin_module.resolveName(origin_name) - if ob is None: - current.report("cannot resolve re-exported name :" - f'{modname}.{origin_name}', thresh=1) - else: - if origin_module.all is None or origin_name not in origin_module.all: - self.system.msg( - "astbuilder", - "moving %r into %r" % (ob.fullName(), current.fullName()) - ) - # Must be a Module since the exports is set to an empty list if it's not. - assert isinstance(current, model.Module) - ob.reparent(current, as_name) - return True - return False - def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None: """Handle a C{from import } statement.""" # Process the module we're importing from. mod = self.system.getProcessedModule(modname) - # Fetch names to export. - exports = self._getCurrentModuleExports() - current = self.builder.current assert isinstance(current, model.CanContainImportsDocumentable) _localNameToFullName = current._localNameToFullName_map + is_module = isinstance(current, model.Module) for al in names: orgname, asname = al.name, al.asname if asname is None: @@ -398,11 +421,11 @@ def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None: # are processed (getProcessedModule() ignores non-modules). if isinstance(mod, model.Package): self.system.getProcessedModule(f'{modname}.{orgname}') - - if mod is not None and self._handleReExport(exports, orgname, asname, mod) is True: - continue _localNameToFullName[asname] = f'{modname}.{orgname}' + if is_module: + cast(model.Module, + current).imports.append(model.Import(asname, modname, orgname)) def visit_Import(self, node: ast.Import) -> None: """Process an import statement. @@ -417,16 +440,21 @@ 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): + ctx = self.builder.current + if not isinstance(ctx, model.CanContainImportsDocumentable): # processing import statement in odd context return - _localNameToFullName = self.builder.current._localNameToFullName_map + _localNameToFullName = ctx._localNameToFullName_map + is_module = isinstance(ctx, model.Module) 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] _localNameToFullName[asname] = targetname + if is_module: + cast(model.Module, + ctx).imports.append(model.Import(asname, targetname)) def _handleOldSchoolMethodDecoration(self, target: str, expr: Optional[ast.expr]) -> bool: if not isinstance(expr, ast.Call): diff --git a/pydoctor/model.py b/pydoctor/model.py index 8d0d6c2ba..36aee9367 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -20,8 +20,8 @@ from inspect import signature, Signature from pathlib import Path from typing import ( - TYPE_CHECKING, Any, Callable, Collection, Dict, Iterator, List, Mapping, - Optional, Sequence, Set, Tuple, Type, TypeVar, Union, cast, overload + TYPE_CHECKING, Any, Callable, Collection, Dict, Iterable, Iterator, List, Mapping, + Optional, Sequence, Set, Tuple, Type, TypeVar, Union, cast, overload, ) from urllib.parse import quote @@ -111,6 +111,92 @@ class DocumentableKind(Enum): PROPERTY = 150 VARIABLE = 100 +class AllObjectsMap: + """ + Implementation of L{System.allobjects} (read-only). + Proxy that lazily get the documentable having the requested fullname based on L{System.rootobjects} and L{System.modules}. + + Provides mapping-like interface: L{__getitem__}, L{__contains__}, L{get} and L{values}. + + @note: It does not implement C{__iter__} and C{__len__} on purpose. + """ + def __init__(self, rootobjects:Sequence['Module'], modules:Mapping[str, 'Module']) -> None: + self._roots = rootobjects + self._modules = modules + + def __getitem__(self, item: str) -> 'Documentable': + def get(_root_contents:Mapping[str, 'Documentable'], + _names:Iterable[str]) -> Optional['Documentable']: + contents:Mapping[str, 'Documentable'] = _root_contents + root, *parts = _names + ob = None + while root: + ob = contents.get(root) + if ob: + contents = ob.contents + else: + break + if not parts: + break + root, *parts = parts + return ob + + ob = get({r.name:r for r in self._roots}, item.split('.')) + if ob: + return ob + + # support getting modules by name in System.modules + # (also modules with dots in their names) + mod = item.split('.') + names:Tuple[str, ...] = () + module = None + while mod: + maybe_modname = '.'.join(mod) + module = self._modules.get(maybe_modname) + if module: + break + else: + *mod, name = maybe_modname.split('.') + names = (name, ) + names + if module: + if not names: + return module + ob = get(module.contents, names) + if ob: + return ob + + raise KeyError(f'object {item!r} not found in the system') + + def __contains__(self, item: str) -> bool: + try: + self[item] + except KeyError: + return False + return True + + def get(self, item:str) -> Optional['Documentable']: + try: + return self[item] + except KeyError: + return None + + def values(self) -> Iterator['Documentable']: + for r in self._roots: + yield from walk(r) + +def walk(node:'Documentable') -> Iterator['Documentable']: + """ + Recursively yield all descendant nodes in the tree starting at *node* + (including *node* itself), in no specified order. This is useful if you + only want to modify nodes in place and don't care about the context. + """ + from collections import deque + todo = deque([node]) + while todo: + node = todo.popleft() + todo.extend(node.contents.values()) + yield node + class Documentable: """An object that can be documented. @@ -246,27 +332,14 @@ def reparent(self, new_parent: 'Module', new_name: str) -> None: # invariants assumed by various bits of pydoctor # and that are of course not written down anywhere # :/ - self._handle_reparenting_pre() old_parent = self.parent assert isinstance(old_parent, CanContainImportsDocumentable) old_name = self.name self.parent = self.parentMod = new_parent self.name = new_name - self._handle_reparenting_post() del old_parent.contents[old_name] old_parent._localNameToFullName_map[old_name] = self.fullName() new_parent.contents[new_name] = self - self._handle_reparenting_post() - - def _handle_reparenting_pre(self) -> None: - del self.system.allobjects[self.fullName()] - for o in self.contents.values(): - o._handle_reparenting_pre() - - def _handle_reparenting_post(self) -> None: - self.system.allobjects[self.fullName()] = self - for o in self.contents.values(): - o._handle_reparenting_post() def _localNameToFullName(self, name: str) -> str: raise NotImplementedError(self._localNameToFullName) @@ -421,7 +494,18 @@ def isNameDefined(self, name: str) -> bool: return self.module.isNameDefined(name) else: return False + +@attr.s(auto_attribs=True, slots=True) +class Import: + """ + An imported name. + @note: One L{Import} instance is created for each + name bound in the C{import} statement. + """ + name:str + orgmodule:str + orgname:Optional[str]=None class Module(CanContainImportsDocumentable): kind = DocumentableKind.MODULE @@ -457,6 +541,8 @@ def setup(self) -> None: self._docformat: Optional[str] = None + self.imports: List[Import] = [] + def _localNameToFullName(self, name: str) -> str: if name in self.contents: o: Documentable = self.contents[name] @@ -914,8 +1000,9 @@ class System: """ def __init__(self, options: Optional['Options'] = None): - self.allobjects: Dict[str, Documentable] = {} + self.modules: Dict[str, Module] = {} self.rootobjects: List[_ModuleT] = [] + self.allobjects = AllObjectsMap(self.rootobjects, self.modules) self.violations = 0 """The number of docstring problems found. @@ -942,8 +1029,6 @@ def __init__(self, options: Optional['Options'] = None): self.needsnl = False self.once_msgs: Set[Tuple[str, str]] = set() - # We're using the id() of the modules as key, and not the fullName becaue modules can - # be reparented, generating KeyError. self.unprocessed_modules: List[_ModuleT] = [] self.module_count = 0 @@ -1135,16 +1220,22 @@ def privacyClass(self, ob: Documentable) -> PrivacyClass: def addObject(self, obj: Documentable) -> None: """Add C{object} to the system.""" + first = self.allobjects.get(obj.fullName()) + + if isinstance(obj, _ModuleT): + self.modules[obj.fullName()] = obj + elif first: + # we already handled duplication of modules. + self.handleDuplicate(obj, first) + if obj.parent: obj.parent.contents[obj.name] = obj elif isinstance(obj, _ModuleT): self.rootobjects.append(obj) else: raise ValueError(f'Top-level object is not a module: {obj!r}') - - first = self.allobjects.setdefault(obj.fullName(), obj) - if obj is not first: - self.handleDuplicate(obj) + + # if we assume: # @@ -1217,7 +1308,7 @@ def _addUnprocessedModule(self, mod: _ModuleT) -> None: module to the system. """ assert mod.state is ProcessingState.UNPROCESSED - first = self.allobjects.get(mod.fullName()) + first = self.modules.get(mod.fullName()) if first is not None: # At this step of processing only modules exists assert isinstance(first, Module) @@ -1226,7 +1317,7 @@ def _addUnprocessedModule(self, mod: _ModuleT) -> None: self.unprocessed_modules.append(mod) self.addObject(mod) self.progress( - "analyzeModule", len(self.allobjects), + "analyzeModule", len(self.modules), None, "modules and packages discovered") self.module_count += 1 @@ -1249,7 +1340,12 @@ def _handleDuplicateModule(self, first: _ModuleT, dup: _ModuleT) -> None: return else: # Else, the last added module wins - self._remove(first) + def removeModule(m:_ModuleT) -> None: + if m.fullName() in self.modules: + del self.modules[m.fullName()] + for child in (c for c in m.contents.values() if isinstance(c, _ModuleT)): + removeModule(child) + removeModule(first) self.unprocessed_modules.remove(first) self._addUnprocessedModule(dup) @@ -1332,14 +1428,8 @@ def addModuleFromPath(self, path: Path, package: Optional[_PackageT]) -> None: elif suffix in importlib.machinery.SOURCE_SUFFIXES: self.analyzeModule(path, module_name, package) break - - def _remove(self, o: Documentable) -> None: - del self.allobjects[o.fullName()] - oc = list(o.contents.values()) - for c in oc: - self._remove(c) - def handleDuplicate(self, obj: Documentable) -> None: + def handleDuplicate(self, obj: Documentable, first: Documentable) -> None: """ This is called when we see two objects with the same .fullName(), for example:: @@ -1358,17 +1448,14 @@ def meth(self): fullName = obj.fullName() while (fullName + ' ' + str(i)) in self.allobjects: i += 1 - prev = self.allobjects[fullName] - obj.report(f"duplicate {str(prev)}", thresh=1) - self._remove(prev) - prev.name = obj.name + ' ' + str(i) - def readd(o: Documentable) -> None: - self.allobjects[o.fullName()] = o - for c in o.contents.values(): - readd(c) - readd(prev) - self.allobjects[fullName] = obj + obj.report(f"duplicate {str(first)}", thresh=1) + # rename first object + parent = first.parent + assert parent is not None + del parent.contents[first.name] + first.name = obj.name + ' ' + str(i) + parent.contents[first.name] = first def getProcessedModule(self, modname: str) -> Optional[_ModuleT]: mod = self.allobjects.get(modname) @@ -1433,6 +1520,11 @@ def postProcess(self) -> None: without the risk of drawing incorrect conclusions because modules were not fully processed yet. """ + # TODO: it might be better to do the re-exporting after default + # post-processes (zopeinterface post process is designed to be run after + # reparenting); or better implement a sorted based or post processing priority + from pydoctor.astbuilder import processReExports + processReExports(self) # default post-processing includes: # - Processing of subclasses diff --git a/pydoctor/templatewriter/__init__.py b/pydoctor/templatewriter/__init__.py index ffaff07a1..3ae033261 100644 --- a/pydoctor/templatewriter/__init__.py +++ b/pydoctor/templatewriter/__init__.py @@ -39,7 +39,7 @@ def parse_xml(text: str) -> minidom.Document: """ try: # TODO: submit a PR to typeshed to add a return type for parseString() - return cast(minidom.Document, minidom.parseString(text)) + return minidom.parseString(text) except Exception as e: raise ValueError(f"Failed to parse template as XML: {e}") from e diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index a12e74307..759304359 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -1971,7 +1971,7 @@ def test_package_name_clash(systemcls: Type[model.System]) -> None: # The following statement completely overrides module 'mod' and all it's submodules. builder.addModuleString('', 'mod', is_package=True) - + assert len(system.root_names)==1 with pytest.raises(KeyError): system.allobjects['mod.sub'] diff --git a/pydoctor/test/test_packages.py b/pydoctor/test/test_packages.py index fe16a9991..018ee4ab1 100644 --- a/pydoctor/test/test_packages.py +++ b/pydoctor/test/test_packages.py @@ -108,10 +108,18 @@ def test_reparented_module() -> None: assert top.resolveName('module') is top.contents['module'] assert top.resolveName('module.f') is mod.contents['f'] - # The module old name is not in allobjects - assert 'reparented_module.mod' not in system.allobjects + # The module old name is still accessible in allobjects, + # because modules keys are never changed in the mapping of modules. + assert 'reparented_module.mod' in system.allobjects # But can still be resolved with it's old name assert top.resolveName('mod') is top.contents['module'] + # the name 'mod' does not exists anymore in 'reparented_module' + assert 'mod' not in top.contents + + from pydoctor.test.test_templatewriter import getHTMLOf + html = getHTMLOf(top) + assert 'reparented_module.mod.html' not in html + assert 'reparented_module.module.html' in html def test_reparenting_follows_aliases() -> None: """ From b25d2a835db5cbfd47a0d54d64e60292ef88aff7 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 19:29:50 -0400 Subject: [PATCH 03/15] Minor simplifications in AllObjectsMap --- pydoctor/model.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/pydoctor/model.py b/pydoctor/model.py index 36aee9367..66702baf9 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -125,38 +125,36 @@ def __init__(self, rootobjects:Sequence['Module'], modules:Mapping[str, 'Module' self._modules = modules def __getitem__(self, item: str) -> 'Documentable': - def get(_root_contents:Mapping[str, 'Documentable'], - _names:Iterable[str]) -> Optional['Documentable']: - contents:Mapping[str, 'Documentable'] = _root_contents - root, *parts = _names + def get(contents:Mapping[str, 'Documentable'], + path:Sequence[str]) -> Optional['Documentable']: + curr, *parts = path ob = None - while root: - ob = contents.get(root) + while curr: + ob = contents.get(curr) if ob: contents = ob.contents else: break if not parts: break - root, *parts = parts + curr, *parts = parts return ob - - ob = get({r.name:r for r in self._roots}, item.split('.')) + + fullname: Sequence[str] = item.split('.') + ob = get({r.name:r for r in self._roots}, fullname) if ob: return ob # support getting modules by name in System.modules # (also modules with dots in their names) - mod = item.split('.') names:Tuple[str, ...] = () module = None - while mod: - maybe_modname = '.'.join(mod) - module = self._modules.get(maybe_modname) + while fullname: + module = self._modules.get('.'.join(fullname)) if module: break else: - *mod, name = maybe_modname.split('.') + *fullname, name = fullname names = (name, ) + names if module: if not names: From e67f0fe3eb2f9b736677952ec6b0814e7752a85d Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 19:31:44 -0400 Subject: [PATCH 04/15] fix mypy --- pydoctor/test/test_sphinx.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pydoctor/test/test_sphinx.py b/pydoctor/test/test_sphinx.py index d05274c04..d71e0caf5 100644 --- a/pydoctor/test/test_sphinx.py +++ b/pydoctor/test/test_sphinx.py @@ -110,7 +110,7 @@ def test_generate_empty_functional() -> None: @contextmanager def openFileForWriting(path: str) -> Iterator[io.BytesIO]: yield output - inv_writer._openFileForWriting = openFileForWriting # type: ignore[assignment] + inv_writer._openFileForWriting = openFileForWriting # type: ignore inv_writer.generate(subjects=[], basepath='base-path') From 522fbff570a42f75378f8f13be78549b251d5712 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 19:33:56 -0400 Subject: [PATCH 05/15] fix pyflakes --- pydoctor/model.py | 2 +- pydoctor/templatewriter/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pydoctor/model.py b/pydoctor/model.py index 66702baf9..d3b3c87a5 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -20,7 +20,7 @@ from inspect import signature, Signature from pathlib import Path from typing import ( - TYPE_CHECKING, Any, Callable, Collection, Dict, Iterable, Iterator, List, Mapping, + TYPE_CHECKING, Any, Callable, Collection, Dict, Iterator, List, Mapping, Optional, Sequence, Set, Tuple, Type, TypeVar, Union, cast, overload, ) from urllib.parse import quote diff --git a/pydoctor/templatewriter/__init__.py b/pydoctor/templatewriter/__init__.py index 3ae033261..e81da8934 100644 --- a/pydoctor/templatewriter/__init__.py +++ b/pydoctor/templatewriter/__init__.py @@ -1,5 +1,5 @@ """Render pydoctor data as HTML.""" -from typing import Any, Iterable, Iterator, Optional, Union, cast, TYPE_CHECKING +from typing import Any, Iterable, Iterator, Optional, Union, TYPE_CHECKING if TYPE_CHECKING: from typing_extensions import Protocol, runtime_checkable else: From a212d7ef76567818b84675ee4cc1821f65c95f3f Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 22:58:56 -0400 Subject: [PATCH 06/15] use better param name --- pydoctor/astbuilder.py | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index 0c5ebc68b..27ef7377f 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -149,39 +149,37 @@ def extract_final_subscript(annotation: ast.Subscript) -> ast.expr: assert isinstance(ann_slice, ast.expr) return ann_slice -def _handleReExport(mod:'model.Module', +def _handleReExport(new_parent:'model.Module', origin_name:str, as_name:str, origin_module:model.Module) -> None: """ - Move re-exported objects into current module. + Move re-exported objects into module C{new_parent}. """ - # Move re-exported objects into current module. - current = mod modname = origin_module.fullName() # In case of duplicates names, we can't rely on resolveName, # So we use content.get first to resolve non-alias names. ob = origin_module.contents.get(origin_name) or origin_module.resolveName(origin_name) if ob is None: - current.report("cannot resolve re-exported name :" + new_parent.report("cannot resolve re-exported name :" f'{modname}.{origin_name}', thresh=1) else: if origin_module.all is None or origin_name not in origin_module.all: if as_name != ob.name: - current.system.msg( + new_parent.system.msg( "astbuilder", - "moving %r into %r as %r" % (ob.fullName(), current.fullName(), as_name) + "moving %r into %r as %r" % (ob.fullName(), new_parent.fullName(), as_name) ) else: - current.system.msg( + new_parent.system.msg( "astbuilder", - "moving %r into %r" % (ob.fullName(), current.fullName()) + "moving %r into %r" % (ob.fullName(), new_parent.fullName()) ) - ob.reparent(current, as_name) + ob.reparent(new_parent, as_name) else: - current.system.msg( + new_parent.system.msg( "astbuilder", - f"not moving %r into %r, because {origin_name} is present in {modname}.__all__" % (ob.fullName(), current.fullName()), + f"not moving %r into %r, because {origin_name} is present in {modname}.__all__" % (ob.fullName(), new_parent.fullName()), thresh=1 ) From 5885503ab56eb327fc20cdd78cc1f2248574d21a Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 23:41:40 -0400 Subject: [PATCH 07/15] Simplifications --- pydoctor/model.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/pydoctor/model.py b/pydoctor/model.py index d3b3c87a5..210605fbf 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -131,12 +131,9 @@ def get(contents:Mapping[str, 'Documentable'], ob = None while curr: ob = contents.get(curr) - if ob: - contents = ob.contents - else: - break - if not parts: + if not ob or not parts: break + contents = ob.contents curr, *parts = parts return ob @@ -153,9 +150,8 @@ def get(contents:Mapping[str, 'Documentable'], module = self._modules.get('.'.join(fullname)) if module: break - else: - *fullname, name = fullname - names = (name, ) + names + *fullname, name = fullname + names = (name, ) + names if module: if not names: return module From 460eb27e48b65329448b53a7317b4d263aa4915b Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Thu, 13 Jul 2023 23:48:45 -0400 Subject: [PATCH 08/15] Add docstring --- pydoctor/model.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pydoctor/model.py b/pydoctor/model.py index 210605fbf..9554e19b3 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -125,6 +125,10 @@ def __init__(self, rootobjects:Sequence['Module'], modules:Mapping[str, 'Module' self._modules = modules def __getitem__(self, item: str) -> 'Documentable': + """ + Returns the L{Documentable} at the given full dotted name. + This function does not do any name resolving, it only looks into L{Documentable.contents}. + """ def get(contents:Mapping[str, 'Documentable'], path:Sequence[str]) -> Optional['Documentable']: curr, *parts = path From 40d806c79a7c1246b4b405f68bc9f3770124aec0 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Fri, 14 Jul 2023 01:56:29 -0400 Subject: [PATCH 09/15] Better messages --- pydoctor/astbuilder.py | 49 ++++++++++++++++++++++-------------------- pydoctor/model.py | 1 + 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index 27ef7377f..064c49a07 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -151,7 +151,7 @@ def extract_final_subscript(annotation: ast.Subscript) -> ast.expr: def _handleReExport(new_parent:'model.Module', origin_name:str, as_name:str, - origin_module:model.Module) -> None: + origin_module:model.Module, linenumber:int) -> None: """ Move re-exported objects into module C{new_parent}. """ @@ -161,27 +161,24 @@ def _handleReExport(new_parent:'model.Module', # So we use content.get first to resolve non-alias names. ob = origin_module.contents.get(origin_name) or origin_module.resolveName(origin_name) if ob is None: - new_parent.report("cannot resolve re-exported name :" - f'{modname}.{origin_name}', thresh=1) + new_parent.report("cannot resolve re-exported name: " + f'\'{modname}.{origin_name}\'', lineno_offset=linenumber) else: if origin_module.all is None or origin_name not in origin_module.all: if as_name != ob.name: new_parent.system.msg( "astbuilder", - "moving %r into %r as %r" % (ob.fullName(), new_parent.fullName(), as_name) - ) + f"moving {ob.fullName()!r} into {new_parent.fullName()!r} as {as_name!r}") else: new_parent.system.msg( "astbuilder", - "moving %r into %r" % (ob.fullName(), new_parent.fullName()) - ) - ob.reparent(new_parent, as_name) + f"moving {ob.fullName()!r} into {new_parent.fullName()!r}") + ob.reparent(new_parent, as_name) else: new_parent.system.msg( "astbuilder", - f"not moving %r into %r, because {origin_name} is present in {modname}.__all__" % (ob.fullName(), new_parent.fullName()), - thresh=1 - ) + f"not moving {ob.fullName()} into {new_parent.fullName()}, " + f"because {origin_name!r} is already exported in {modname}.__all__") def getModuleExports(mod:'model.Module') -> Collection[str]: # Fetch names to export. @@ -215,14 +212,19 @@ def processReExports(system:'model.System') -> None: if local_name != '*': if orgname: # only 'import from' statements can be used in re-exporting currently. - _handleReExport(mod, orgname, local_name, origin) + _handleReExport(mod, orgname, local_name, origin, + linenumber=imported_name.linenumber) else: for n in getPublicNames(origin): if n in exports: - _handleReExport(mod, n, n, origin) - else: - mod.report("cannot resolve origin module of re-exported name :" - f"{'.'.join(imported_name.orgmodule)}.{local_name}", thresh=1) + _handleReExport(mod, n, n, origin, + linenumber=imported_name.linenumber) + elif orgmodule.split('.', 1)[0] in system.root_names: + msg = f"cannot resolve origin module of re-exported name: {orgname or local_name!r}" + if orgname and local_name!=orgname: + msg += f" as {local_name!r}" + msg += f"from origin module {imported_name.orgmodule!r}" + mod.report(msg, lineno_offset=imported_name.linenumber) class ModuleVistor(NodeVisitor): @@ -369,6 +371,9 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None: assert modname is not None if node.names[0].name == '*': + if isinstance(ctx, model.Module): + ctx.imports.append(model.Import('*', modname, + linenumber=node.lineno)) self._importAll(modname) else: self._importNames(modname, node.names) @@ -376,10 +381,6 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None: def _importAll(self, modname: str) -> None: """Handle a C{from import *} statement.""" ctx = self.builder.current - - if isinstance(ctx, model.Module): - ctx.imports.append(model.Import('*', modname)) - mod = self.system.getProcessedModule(modname) if mod is None: # We don't have any information about the module, so we don't know @@ -422,8 +423,9 @@ def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None: _localNameToFullName[asname] = f'{modname}.{orgname}' if is_module: - cast(model.Module, - current).imports.append(model.Import(asname, modname, orgname)) + cast(model.Module, + current).imports.append(model.Import(asname, modname, + orgname=orgname, linenumber=al.lineno)) def visit_Import(self, node: ast.Import) -> None: """Process an import statement. @@ -452,7 +454,8 @@ def visit_Import(self, node: ast.Import) -> None: _localNameToFullName[asname] = targetname if is_module: cast(model.Module, - ctx).imports.append(model.Import(asname, targetname)) + ctx).imports.append(model.Import(asname, targetname, + linenumber=node.lineno)) def _handleOldSchoolMethodDecoration(self, target: str, expr: Optional[ast.expr]) -> bool: if not isinstance(expr, ast.Call): diff --git a/pydoctor/model.py b/pydoctor/model.py index 9554e19b3..d48dc5792 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -503,6 +503,7 @@ class Import: """ name:str orgmodule:str + linenumber:int orgname:Optional[str]=None class Module(CanContainImportsDocumentable): From 892aa90c350f18751a2c5024fa8e81c65c959577 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Fri, 14 Jul 2023 02:28:57 -0400 Subject: [PATCH 10/15] Fix AttributeError: 'alias' object has no attribute 'lineno' --- pydoctor/astbuilder.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index 064c49a07..3b053d7a0 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -371,24 +371,25 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None: assert modname is not None if node.names[0].name == '*': - if isinstance(ctx, model.Module): - ctx.imports.append(model.Import('*', modname, - linenumber=node.lineno)) - self._importAll(modname) + self._importAll(modname, linenumber=node.lineno) else: - self._importNames(modname, node.names) + self._importNames(modname, node.names, linenumber=node.lineno) - def _importAll(self, modname: str) -> None: + def _importAll(self, modname: str, linenumber:int) -> None: """Handle a C{from import *} statement.""" ctx = self.builder.current + if isinstance(ctx, model.Module): + ctx.imports.append(model.Import('*', modname, linenumber=linenumber)) 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. - ctx.report(f"import * from unknown {modname}", thresh=1) + ctx.report(f"import * from unknown module {modname!r}", thresh=1, lineno_offset=linenumber) return - - ctx.report(f"import * from {modname}", thresh=1) + + if mod.state is model.ProcessingState.PROCESSING: + ctx.report(f"import * from partially processed module {modname!r}", + thresh=1, lineno_offset=linenumber) # Get names to import: use __all__ if available, otherwise take all # names that are not private. @@ -401,7 +402,7 @@ def _importAll(self, modname: str) -> None: for name in names: _localNameToFullName[name] = expandName(name) - def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None: + def _importNames(self, modname: str, names: Iterable[ast.alias], linenumber:int) -> None: """Handle a C{from import } statement.""" # Process the module we're importing from. @@ -425,7 +426,7 @@ def _importNames(self, modname: str, names: Iterable[ast.alias]) -> None: if is_module: cast(model.Module, current).imports.append(model.Import(asname, modname, - orgname=orgname, linenumber=al.lineno)) + orgname=orgname, linenumber=linenumber)) def visit_Import(self, node: ast.Import) -> None: """Process an import statement. From 8b806c6cd13f80b45ccee1c993585c3750c31154 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Sat, 15 Jul 2023 10:19:46 -0400 Subject: [PATCH 11/15] add a test --- pydoctor/test/test_astbuilder.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index 759304359..7cedd450e 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -2413,4 +2413,28 @@ def test_typealias_unstring(systemcls: Type[model.System]) -> None: assert typealias.value with pytest.raises(StopIteration): # there is not Constant nodes in the type alias anymore - next(n for n in ast.walk(typealias.value) if isinstance(n, ast.Constant)) \ No newline at end of file + next(n for n in ast.walk(typealias.value) if isinstance(n, ast.Constant)) + +@systemcls_param +def test_allobjects_mapping_reparented_confusion(systemcls: Type[model.System]) -> None: + src1 = '''\ + class mything: + class stuff: + do = object.__call__ + ''' + mything_src = '''\ + class stuff: + def do(x:int):... + ''' + pack = 'from ._src import mything; __all__=["mything"]' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(src1, '_src', parent_name='pack') + builder.addModuleString(mything_src, 'mything', parent_name='pack') + builder.buildModules() + + assert isinstance(system.allobjects['pack.mything'], model.Class) + assert isinstance(system.allobjects['pack.mything.stuff.do'], model.Attribute) + assert isinstance(system.modules['pack.mything'], model.Module) \ No newline at end of file From 7e4107b2a5eeb8fac908874841b59697479f1cb8 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Sun, 16 Jul 2023 12:53:49 -0400 Subject: [PATCH 12/15] Fix duplicate handling when reparenting --- pydoctor/model.py | 5 ++++- pydoctor/test/test_astbuilder.py | 30 +++++++++++++++++++++++++----- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/pydoctor/model.py b/pydoctor/model.py index d48dc5792..09eb084c7 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -337,7 +337,10 @@ def reparent(self, new_parent: 'Module', new_name: str) -> None: self.name = new_name del old_parent.contents[old_name] old_parent._localNameToFullName_map[old_name] = self.fullName() - new_parent.contents[new_name] = self + new_contents = new_parent.contents + if new_name in new_contents: + self.system.handleDuplicate(self, new_contents[new_name]) + new_contents[new_name] = self def _localNameToFullName(self, name: str) -> str: raise NotImplementedError(self._localNameToFullName) diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index 7cedd450e..6e9302d60 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -2416,14 +2416,19 @@ def test_typealias_unstring(systemcls: Type[model.System]) -> None: next(n for n in ast.walk(typealias.value) if isinstance(n, ast.Constant)) @systemcls_param -def test_allobjects_mapping_reparented_confusion(systemcls: Type[model.System]) -> None: +def test_allobjects_mapping_reparented_confusion(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + When reparenting, it takes care to handle duplicte objects with system.handleDuplicate. + """ src1 = '''\ class mything: + "reparented" class stuff: - do = object.__call__ + do = object() ''' mything_src = '''\ class stuff: + "doc" def do(x:int):... ''' pack = 'from ._src import mything; __all__=["mything"]' @@ -2435,6 +2440,21 @@ def do(x:int):... builder.addModuleString(mything_src, 'mything', parent_name='pack') builder.buildModules() - assert isinstance(system.allobjects['pack.mything'], model.Class) - assert isinstance(system.allobjects['pack.mything.stuff.do'], model.Attribute) - assert isinstance(system.modules['pack.mything'], model.Module) \ No newline at end of file + assert [(o.name,o.kind) for o in + system.allobjects['pack'].contents.values()] == [('_src', model.DocumentableKind.MODULE), + ('mything 0', model.DocumentableKind.MODULE), + ('mything', model.DocumentableKind.CLASS)] + + assert system.allobjects['pack.mything'].docstring == "reparented" + + assert system.allobjects['pack.mything.stuff'].docstring == None + assert system.allobjects['pack.mything.stuff.do'].kind == model.DocumentableKind.CLASS_VARIABLE + + assert system.allobjects['pack.mything 0.stuff'].docstring == "doc" + assert system.allobjects['pack.mything 0.stuff'].kind == model.DocumentableKind.CLASS + assert system.allobjects['pack.mything 0.stuff.do'].kind == model.DocumentableKind.METHOD + + assert capsys.readouterr().out == ( + "moving 'pack._src.mything' into 'pack'\n" + "pack:1: duplicate Module 'pack.mything'\n" + ) \ No newline at end of file From 5cd97f21c3a5bcfc0c671ac29891507dfc4b0798 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Sun, 16 Jul 2023 13:12:30 -0400 Subject: [PATCH 13/15] refactor --- pydoctor/model.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/pydoctor/model.py b/pydoctor/model.py index 09eb084c7..4186ee992 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -1322,7 +1322,13 @@ def _addUnprocessedModule(self, mod: _ModuleT) -> None: "analyzeModule", len(self.modules), None, "modules and packages discovered") self.module_count += 1 - + + def _removeModule(self, m:_ModuleT) -> None: + if m.fullName() in self.modules: + del self.modules[m.fullName()] + for child in (c for c in m.contents.values() if isinstance(c, _ModuleT)): + self._removeModule(child) + def _handleDuplicateModule(self, first: _ModuleT, dup: _ModuleT) -> None: """ This is called when two modules have the same name. @@ -1342,12 +1348,7 @@ def _handleDuplicateModule(self, first: _ModuleT, dup: _ModuleT) -> None: return else: # Else, the last added module wins - def removeModule(m:_ModuleT) -> None: - if m.fullName() in self.modules: - del self.modules[m.fullName()] - for child in (c for c in m.contents.values() if isinstance(c, _ModuleT)): - removeModule(child) - removeModule(first) + self._removeModule(first) self.unprocessed_modules.remove(first) self._addUnprocessedModule(dup) From 14637f4fc0fc46c35e32c3d1c1b82574f6efc2f3 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Sun, 16 Jul 2023 13:13:21 -0400 Subject: [PATCH 14/15] Add a test for warnings --- pydoctor/test/test_astbuilder.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index 6e9302d60..5613430b6 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -2457,4 +2457,29 @@ def do(x:int):... assert capsys.readouterr().out == ( "moving 'pack._src.mything' into 'pack'\n" "pack:1: duplicate Module 'pack.mything'\n" - ) \ No newline at end of file + ) + +@systemcls_param +def test_cannot_resolve_reparented(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + When reparenting, it warns when the reparented target cannot be found + """ + src1 = '''\ + class Cls:... + ''' + mything_src = '''\ + class Slc:... + ''' + pack = 'from ._src2 import Slc;from ._src1 import Cls; __all__=["Cls", "Slc"]' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(src1, '_src0', parent_name='pack') + builder.addModuleString(mything_src, '_src1', parent_name='pack') + builder.buildModules() + + assert list(system.allobjects['pack'].contents) == ['_src0', '_src1'] + + assert capsys.readouterr().out == ("pack:1: cannot resolve origin module of re-exported name: 'Slc'from origin module 'pack._src2'\n" + "pack:1: cannot resolve re-exported name: 'pack._src1.Cls'\n") \ No newline at end of file From d6176032e8da69a8b7e0c07d9ace6bbe88a35368 Mon Sep 17 00:00:00 2001 From: tristanlatr Date: Sun, 16 Jul 2023 15:14:17 -0400 Subject: [PATCH 15/15] better messages for duplicate entries introduces by reparenting --- pydoctor/model.py | 19 ++++++++++++------- pydoctor/test/test_astbuilder.py | 6 +++++- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/pydoctor/model.py b/pydoctor/model.py index 4186ee992..e83d990d0 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -333,15 +333,20 @@ def reparent(self, new_parent: 'Module', new_name: str) -> None: old_parent = self.parent assert isinstance(old_parent, CanContainImportsDocumentable) old_name = self.name - self.parent = self.parentMod = new_parent - self.name = new_name + # delete old object del old_parent.contents[old_name] - old_parent._localNameToFullName_map[old_name] = self.fullName() + # issue warnings new_contents = new_parent.contents if new_name in new_contents: self.system.handleDuplicate(self, new_contents[new_name]) - new_contents[new_name] = self - + self.report(f"introduced by re-exporting {self} into {new_parent}" + '' if new_name==old_name else f' as {new_name!r}', thresh=1) + # do reparenting + self.parent = self.parentMod = new_parent # change parent + self.name = new_name # change name + old_parent._localNameToFullName_map[old_name] = self.fullName() # leave reference + new_contents[new_name] = self # setup parent.contents + def _localNameToFullName(self, name: str) -> str: raise NotImplementedError(self._localNameToFullName) @@ -1441,11 +1446,11 @@ class C: if something: def meth(self): implementation 1 - else: + if something_else: def meth(self): implementation 2 - The default is that the second definition "wins". + The second definition "wins" and object C{first} is renamed. """ i = 0 fullName = obj.fullName() diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index 5613430b6..01da7fdae 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -2453,10 +2453,14 @@ def do(x:int):... assert system.allobjects['pack.mything 0.stuff'].docstring == "doc" assert system.allobjects['pack.mything 0.stuff'].kind == model.DocumentableKind.CLASS assert system.allobjects['pack.mything 0.stuff.do'].kind == model.DocumentableKind.METHOD + + # the module is still accessible in System.modules even if shadowed by reparenting + assert system.modules['pack.mything'].name == 'mything 0' assert capsys.readouterr().out == ( "moving 'pack._src.mything' into 'pack'\n" - "pack:1: duplicate Module 'pack.mything'\n" + "pack._src:1: duplicate Module 'pack.mything'\n" + "pack._src:1: introduced by re-exporting Class 'pack._src.mything' into Package 'pack'\n" ) @systemcls_param