Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 47 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
6f2e374
Add a test
tristanlatr May 23, 2022
0c50517
Fix bug in deprecate.py extension.
tristanlatr May 24, 2022
7d40873
Since the new NodeVisitor class, all the node are not visited as befo…
tristanlatr May 24, 2022
ae6582f
fix tests
tristanlatr May 24, 2022
3bd4407
Move _name_in_override_guard() such that it's covered by the tests.
tristanlatr May 24, 2022
8613422
Fix typo
tristanlatr May 24, 2022
9a02722
Ignore Try.handlers blocks If.orelse blocks in functions/methods.
tristanlatr May 24, 2022
7ad998f
Revert ignoring 'If.orelse' and 'Try.handler' in functions and methods.
tristanlatr May 24, 2022
bd90f2b
Fix mypy
tristanlatr May 24, 2022
39ca9a3
Apply suggestions from code review
tristanlatr May 24, 2022
0ad23b8
Add documentation about the branch priorities
tristanlatr May 24, 2022
a5314c9
Merge branch 'visit-try-orelse-finalbody-and-if-orelse' of github.com…
tristanlatr May 24, 2022
d4fb2be
Update docs/source/codedoc.rst
tristanlatr Jun 11, 2022
d38659e
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Jun 11, 2022
9cfb06a
Use SkipChildren instead of SkipNode in visit_If()
tristanlatr Jul 14, 2022
db51139
Typos
tristanlatr Jul 14, 2022
5e9a6fc
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Nov 12, 2022
8c61bc1
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Jun 5, 2023
312b301
Refactor
tristanlatr Jun 5, 2023
872ec07
Fix ambiguous ref
tristanlatr Jun 5, 2023
505e113
Fix interaction with deprecated extension
tristanlatr Jun 5, 2023
a100a16
Simplify override_guard()
tristanlatr Jun 5, 2023
0ea5b7c
Simplify the visitors
tristanlatr Jun 6, 2023
5a259d2
Simplify the visitor extension system and add test for attrs classes …
tristanlatr Jun 9, 2023
52d0713
Simplifications
tristanlatr Jun 9, 2023
99b8205
refactor _ignore_name()
tristanlatr Jun 9, 2023
77099c5
Update docs/source/codedoc.rst
tristanlatr Jun 10, 2023
2f5cfc9
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Jan 20, 2024
f9022f3
Fix tests for the new constant semantics.
tristanlatr Jan 21, 2024
3d3b730
Merge branch 'visit-try-orelse-finalbody-and-if-orelse' of github.com…
tristanlatr Jan 21, 2024
d393249
Fix one mypy error.
tristanlatr Jan 24, 2024
2dd25e1
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Mar 25, 2024
4c00191
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Jul 6, 2024
1993e51
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Sep 16, 2024
8f82e87
Use two subclass of tree pruning exception in order to also skip exte…
tristanlatr Sep 20, 2024
0650c35
Better document branch priorities.
tristanlatr Sep 20, 2024
8bcfb56
Refactor the visitor extension pattern and ensure the extension funct…
tristanlatr Sep 24, 2024
b3957fc
Fix python directive
tristanlatr Sep 24, 2024
3189f7c
Fix importing wildcard inside a else branch
tristanlatr Sep 24, 2024
9f576de
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Sep 24, 2024
ecfa367
Remove unused import
tristanlatr Sep 24, 2024
6acffaf
update test comments
tristanlatr Sep 25, 2024
e96b927
Add readme entry
tristanlatr Sep 25, 2024
6a542ed
Merge branch 'visit-try-orelse-finalbody-and-if-orelse' of github.com…
tristanlatr Sep 25, 2024
2235f98
Merge branch 'master' into visit-try-orelse-finalbody-and-if-orelse
tristanlatr Sep 25, 2024
fdc2755
Merge branch 'visit-try-orelse-finalbody-and-if-orelse' of github.com…
tristanlatr Sep 25, 2024
0278320
Fix rst
tristanlatr Sep 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 78 additions & 1 deletion pydoctor/astbuilder.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Convert ASTs into L{pydoctor.model.Documentable} instances."""

import ast
import contextlib
import sys
from attr import attrs, attrib
from functools import partial
Expand Down Expand Up @@ -201,7 +202,39 @@ def __init__(self, builder: 'ASTBuilder', module: model.Module):
self.builder = builder
self.system = builder.system
self.module = module
self._override_guard_state: Tuple[bool, model.Documentable, Sequence[str]] = \
(False, cast(model.Documentable, None), [])

@contextlib.contextmanager
def override_guard(self) -> Iterator[None]:
"""
Returns a context manager that will make the builder ignore any extraneous
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A clearer explanation of the definition of "extraneous" would be interesting here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extraneous meaning assignments to name that already exist

Copy link
Contributor Author

@tristanlatr tristanlatr Jun 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can read: "Returns a context manager that will make the builder ignore any new assignments to names that already existed in the same context"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Returns a context manager that will make the builder ignore any extraneous
Returns a context manager that will make the builder ignore any new

assigments to existing names within the same context.
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved

@note: The list of existing names is generated at the moment of
calling the function.
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved
"""
current = self.builder.current
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 = (True, current, self._list_names(current))
yield
self._override_guard_state = ignore_override_init

def _list_names(self, ob: model.Documentable) -> List[str]:
"""
List the names currently defined in a class/module.
"""
names = list(ob.contents)
if isinstance(ob, model.CanContainImportsDocumentable):
names.extend(ob._localNameToFullName_map)
return names

def _name_in_override_guard(self, ob: model.Documentable, name:str) -> bool:
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved
return self._override_guard_state[0] is True \
and self._override_guard_state[1] is ob \
and name in self._override_guard_state[2]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glyph please see this method for the implementation of the override guard.

The override guard is a re-entrant context manager that ensures that follow-up assignments to names that already existed before in the same frame context are simply ignored.


def visit_If(self, node: ast.If) -> None:
if isinstance(node.test, ast.Compare):
Expand All @@ -210,6 +243,28 @@ def visit_If(self, node: ast.If) -> None:
# whatever is declared in them cannot be imported
# and thus is not part of the API
raise self.SkipNode()

def depart_If(self, node: ast.If) -> None:
# At this point the body of the Try node has already been visited
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# At this point the body of the Try node has already been visited
# 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
Expand All @@ -227,6 +282,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._name_in_override_guard(parent, node.name):
raise self.SkipNode()
Copy link
Contributor Author

@tristanlatr tristanlatr May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why this code is required, a module under an if branch is not possible. I also wonder how this code is not marked as unreached by the tests…


rawbases = []
bases = []
Expand Down Expand Up @@ -328,6 +386,11 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None:
def _importAll(self, modname: str) -> None:
"""Handle a C{from <modname> import *} statement."""

# Always ignore import * in override guard
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved
if self._override_guard_state[0]:
self.builder.warning("ignored import * from", modname)
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved
return

mod = self.system.getProcessedModule(modname)
if mod is None:
# We don't have any information about the module, so we don't know
Expand Down Expand Up @@ -420,7 +483,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._name_in_override_guard(current, asname):
continue

if mod is not None and self._handleReExport(exports, orgname, asname, mod) is True:
continue

Expand Down Expand Up @@ -449,9 +516,14 @@ def visit_Import(self, node: ast.Import) -> None:
str(self.builder.current))
return
_localNameToFullName = self.builder.current._localNameToFullName_map
current = self.builder.current
for al in node.names:
fullname, asname = al.name, al.asname
# Ignore in override guard
if self._name_in_override_guard(current, asname or fullname):
continue
if asname is not None:
# Do not create an alias with the same name as value
_localNameToFullName[asname] = fullname


Expand Down Expand Up @@ -729,6 +801,8 @@ def _handleAssignment(self,
if isinstance(targetNode, ast.Name):
target = targetNode.id
scope = self.builder.current
if self._name_in_override_guard(scope, target):
return
if isinstance(scope, model.Module):
self._handleAssignmentInModule(target, annotation, expr, lineno)
elif isinstance(scope, model.Class):
Expand Down Expand Up @@ -788,6 +862,9 @@ def _handleFunctionDef(self,
parent = self.builder.current
if isinstance(parent, model.Function):
raise self.SkipNode()
# Ignore in override guard
if self._name_in_override_guard(parent, node.name):
raise self.SkipNode()

lineno = node.lineno

Expand Down
6 changes: 4 additions & 2 deletions pydoctor/extensions/deprecate.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ def depart_ClassDef(self, node:ast.ClassDef) -> None:
except KeyError:
# Classes inside functions are ignored.
return
assert isinstance(cls, model.Class)
if not isinstance(cls, model.Class):
return
getDeprecated(cls, node.decorator_list)

def depart_FunctionDef(self, node:ast.FunctionDef) -> None:
Expand All @@ -73,7 +74,8 @@ def depart_FunctionDef(self, node:ast.FunctionDef) -> None:
except KeyError:
# Inner functions are ignored.
return
assert isinstance(func, (model.Function, model.Attribute))
if not isinstance(func, (model.Function, model.Attribute)):
return
getDeprecated(func, node.decorator_list)

_incremental_Version_signature = inspect.signature(Version)
Expand Down
227 changes: 227 additions & 0 deletions pydoctor/test/test_astbuilder.py
Original file line number Diff line number Diff line change
Expand Up @@ -2075,3 +2075,230 @@ class j: pass
assert system.allobjects['top._impl'].resolveName('f') == system.allobjects['top'].contents['f']
assert system.allobjects['_impl2'].resolveName('i') == system.allobjects['top'].contents['i']
assert all(n in system.allobjects['top'].contents for n in ['f', 'g', 'h', 'i', 'j'])

@systemcls_param
def test_module_level_attributes_and_aliases(systemcls: Type[model.System]) -> None:
"""
Currently, the first analyzed assigment wins, basically. I believe further logic should be added
such that definitions in the orelse clause of the Try node is processed before the
except handlers. This way could define our aliases both there and in the body of the
Try node and fall back to what's defnied in the handlers if the names doesn't exist yet.
"""
tristanlatr marked this conversation as resolved.
Show resolved Hide resolved
system = systemcls()
builder = system.systemBuilder(system)
builder.addModuleString('''
ssl = 1
''', modname='twisted.internet')
builder.addModuleString('''
try:
from twisted.internet import ssl as _ssl
# The names defined in the body of the if block wins over the
# names defined in the except handler
ssl = _ssl
var = 1
VAR = 1
ALIAS = _ssl
except ImportError:
ssl = None
var = 2
VAR = 2
ALIAS = None
''', modname='mod')
builder.buildModules()
mod = system.allobjects['mod']

# Test alias
assert mod.expandName('ssl')=="twisted.internet.ssl"
assert mod.expandName('_ssl')=="twisted.internet.ssl"
s = mod.resolveName('ssl')
assert isinstance(s, model.Attribute)
assert s.value is not None
assert ast.literal_eval(s.value)==1
assert s.kind == model.DocumentableKind.VARIABLE

# Test variable
assert mod.expandName('var')=="mod.var"
v = mod.resolveName('var')
assert isinstance(v, model.Attribute)
assert v.value is not None
assert ast.literal_eval(v.value)==1
assert v.kind == model.DocumentableKind.VARIABLE

# Test constant
assert mod.expandName('VAR')=="mod.VAR"
V = mod.resolveName('VAR')
assert isinstance(V, model.Attribute)
assert V.value is not None
assert ast.literal_eval(V.value)==1
assert V.kind == model.DocumentableKind.CONSTANT

# Test looks like constant but actually an alias.
assert mod.expandName('ALIAS')=="twisted.internet.ssl"
s = mod.resolveName('ALIAS')
assert isinstance(s, model.Attribute)
assert s.value is not None
assert ast.literal_eval(s.value)==1
assert s.kind == model.DocumentableKind.VARIABLE

@systemcls_param
def test_module_level_attributes_and_aliases_orelse(systemcls: Type[model.System]) -> None:
"""
We visit the orelse body and these names have priority over the names in the except handlers.
"""
system = systemcls()
builder = system.systemBuilder(system)
builder.addModuleString('''
ssl = 1
''', modname='twisted.internet')
builder.addModuleString('''
try:
from twisted.internet import ssl as _ssl
except ImportError:
ssl = None
var = 2
VAR = 2
ALIAS = None
newname = 2
else:
# The names defined in the orelse or finally block wins over the
# names defined in the except handler
ssl = _ssl
var = 1
finally:
VAR = 1
ALIAS = _ssl

if sys.version_info > (3,7):
def func():
'func doc'
class klass:
'klass doc'
var2 = 1
'var2 doc'
else:
# these definition will be ignored since they are
# alreade definied in the body of the If block.
func = None
'not this one'
def klass():
'not this one'
class var2:
'not this one'
''', modname='mod')
builder.buildModules()
mod = system.allobjects['mod']

# Tes newname survives the override guard
assert 'newname' in mod.contents

# Test alias
assert mod.expandName('ssl')=="twisted.internet.ssl"
assert mod.expandName('_ssl')=="twisted.internet.ssl"
s = mod.resolveName('ssl')
assert isinstance(s, model.Attribute)
assert s.value is not None
assert ast.literal_eval(s.value)==1
assert s.kind == model.DocumentableKind.VARIABLE

# Test variable
assert mod.expandName('var')=="mod.var"
v = mod.resolveName('var')
assert isinstance(v, model.Attribute)
assert v.value is not None
assert ast.literal_eval(v.value)==1
assert v.kind == model.DocumentableKind.VARIABLE

# Test constant
assert mod.expandName('VAR')=="mod.VAR"
V = mod.resolveName('VAR')
assert isinstance(V, model.Attribute)
assert V.value is not None
assert ast.literal_eval(V.value)==1
assert V.kind == model.DocumentableKind.CONSTANT

# Test looks like constant but actually an alias.
assert mod.expandName('ALIAS')=="twisted.internet.ssl"
s = mod.resolveName('ALIAS')
assert isinstance(s, model.Attribute)
assert s.value is not None
assert ast.literal_eval(s.value)==1
assert s.kind == model.DocumentableKind.VARIABLE

# Test if override guard
func, klass, var2 = mod.resolveName('func'), mod.resolveName('klass'), mod.resolveName('var2')
assert isinstance(func, model.Function)
assert func.docstring == 'func doc'
assert isinstance(klass, model.Class)
assert klass.docstring == 'klass doc'
assert isinstance(var2, model.Attribute)
assert var2.docstring == 'var2 doc'

@systemcls_param
def test_class_level_attributes_and_aliases_orelse(systemcls: Type[model.System]) -> None:
system = systemcls()
builder = system.systemBuilder(system)
builder.addModuleString('crazy_var=2', modname='crazy')
builder.addModuleString('''
if sys.version_info > (3,0):
thing = object
class klass(thing):
'klass doc'
var2 = 3

# regular import
from crazy import crazy_var as cv
else:
# these imports will be ignored because the names
# have been defined in the body of the If block.
from six import t as thing
import klass
from crazy27 import crazy_var as cv

# Wildcard imports are not processed
# in name override guard context
from crazy import *

# this import is not ignored
from six import seven

# this class is not ignored and will be part of the public docs.
class klassfallback(thing):
'klassfallback doc'
var2 = 1
# this overrides var2
var2 = 2

# this is ignored because the name 'klass'
# has been defined in the body of the If block.
klass = klassfallback
'ignored'

var3 = 1
# this overrides var3
var3 = 2
''', modname='mod')
builder.buildModules()
mod = system.allobjects['mod']
assert isinstance(mod, model.Module)

klass, klassfallback, var2, var3 = \
mod.resolveName('klass'), \
mod.resolveName('klassfallback'), \
mod.resolveName('klassfallback.var2'), \
mod.resolveName('var3')

assert isinstance(klass, model.Class)
assert isinstance(klassfallback, model.Class)
assert isinstance(var2, model.Attribute)
assert isinstance(var3, model.Attribute)

assert klassfallback.docstring == 'klassfallback doc'
assert klass.docstring == 'klass doc'
assert ast.literal_eval(var2.value or '') == 2
assert ast.literal_eval(var3.value or '') == 2

assert mod.expandName('cv') == 'crazy.crazy_var'
assert mod.expandName('thing') == 'object'
assert mod.expandName('seven') == 'six.seven'
assert 'klass' not in mod._localNameToFullName_map
assert 'crazy_var' not in mod._localNameToFullName_map