diff --git a/docs/tests/test_twisted_docs.py b/docs/tests/test_twisted_docs.py index 339802219..2a5af1386 100644 --- a/docs/tests/test_twisted_docs.py +++ b/docs/tests/test_twisted_docs.py @@ -25,10 +25,10 @@ def test_IPAddress_implementations() -> None: assert all(impl in page for impl in show_up), page # Test for https://github.com/twisted/pydoctor/issues/505 -def test_web_template_api() -> None: +def test_some_apis() -> None: """ This test ensures all important members of the twisted.web.template - module are documented at the right place + module are documented at the right place, and other APIs exist as well. """ exists = ['twisted.web.template.Tag.html', @@ -39,7 +39,8 @@ def test_web_template_api() -> None: 'twisted.web.template.TagLoader.html', 'twisted.web.template.XMLString.html', 'twisted.web.template.XMLFile.html', - 'twisted.web.template.Element.html',] + 'twisted.web.template.Element.html', + 'twisted.internet.ssl.DistinguishedName.html'] for e in exists: assert (BASE_DIR / e).exists(), f"{e} not found" diff --git a/pydoctor/astbuilder.py b/pydoctor/astbuilder.py index 3b053d7a0..91a9ff479 100644 --- a/pydoctor/astbuilder.py +++ b/pydoctor/astbuilder.py @@ -1,6 +1,7 @@ """Convert ASTs into L{pydoctor.model.Documentable} instances.""" import ast +from collections import defaultdict import sys from functools import partial @@ -12,6 +13,7 @@ Type, TypeVar, Union, cast ) +import attr import astor from pydoctor import epydoc2stan, model, node2stan, extensions, linker from pydoctor.epydoc.markup._pyval_repr import colorize_inline_pyval @@ -149,36 +151,51 @@ def extract_final_subscript(annotation: ast.Subscript) -> ast.expr: assert isinstance(ann_slice, ast.expr) return ann_slice -def _handleReExport(new_parent:'model.Module', - origin_name:str, as_name:str, - origin_module:model.Module, linenumber:int) -> None: - """ - Move re-exported objects into module C{new_parent}. - """ - modname = origin_module.fullName() - +def _resolveReExportTarget(origin_module:model.Module, origin_name:str, + new_parent:model.Module, linenumber:int) -> Optional[model.Documentable]: # 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: new_parent.report("cannot resolve re-exported name: " - f'\'{modname}.{origin_name}\'', lineno_offset=linenumber) + f'\'{origin_module.fullName()}.{origin_name}\'', lineno_offset=linenumber) + return ob + +def _handleReExport(info:'ReExport', elsewhere:Collection['ReExport']) -> None: + """ + Move re-exported objects into module C{new_parent}. + """ + new_parent = info.new_parent + target = info.target + as_name = info.as_name + target_parent = target.parent + assert isinstance(target_parent, model.Module) + + if as_name != target.name: + new_parent.system.msg( + "astbuilder", + f"moving {target.fullName()!r} into {new_parent.fullName()!r} as {as_name!r}") 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", - f"moving {ob.fullName()!r} into {new_parent.fullName()!r} as {as_name!r}") - else: - new_parent.system.msg( - "astbuilder", - 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 {ob.fullName()} into {new_parent.fullName()}, " - f"because {origin_name!r} is already exported in {modname}.__all__") + new_parent.system.msg( + "astbuilder", + f"moving {target.fullName()!r} into {new_parent.fullName()!r}") + + target_parent.elsewhere_contents[target.name] = target + + for e in elsewhere: + new_parent.system.msg( + "astbuilder", + f"also available at '{e.new_parent.fullName()}.{e.as_name}'") + e.new_parent.elsewhere_contents[e.as_name] = target + + target.reparent(new_parent, as_name) + + # if origin_module.all is None or origin_name not in origin_module.all: + # else: + # new_parent.system.msg( + # "astbuilder", + # f"not moving {target.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. @@ -198,7 +215,35 @@ def getPublicNames(mod:'model.Module') -> Collection[str]: ] return names +@attr.s(auto_attribs=True, slots=True) +class ReExport: + new_parent: model.Module + as_name: str + origin_module: model.Module + target: model.Documentable + + +def _maybeExistingNameOverridesImport(mod:model.Module, local_name:str, + imp:model.Import, target:model.Documentable) -> bool: + if local_name in mod.contents: + existing = mod.contents[local_name] + # The imported name already exists in the locals, we test the linenumbers to + # know whether the import should override the local name. We could do better if + # integrate with better static analysis like def-use chains. + if (not isinstance(existing, model.Module) and # modules are always shadowed by members + mod.contents[local_name].linenumber > imp.linenumber): + mod.report(f"not moving {target.fullName()} into {mod.fullName()}, " + f"because {local_name!r} is defined at line {existing.linenumber}", + lineno_offset=imp.linenumber, + thresh=-1) + return True + return False + def processReExports(system:'model.System') -> None: + # first gather all export infos, clean them up + # and apply them at the end. + reexports: List[ReExport] = [] + for mod in system.objectsOfType(model.Module): exports = getModuleExports(mod) for imported_name in mod.imports: @@ -210,15 +255,26 @@ def processReExports(system:'model.System') -> None: origin = system.modules.get(orgmodule) or system.allobjects.get(orgmodule) if isinstance(origin, model.Module): if local_name != '*': + # only 'import from' statements can be used in re-exporting currently. if orgname: - # only 'import from' statements can be used in re-exporting currently. - _handleReExport(mod, orgname, local_name, origin, - linenumber=imported_name.linenumber) + target = _resolveReExportTarget(origin, orgname, + mod, imported_name.linenumber) + if target: + if _maybeExistingNameOverridesImport(mod, local_name, imported_name, target): + continue + reexports.append( + ReExport(mod, local_name, origin, target) + ) else: for n in getPublicNames(origin): if n in exports: - _handleReExport(mod, n, n, origin, - linenumber=imported_name.linenumber) + target = _resolveReExportTarget(origin, n, mod, imported_name.linenumber) + if target: + if _maybeExistingNameOverridesImport(mod, n, imported_name, target): + continue + reexports.append( + ReExport(mod, n, origin, target) + ) 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: @@ -226,6 +282,26 @@ def processReExports(system:'model.System') -> None: msg += f"from origin module {imported_name.orgmodule!r}" mod.report(msg, lineno_offset=imported_name.linenumber) + exports_per_target:Dict[model.Documentable, List[ReExport]] = defaultdict(list) + for r in reexports: + exports_per_target[r.target].append(r) + + for target, _exports in exports_per_target.items(): + elsewhere = [] + assert len(_exports) > 0 + if len(_exports) > 1: + # when an object has several re-exports, the module with the lowest number + # of dot in it's name is choosen, if there is an equality, the longer local name + # if choosen + # TODO: move this into a system method + # TODO: do not move objects inside a private module + # TODO: do not move objects when they are listed in __all__ of a public module + _exports.sort(key=lambda r:(r.new_parent.fullName().count('.'), -len(r.as_name))) + elsewhere.extend(_exports[1:]) + + reexport = _exports[0] + _handleReExport(reexport, elsewhere) + class ModuleVistor(NodeVisitor): def __init__(self, builder: 'ASTBuilder', module: model.Module): diff --git a/pydoctor/model.py b/pydoctor/model.py index e83d990d0..ac0d38a5e 100644 --- a/pydoctor/model.py +++ b/pydoctor/model.py @@ -549,6 +549,13 @@ def setup(self) -> None: self._docformat: Optional[str] = None self.imports: List[Import] = [] + self.elsewhere_contents: Dict[str, 'Documentable'] = {} + """ + When pydoctor re-export objects, it leaves references to object in this dict + so they can still be listed in childtable of origin modules. This attribute belongs + to the "view model" part of Documentable interface and should only be used to present + links to these objects, not to do any name resolving. + """ def _localNameToFullName(self, name: str) -> str: if name in self.contents: diff --git a/pydoctor/templatewriter/pages/__init__.py b/pydoctor/templatewriter/pages/__init__.py index 272239605..6d73fbffa 100644 --- a/pydoctor/templatewriter/pages/__init__.py +++ b/pydoctor/templatewriter/pages/__init__.py @@ -1,7 +1,8 @@ """The classes that turn L{Documentable} instances into objects we can render.""" +from itertools import chain from typing import ( - TYPE_CHECKING, Dict, Iterator, List, Optional, Mapping, Sequence, + TYPE_CHECKING, Callable, Dict, Iterator, List, Optional, Mapping, Sequence, Tuple, Type, Union ) import ast @@ -296,11 +297,20 @@ def extras(self) -> List["Flattenable"]: def docstring(self) -> "Flattenable": return self.docgetter.get(self.ob) + + def _childtable_objects_order(self, + v:Union[model.Documentable, Tuple[str, model.Documentable]]) -> Tuple[int, int, str]: + if isinstance(v, model.Documentable): + return util.objects_order(v) + else: + name, o = v + i,j,_ = util.objects_order(o) + return (i,j, f'{self.ob.fullName()}.{name}'.lower()) - def children(self) -> Sequence[model.Documentable]: + def children(self) -> Sequence[Union[model.Documentable, Tuple[str, model.Documentable]]]: return sorted( (o for o in self.ob.contents.values() if o.isVisible), - key=util.objects_order) + key=self._childtable_objects_order) def packageInitTable(self) -> "Flattenable": return () @@ -380,7 +390,6 @@ def slot_map(self) -> Dict[str, "Flattenable"]: ) return slot_map - class ModulePage(CommonPage): ob: model.Module @@ -393,17 +402,35 @@ def extras(self) -> List["Flattenable"]: r.extend(super().extras()) return r + + def _iter_reexported_members(self, predicate: Optional[Callable[[model.Documentable], bool]]=None) -> Iterator[Tuple[str, model.Documentable]]: + if not predicate: + predicate = lambda v:True + return ((n,o) for n,o in self.ob.elsewhere_contents.items() if o.isVisible and predicate(o)) + def children(self) -> Sequence[Union[model.Documentable, Tuple[str, model.Documentable]]]: + return sorted(chain( + super().children(), self._iter_reexported_members()), + key=self._childtable_objects_order) -class PackagePage(ModulePage): - def children(self) -> Sequence[model.Documentable]: - return sorted(self.ob.submodules(), key=objects_order) - def packageInitTable(self) -> "Flattenable": - children = sorted( - (o for o in self.ob.contents.values() +class PackagePage(ModulePage): + def children(self) -> Sequence[Union[model.Documentable, Tuple[str, model.Documentable]]]: + return sorted(chain(self.ob.submodules(), + self._iter_reexported_members( + predicate=lambda o: isinstance(o, model.Module))), + key=self._childtable_objects_order) + + def initTableChildren(self) -> Sequence[Union[model.Documentable, Tuple[str, model.Documentable]]]: + return sorted( + chain((o for o in self.ob.contents.values() if not isinstance(o, model.Module) and o.isVisible), - key=util.objects_order) + self._iter_reexported_members( + predicate=lambda o: not isinstance(o, model.Module))), + key=self._childtable_objects_order) + + def packageInitTable(self) -> "Flattenable": + children = self.initTableChildren() if children: loader = ChildTable.lookup_loader(self.template_lookup) return [ diff --git a/pydoctor/templatewriter/pages/table.py b/pydoctor/templatewriter/pages/table.py index 0099eb7df..76e088d9b 100644 --- a/pydoctor/templatewriter/pages/table.py +++ b/pydoctor/templatewriter/pages/table.py @@ -1,10 +1,10 @@ -from typing import TYPE_CHECKING, Collection +from typing import TYPE_CHECKING, Collection, Optional, Tuple, Union from twisted.web.iweb import ITemplateLoader from twisted.web.template import Element, Tag, TagLoader, renderer, tags from pydoctor import epydoc2stan -from pydoctor.model import Documentable, Function +from pydoctor.model import Documentable, Function, Class from pydoctor.templatewriter import TemplateElement, util if TYPE_CHECKING: @@ -18,16 +18,18 @@ def __init__(self, docgetter: util.DocGetter, ob: Documentable, child: Documentable, + as_name:Optional[str] ): super().__init__(loader) self.docgetter = docgetter self.ob = ob self.child = child + self.as_name = as_name @renderer def class_(self, request: object, tag: Tag) -> "Flattenable": class_ = util.css_class(self.child) - if self.child.parent is not self.ob: + if isinstance(self.ob, Class) and self.child.parent is not self.ob: class_ = 'base' + class_ return class_ @@ -47,8 +49,9 @@ def kind(self, request: object, tag: Tag) -> Tag: @renderer def name(self, request: object, tag: Tag) -> Tag: return tag.clear()(tags.code( - epydoc2stan.taglink(self.child, self.ob.url, epydoc2stan.insert_break_points(self.child.name)) - )) + epydoc2stan.taglink(self.child, self.ob.url, + epydoc2stan.insert_break_points( + self.as_name or self.child.name)))) @renderer def summaryDoc(self, request: object, tag: Tag) -> Tag: @@ -64,7 +67,7 @@ class ChildTable(TemplateElement): def __init__(self, docgetter: util.DocGetter, ob: Documentable, - children: Collection[Documentable], + children: Collection[Union[Documentable, Tuple[str, Documentable]]], loader: ITemplateLoader, ): super().__init__(loader) @@ -85,7 +88,8 @@ def rows(self, request: object, tag: Tag) -> "Flattenable": TagLoader(tag), self.docgetter, self.ob, - child) + child=child if isinstance(child, Documentable) else child[1], + as_name=None if isinstance(child, Documentable) else child[0]) for child in self.children - if child.isVisible - ] + if (child if isinstance(child, Documentable) else child[1]).isVisible + ] diff --git a/pydoctor/test/test_astbuilder.py b/pydoctor/test/test_astbuilder.py index 01da7fdae..b13d8f4ce 100644 --- a/pydoctor/test/test_astbuilder.py +++ b/pydoctor/test/test_astbuilder.py @@ -2486,4 +2486,114 @@ class Slc:... 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 + "pack:1: cannot resolve re-exported name: 'pack._src1.Cls'\n") + +@systemcls_param +def test_reparenting_from_module_that_defines__all__(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + Even if a module defined it's own __all__ attribute, we can reparent it's direct children to a new module. + """ + src = '''\ + class cls:... + __all__ = ['cls'] + ''' + pack = 'from ._src import cls; __all__=["cls"]' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(src, '_src', parent_name='pack') + builder.buildModules() + assert capsys.readouterr().out == "moving 'pack._src.cls' into 'pack'\n" + assert system.allobjects['pack.cls'] is system.allobjects['pack._src'].elsewhere_contents['cls'] # type:ignore + +@systemcls_param +def test_do_not_reparent_to_existing_name(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + Pydoctor will not re-export a name that is shadowed by a local by the same name. + """ + src1 = '''\ + class Cls:... + ''' + src2 = '''\ + class Slc:... + ''' + pack = '''\ + class Slc:... + from ._src1 import Slc + from ._src import Cls + class Cls:... + __all__=["Cls", "Slc"] + ''' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(src1, '_src', parent_name='pack') + builder.addModuleString(src2, '_src1', parent_name='pack') + builder.buildModules() + + assert capsys.readouterr().out == ("pack:3: not moving pack._src.Cls into pack, because 'Cls' is defined at line 4\n" + "moving 'pack._src1.Slc' into 'pack'\n" + "pack._src1:1: duplicate Class 'pack.Slc'\n" + "pack._src1:1: introduced by re-exporting Class 'pack._src1.Slc' into Package 'pack'\n") + + assert system.allobjects['pack.Slc'] is system.allobjects['pack._src1'].elsewhere_contents['Slc'] # type:ignore + +@systemcls_param +def test_multiple_re_exports(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + Pydoctor will re-export a name to the module with the lowest amount of dots in it's fullname. + """ + src = '''\ + class Cls:... + ''' + subpack = '''\ + from pack.subpack.src import Cls + __all__=['Cls'] + ''' + pack = '''\ + from pack.subpack import Cls + __all__=["Cls"] + ''' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(subpack, 'subpack', is_package=True, parent_name='pack') + builder.addModuleString(src, 'src', parent_name='pack.subpack') + builder.buildModules() + + assert capsys.readouterr().out == ("moving 'pack.subpack.src.Cls' into 'pack'\n" + "also available at 'pack.subpack.Cls'\n") + + assert system.allobjects['pack.Cls'] is system.allobjects['pack.subpack'].elsewhere_contents['Cls'] # type:ignore + assert system.allobjects['pack.Cls'] is system.allobjects['pack.subpack.src'].elsewhere_contents['Cls'] # type:ignore + +@systemcls_param +def test_multiple_re_exports_alias(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + The case of twisted.internet.ssl.DistinguishedName/DN + """ + src = '''\ + class DistinguishedName:... + DN = DistinguishedName + ''' + subpack = '' + pack = ''' + from pack.subpack.src import DN, DistinguishedName as DisName + __all__=['DN', 'DisName'] + ''' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(subpack, 'subpack', is_package=True, parent_name='pack') + builder.addModuleString(src, 'src', parent_name='pack.subpack') + builder.buildModules() + + assert capsys.readouterr().out == ("moving 'pack.subpack.src.DistinguishedName' into 'pack' as 'DisName'\n" + "also available at 'pack.DN'\n") + + assert system.allobjects['pack.DisName'] is system.allobjects['pack'].elsewhere_contents['DN'] # type:ignore + assert system.allobjects['pack.DisName'] is system.allobjects['pack.subpack.src'].elsewhere_contents['DistinguishedName'] # type:ignore \ No newline at end of file diff --git a/pydoctor/test/test_packages.py b/pydoctor/test/test_packages.py index 018ee4ab1..ae7f82959 100644 --- a/pydoctor/test/test_packages.py +++ b/pydoctor/test/test_packages.py @@ -57,8 +57,10 @@ def test_allgames() -> None: assert isinstance(mod1, model.Module) mod2 = system.allobjects['allgames.mod2'] assert isinstance(mod2, model.Module) - # InSourceAll is not moved into mod2, but NotInSourceAll is. - assert 'InSourceAll' in mod1.contents + # changed in 2023 + # InSourceAll is moved into mod2 as well as NotInSourceAll. + assert 'InSourceAll' not in mod1.contents + assert 'InSourceAll' in mod2.contents assert 'NotInSourceAll' in mod2.contents # Source paths must be unaffected by the move, so that error messages # point to the right source code. diff --git a/pydoctor/test/test_templatewriter.py b/pydoctor/test/test_templatewriter.py index 64a331474..227b1c508 100644 --- a/pydoctor/test/test_templatewriter.py +++ b/pydoctor/test/test_templatewriter.py @@ -13,6 +13,7 @@ TemplateLookup, Template, HtmlTemplate, UnsupportedTemplateVersion, OverrideTemplateNotAllowed) +from pydoctor.templatewriter.pages import PackagePage, ModulePage from pydoctor.templatewriter.pages.table import ChildTable from pydoctor.templatewriter.pages.attributechild import AttributeChild from pydoctor.templatewriter.summary import isClassNodePrivate, isPrivate, moduleSummary, ClassIndexPage @@ -816,3 +817,62 @@ class Stuff(socket): index = flatten(ClassIndexPage(mod.system, TemplateLookup(template_dir))) assert 'href="https://docs.python.org/3/library/socket.html#socket.socket"' in index +def test_multiple_re_exports_documented_elsewhere_renders() -> None: + """ + Pydoctor will leave links from the origin module. + """ + src = '''\ + class Cls:... + ''' + subpack = '''\ + from pack.subpack.src import Cls + __all__=['Cls'] + ''' + pack = '''\ + from pack.subpack import Cls + __all__=["Cls"] + ''' + + system = model.System() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString(subpack, 'subpack', is_package=True, parent_name='pack') + builder.addModuleString(src, 'src', parent_name='pack.subpack') + builder.buildModules() + + subpackpage = PackagePage(system.allobjects['pack.subpack'], TemplateLookup(template_dir)) + srcpage = ModulePage(system.allobjects['pack.subpack.src'], TemplateLookup(template_dir)) + assert len(subpackpage.children())==1 + assert ('Cls', system.allobjects['pack.Cls']) in subpackpage.initTableChildren() + assert ('Cls', system.allobjects['pack.Cls']) in srcpage.children() + + assert system.allobjects['pack.Cls'].url in flatten(subpackpage) + assert system.allobjects['pack.Cls'].url in flatten(srcpage) + +@systemcls_param +def test_multiple_re_exports_alias_renders_asname(systemcls: Type[model.System], capsys:CapSys) -> None: + """ + The case of twisted.internet.ssl.DistinguishedName/DN + """ + src = '''\ + class DistinguishedName:... + DN = DistinguishedName + ''' + pack = ''' + from pack.subpack.src import DN, DistinguishedName + __all__=['DN', 'DistinguishedName'] + ''' + + system = systemcls() + builder = system.systemBuilder(system) + builder.addModuleString(pack, 'pack', is_package=True) + builder.addModuleString('', 'subpack', is_package=True, parent_name='pack') + builder.addModuleString(src, 'src', parent_name='pack.subpack') + builder.buildModules() + + subpackpage = PackagePage(system.allobjects['pack'], TemplateLookup(template_dir)) + html = flatten(subpackpage) + + assert system.allobjects['pack.DistinguishedName'].url in html + assert 'DN' in html + \ No newline at end of file