Skip to content

Ensure the *_fields attributes of __init__/postinit are cohesive #1218

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

Closed
wants to merge 15 commits into from
Closed
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ What's New in astroid 2.8.4?
============================
Release date: TBA

* Fix incosistency between ``NodeNG`` subclass ``*_field`` definitions and
``__init__``/``postinit``. A ``DeprecationWarning`` will be emitted for code
which provides an argument to the now-incorrect method (generally the
parameters have been moved from ``postinit`` to ``__init__``).


What's New in astroid 2.8.3?
Expand Down
40 changes: 39 additions & 1 deletion astroid/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
)
):
warnings.warn(
f"'{arg}' will be a required attribute for "
f"'{arg}' will be a required argument for "
f"'{args[0].__class__.__qualname__}.{func.__name__}' in astroid {astroid_version} "
f"('{arg}' should be of type: '{type_annotation}')",
DeprecationWarning,
Expand All @@ -207,3 +207,41 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
return wrapper

return deco


def deprecate_arguments(
*arguments: str, hint: str
) -> Callable[[Callable[P, R]], Callable[P, R]]:
"""Decorator which emits a DeprecationWarning if any arguments specified
are provided.
"""

def deco(func: Callable[P, R]) -> Callable[P, R]:
"""Decorator function."""

parameter_names = inspect.signature(func).parameters.keys()
for arg in arguments:
if arg not in parameter_names:
raise ValueError(f"Can't find argument '{arg}'")
params = list(parameter_names)
func.deprecated_args = arguments

@functools.wraps(func)
def wrapper(*args: P.args, **kwargs: P.kwargs) -> R:
"""Emit DeprecationWarnings if conditions are met."""

for arg in arguments:
index = params.index(arg)
if arg in kwargs or len(args) > index:
warnings.warn(
f"'{arg}' is a deprecated argument for "
f"'{args[0].__class__.__qualname__}.{func.__name__}' "
"and will be removed in a future version of astroid."
f"({hint})",
DeprecationWarning,
)
return func(*args, **kwargs)

return wrapper

return deco
58 changes: 40 additions & 18 deletions astroid/nodes/node_classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,8 @@ def __init__(
vararg: Optional[str] = None,
kwarg: Optional[str] = None,
parent: Optional[NodeNG] = None,
lineno: Optional[int] = None,
col_offset: Optional[int] = None,
Comment on lines +755 to +756
Copy link
Member

Choose a reason for hiding this comment

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

The ast node arguments doesn't have lineno or col_offset information. IMO it doesn't make sense to add them. https://docs.python.org/3.10/library/ast.html

) -> None:
"""
:param vararg: The name of the variable length arguments.
Expand All @@ -760,7 +762,7 @@ def __init__(

:param parent: The parent node in the syntax tree.
"""
super().__init__(parent=parent)
super().__init__(lineno=lineno, col_offset=col_offset, parent=parent)

self.vararg: Optional[str] = vararg # can be None
"""The name of the variable length arguments."""
Expand Down Expand Up @@ -1284,6 +1286,7 @@ def __init__(
lineno: Optional[int] = None,
col_offset: Optional[int] = None,
parent: Optional[NodeNG] = None,
simple: Optional[int] = None,
) -> None:
"""
:param lineno: The line that this node appears on in the source code.
Expand All @@ -1302,16 +1305,17 @@ def __init__(
self.value: Optional[NodeNG] = None # can be None
"""The value being assigned to the variables."""

self.simple: Optional[int] = None
self.simple: Optional[int] = simple
"""Whether :attr:`target` is a pure name or a complex statement."""

super().__init__(lineno=lineno, col_offset=col_offset, parent=parent)

@decorators.deprecate_arguments("simple", hint="Pass it to __init__ instead")
def postinit(
self,
target: NodeNG,
annotation: NodeNG,
simple: int,
simple: Optional[int] = None,
value: Optional[NodeNG] = None,
) -> None:
"""Do some setup after initialisation.
Expand All @@ -1328,7 +1332,7 @@ def postinit(
self.target = target
self.annotation = annotation
self.value = value
self.simple = simple
self.simple = simple or self.simple
Copy link
Member

Choose a reason for hiding this comment

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

This can easily break. simple is a boolean integer so either 0 or 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this was a forehead-slapper.


def get_children(self):
yield self.target
Expand Down Expand Up @@ -1759,7 +1763,13 @@ class Comprehension(NodeNG):
optional_assign = True
"""Whether this node optionally assigns a variable."""

def __init__(self, parent: Optional[NodeNG] = None) -> None:
def __init__(
self,
parent: Optional[NodeNG] = None,
lineno: Optional[int] = None,
col_offset: Optional[int] = None,
Comment on lines +1769 to +1770
Copy link
Member

Choose a reason for hiding this comment

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

Same here, the ast Comprehension node doesn't have lineno or col_offset information.
https://docs.python.org/3.10/library/ast.html

is_async: Optional[bool] = None,
) -> None:
"""
:param parent: The parent node in the syntax tree.
"""
Expand All @@ -1772,18 +1782,19 @@ def __init__(self, parent: Optional[NodeNG] = None) -> None:
self.ifs: typing.List[NodeNG] = []
"""The contents of any if statements that filter the comprehension."""

self.is_async: Optional[bool] = None
self.is_async: Optional[bool] = is_async
"""Whether this is an asynchronous comprehension or not."""

super().__init__(parent=parent)
super().__init__(lineno=lineno, col_offset=col_offset, parent=parent)

# pylint: disable=redefined-builtin; same name as builtin ast module.
@decorators.deprecate_arguments("is_async", hint="Pass to __init__ instead")
def postinit(
self,
target: Optional[NodeNG] = None,
iter: Optional[NodeNG] = None,
ifs: Optional[typing.List[NodeNG]] = None,
is_async: Optional[bool] = None,
is_async: Optional[bool] = None, # @TODO: Deprecate and remove
) -> None:
"""Do some setup after initialisation.

Expand All @@ -1800,7 +1811,7 @@ def postinit(
self.iter = iter
if ifs is not None:
self.ifs = ifs
self.is_async = is_async
self.is_async = is_async or self.is_async
Copy link
Member

Choose a reason for hiding this comment

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

Can break too!


def assign_type(self):
"""The type of assignment that this node performs.
Expand Down Expand Up @@ -1846,7 +1857,7 @@ class Const(mixins.NoChildrenMixin, NodeNG, Instance):
<Const.bytes l.1 at 0x7f23b2e35a20>]
"""

_other_fields = ("value",)
_other_fields = ("value", "kind")

def __init__(
self,
Expand Down Expand Up @@ -2630,7 +2641,7 @@ class ImportFrom(mixins.NoChildrenMixin, mixins.ImportFromMixin, Statement):
<ImportFrom l.1 at 0x7f23b2e415c0>
"""

_other_fields = ("modname", "names", "level")
_other_fields = ("fromname", "names", "level")

def __init__(
self,
Expand All @@ -2655,7 +2666,8 @@ def __init__(

:param parent: The parent node in the syntax tree.
"""
self.modname: Optional[str] = fromname # can be None
self.fromname: Optional[str] = fromname # can be None
self.modname = self.fromname # For backwards compatibility
Comment on lines +2669 to +2670
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be part of this PR. IMO it's not worth it to do this change in isolation, there is no really need for it.

"""The module that is being imported from.

This is ``None`` for relative imports.
Expand Down Expand Up @@ -4076,6 +4088,7 @@ class FormattedValue(NodeNG):
"""

_astroid_fields = ("value", "format_spec")
_other_other_fields = ("conversion",)
Copy link
Member

Choose a reason for hiding this comment

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

Why should this be in _other_other_fields and not _other_fields? It's just an optional integer.


def __init__(
self,
Expand Down Expand Up @@ -4252,7 +4265,7 @@ class EvaluatedObject(NodeNG):

name = "EvaluatedObject"
_astroid_fields = ("original",)
_other_fields = ("value",)
_other_other_fields = ("value",)
Copy link
Member

Choose a reason for hiding this comment

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

Why here?


def __init__(
self, original: NodeNG, value: typing.Union[NodeNG, util.Uninferable]
Expand Down Expand Up @@ -4333,11 +4346,17 @@ class MatchCase(mixins.MultiLineBlockMixin, NodeNG):
_astroid_fields = ("pattern", "guard", "body")
_multi_line_block_fields = ("body",)

def __init__(self, *, parent: Optional[NodeNG] = None) -> None:
def __init__(
self,
*,
lineno: Optional[int] = None,
col_offset: Optional[int] = None,
Comment on lines +4352 to +4353
Copy link
Member

Choose a reason for hiding this comment

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

MatchCase doesn't have lineno or col_offset information.

parent: Optional[NodeNG] = None,
) -> None:
self.pattern: Pattern
self.guard: Optional[NodeNG]
self.body: typing.List[NodeNG]
super().__init__(parent=parent)
super().__init__(lineno=lineno, col_offset=col_offset, parent=parent)

def postinit(
self,
Expand Down Expand Up @@ -4519,24 +4538,27 @@ def __init__(
lineno: Optional[int] = None,
col_offset: Optional[int] = None,
parent: Optional[NodeNG] = None,
*,
kwd_attrs: typing.Optional[typing.List[str]] = None,
) -> None:
self.cls: NodeNG
self.patterns: typing.List[Pattern]
self.kwd_attrs: typing.List[str]
self.kwd_attrs = kwd_attrs or []
self.kwd_patterns: typing.List[Pattern]
super().__init__(lineno=lineno, col_offset=col_offset, parent=parent)

@decorators.deprecate_arguments("kwd_attrs", hint="Pass it to __init__ instead")
def postinit(
self,
*,
cls: NodeNG,
patterns: typing.List[Pattern],
kwd_attrs: typing.List[str],
kwd_patterns: typing.List[Pattern],
kwd_attrs: typing.Optional[typing.List[str]] = None,
Comment on lines -4534 to +4557
Copy link
Member

Choose a reason for hiding this comment

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

IMO it does make more sense to keep kwd_attrs and kwd_patterns together in postinit. No need to move one to __init__.

) -> None:
self.cls = cls
self.patterns = patterns
self.kwd_attrs = kwd_attrs
self.kwd_attrs = kwd_attrs or self.kwd_attrs or []
self.kwd_patterns = kwd_patterns


Expand Down
2 changes: 2 additions & 0 deletions astroid/nodes/node_ng.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,8 @@ def __str__(self):
result = []
for field in self._other_fields + self._astroid_fields:
value = getattr(self, field)
if value is None:
continue
width = 80 - len(field) - alignment
lines = pprint.pformat(value, indent=2, width=width).splitlines(True)

Expand Down
12 changes: 7 additions & 5 deletions astroid/nodes/scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,7 @@ def __init__(
package=None,
parent=None,
pure_python=True,
future_imports=None,
):
"""
:param name: The name of the module.
Expand All @@ -496,6 +497,9 @@ def __init__(

:param pure_python: Whether the ast was built from source.
:type pure_python: bool or None

:param future_imports: The imports from ``__future__``.
:type future_imports: Optional[Set[str]]
"""
self.name = name
self.doc = doc
Expand All @@ -514,7 +518,7 @@ def __init__(

:type: list(NodeNG) or None
"""
self.future_imports = set()
self.future_imports = future_imports or set()

# pylint: enable=redefined-builtin

Expand Down Expand Up @@ -1393,7 +1397,6 @@ class FunctionDef(mixins.MultiLineBlockMixin, node_classes.Statement, Lambda):
_other_fields = ("name", "doc")
_other_other_fields = (
"locals",
"_type",
"type_comment_returns",
"type_comment_args",
)
Expand Down Expand Up @@ -2000,7 +2003,7 @@ def my_meth(self, arg):
),
)
_other_fields = ("name", "doc")
_other_other_fields = ("locals", "_newstyle")
_other_other_fields = ("locals", "_newstyle", "_metaclass")
_newstyle = None

def __init__(self, name=None, doc=None, lineno=None, col_offset=None, parent=None):
Expand Down Expand Up @@ -2105,8 +2108,7 @@ def postinit(
:param keywords: The keywords given to the class definition.
:type keywords: list(Keyword) or None
"""
if keywords is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This was a very strange way to do it.

Copy link
Member

Choose a reason for hiding this comment

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

What is strange about it? The default for self.keywords is an empty list. If I remember correctly, this change was added recently to fix a crash in pylint.

Copy link
Member

Choose a reason for hiding this comment

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

Well usually you use if keyword is None: to apply the default value when the default value is a mutable like [] in order to avoid the problem of mutable default argument being function attribute, so doing the opposite where you initialize only if it's not None "feel" wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See the relevant PR for this change here:
#1181

self.keywords = keywords
self.keywords = keywords
self.bases = bases
self.body = body
self.decorators = decorators
Expand Down
2 changes: 2 additions & 0 deletions astroid/objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,8 @@ def qname(self):
class Property(scoped_nodes.FunctionDef):
"""Class representing a Python property"""

_other_fields = scoped_nodes.FunctionDef._other_fields + ("function",)

def __init__(
self, function, name=None, doc=None, lineno=None, col_offset=None, parent=None
):
Expand Down
16 changes: 10 additions & 6 deletions astroid/rebuilder.py
Original file line number Diff line number Diff line change
Expand Up @@ -973,11 +973,12 @@ def visit_assign(self, node: "ast.Assign", parent: NodeNG) -> nodes.Assign:

def visit_annassign(self, node: "ast.AnnAssign", parent: NodeNG) -> nodes.AnnAssign:
"""visit an AnnAssign node by returning a fresh instance of it"""
newnode = nodes.AnnAssign(node.lineno, node.col_offset, parent)
newnode = nodes.AnnAssign(
node.lineno, node.col_offset, parent, simple=node.simple
)
newnode.postinit(
target=self.visit(node.target, newnode),
annotation=self.visit(node.annotation, newnode),
simple=node.simple,
value=self.visit(node.value, newnode),
)
return newnode
Expand Down Expand Up @@ -1112,12 +1113,11 @@ def visit_comprehension(
self, node: "ast.comprehension", parent: NodeNG
) -> nodes.Comprehension:
"""visit a Comprehension node by returning a fresh instance of it"""
newnode = nodes.Comprehension(parent)
newnode = nodes.Comprehension(parent, is_async=bool(node.is_async))
newnode.postinit(
self.visit(node.target, newnode),
self.visit(node.iter, newnode),
[self.visit(child, newnode) for child in node.ifs],
bool(node.is_async),
)
return newnode

Expand Down Expand Up @@ -1788,11 +1788,15 @@ def visit_matchmapping(
def visit_matchclass(
self, node: "ast.MatchClass", parent: NodeNG
) -> nodes.MatchClass:
newnode = nodes.MatchClass(node.lineno, node.col_offset, parent)
newnode = nodes.MatchClass(
node.lineno,
node.col_offset,
parent,
kwd_attrs=node.kwd_attrs,
)
newnode.postinit(
cls=self.visit(node.cls, newnode),
patterns=[self.visit(pattern, newnode) for pattern in node.patterns],
kwd_attrs=node.kwd_attrs,
kwd_patterns=[
self.visit(pattern, newnode) for pattern in node.kwd_patterns
],
Expand Down
Loading