Skip to content

Add typing to NodeNG.statement #1217

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

Merged
merged 24 commits into from
Nov 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d8347f0
Add typing to ``NodeNG.statement``
DanielNoord Oct 22, 2021
f9fe68d
Rewrite to use ``StatementMissing``
DanielNoord Nov 2, 2021
7c47926
Merge branch 'main' of https://github.com/PyCQA/astroid into statement
DanielNoord Nov 2, 2021
b6373e3
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 2, 2021
19c5ed4
Add ``future`` parameter to ``NodeNG.statement()`` and add typing
DanielNoord Nov 3, 2021
baef395
Merge branch 'statement' of https://github.com/DanielNoord/astroid in…
DanielNoord Nov 3, 2021
005a9ff
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 3, 2021
da1b989
Update astroid/exceptions.py
DanielNoord Nov 3, 2021
d95d5ac
Fix overload
DanielNoord Nov 3, 2021
4a9af1e
Merge branch 'statement' of https://github.com/DanielNoord/astroid in…
DanielNoord Nov 3, 2021
ef019e6
Update astroid/nodes/node_ng.py
DanielNoord Nov 3, 2021
1f6ec98
WIP Code Review
DanielNoord Nov 4, 2021
df9b49c
Update astroid/nodes/scoped_nodes.py
DanielNoord Nov 4, 2021
6026560
Add NoReturn
DanielNoord Nov 4, 2021
d07374c
Code review
DanielNoord Nov 6, 2021
2596ee6
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 6, 2021
e8d0ff2
Apply suggestions from code review
DanielNoord Nov 6, 2021
73fb21d
Add tests
DanielNoord Nov 6, 2021
6777dc9
Apply suggestions from code review
DanielNoord Nov 6, 2021
1900532
Code Review
DanielNoord Nov 6, 2021
11aefd2
Added keyword parameter to all overloads and statements
DanielNoord Nov 6, 2021
2b6e7d0
Add disable
DanielNoord Nov 6, 2021
cab6a88
Update astroid/exceptions.py
DanielNoord Nov 6, 2021
78588ff
Update astroid/nodes/scoped_nodes.py
DanielNoord Nov 7, 2021
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
18 changes: 18 additions & 0 deletions astroid/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,24 @@ def __init__(self, target: "nodes.NodeNG") -> None:
super().__init__(message=f"Parent not found on {target!r}.")


class StatementMissing(ParentMissingError):
"""Raised when a call to node.statement() does not return a node. This is because
a node in the chain does not have a parent attribute and therefore does not
return a node for statement().

Standard attributes:
target: The node for which the parent lookup failed.
"""

def __init__(self, target: "nodes.NodeNG") -> None:
# pylint: disable-next=bad-super-call
# https://github.com/PyCQA/pylint/issues/2903
Copy link
Member

Choose a reason for hiding this comment

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

I think the spirit of the check is to signal the problem to people that don't really know how to use super(). Here it's a false positive because calling the father's function is what you really want to do, but maybe in most case and especially for beginner the check is right. If we remove this "false positive" the check will only have really obvious cases where the class called is not even an ancestor ?

Copy link
Member

Choose a reason for hiding this comment

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

🤔 Not sure about that. What it comes done to is if we want to emit errors for valid Python code. Even if it's probably not what was intended, there are still situations where it's useful and has it's place, especially with multi-inheritance.

The check also emits errors in cases that could cause recursion loops:

class SuperWithType(object):
    """type(self) may lead to recursion loop in derived classes"""
    def __init__(self):
        super(type(self), self).__init__()  # [bad-super-call]

class SuperWithSelfClass(object):
    """self.__class__ may lead to recursion loop in derived classes"""
    def __init__(self):
        super(self.__class__, self).__init__()  # [bad-super-call]

Any modifications should also make sure to still emit errors if the class is not part of the MRO.

class Getattr(object):
    """ crash """
    name = NewAaaa

class CrashSuper(object):
    """ test a crash with this checker """
    def __init__(self):
        super(Getattr.name, self).__init__()  # [bad-super-call]

Test cases taken from the pylint testsuit: super_checks.py

Copy link
Member

Choose a reason for hiding this comment

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

OK, I was trying to search for the initial issue where this was implemented for the rational but I can't find it. It makes sense to not warn for valid code, and the check has a lot of valid test cases, so let's fix this false positive.

# https://github.com/PyCQA/astroid/pull/1217#discussion_r744149027
super(ParentMissingError, self).__init__(
message=f"Statement not found on {target!r}"
)


# Backwards-compatibility aliases
OperationError = util.BadOperationMessage
UnaryOperationError = util.BadUnaryOperationMessage
Expand Down
51 changes: 46 additions & 5 deletions astroid/nodes/node_ng.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import pprint
import sys
import typing
import warnings
from functools import singledispatch as _singledispatch
from typing import (
TYPE_CHECKING,
Expand All @@ -10,6 +12,7 @@
Type,
TypeVar,
Union,
cast,
overload,
)

Expand All @@ -18,6 +21,7 @@
AstroidError,
InferenceError,
ParentMissingError,
StatementMissing,
UseInferenceDefault,
)
from astroid.manager import AstroidManager
Expand All @@ -27,6 +31,17 @@
if TYPE_CHECKING:
from astroid import nodes

if sys.version_info >= (3, 6, 2):
from typing import NoReturn
else:
from typing_extensions import NoReturn

if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal


# Types for 'NodeNG.nodes_of_class()'
T_Nodes = TypeVar("T_Nodes", bound="NodeNG")
T_Nodes2 = TypeVar("T_Nodes2", bound="NodeNG")
Expand Down Expand Up @@ -248,15 +263,41 @@ def parent_of(self, node):
return True
return False

def statement(self):
@overload
def statement(
self, *, future: Literal[None] = ...
) -> Union["nodes.Statement", "nodes.Module"]:
...

@overload
def statement(self, *, future: Literal[True]) -> "nodes.Statement":
...

def statement(
self, *, future: Literal[None, True] = None
) -> Union["nodes.Statement", "nodes.Module", NoReturn]:
"""The first parent node, including self, marked as statement node.

:returns: The first parent statement.
:rtype: NodeNG
TODO: Deprecate the future parameter and only raise StatementMissing and return
nodes.Statement

:raises AttributeError: If self has no parent attribute
:raises StatementMissing: If self has no parent attribute and future is True
"""
if self.is_statement:
return self
return self.parent.statement()
return cast("nodes.Statement", self)
if not self.parent:
if future:
raise StatementMissing(target=self)
warnings.warn(
"In astroid 3.0.0 NodeNG.statement() will return either a nodes.Statement "
"or raise a StatementMissing exception. AttributeError will no longer be raised. "
"This behaviour can already be triggered "
"by passing 'future=True' to a statement() call.",
DeprecationWarning,
)
raise AttributeError(f"{self} object has no attribute 'parent'")
return self.parent.statement(future=future)

def frame(
self,
Expand Down
45 changes: 41 additions & 4 deletions astroid/nodes/scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,10 @@
import io
import itertools
import os
import sys
import typing
from typing import List, Optional, TypeVar
import warnings
from typing import List, Optional, TypeVar, Union, overload

from astroid import bases
from astroid import decorators as decorators_mod
Expand All @@ -65,13 +67,26 @@
InconsistentMroError,
InferenceError,
MroError,
StatementMissing,
TooManyLevelsError,
)
from astroid.interpreter.dunder_lookup import lookup
from astroid.interpreter.objectmodel import ClassModel, FunctionModel, ModuleModel
from astroid.manager import AstroidManager
from astroid.nodes import Arguments, Const, node_classes

if sys.version_info >= (3, 6, 2):
from typing import NoReturn
else:
from typing_extensions import NoReturn


if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal


ITER_METHODS = ("__iter__", "__getitem__")
EXCEPTION_BASE_CLASSES = frozenset({"Exception", "BaseException"})
objects = util.lazy_import("objects")
Expand Down Expand Up @@ -637,12 +652,34 @@ def fully_defined(self):
"""
return self.file is not None and self.file.endswith(".py")

def statement(self):
@overload
def statement(self, *, future: Literal[None] = ...) -> "Module":
...

@overload
def statement(self, *, future: Literal[True]) -> NoReturn:
...

def statement(
self, *, future: Literal[None, True] = None
) -> Union[NoReturn, "Module"]:
"""The first parent node, including self, marked as statement node.

:returns: The first parent statement.
:rtype: NodeNG
When called on a :class:`Module` with the future parameter this raises an error.

TODO: Deprecate the future parameter and only raise StatementMissing

:raises StatementMissing: If no self has no parent attribute and future is True
"""
if future:
raise StatementMissing(target=self)
warnings.warn(
"In astroid 3.0.0 NodeNG.statement() will return either a nodes.Statement "
"or raise a StatementMissing exception. nodes.Module will no longer be "
"considered a statement. This behaviour can already be triggered "
"by passing 'future=True' to a statement() call.",
DeprecationWarning,
)
return self

def previous_sibling(self):
Expand Down
7 changes: 6 additions & 1 deletion tests/unittest_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
AstroidSyntaxError,
AttributeInferenceError,
InferenceError,
StatementMissing,
)
from astroid.nodes.scoped_nodes import Module

Expand Down Expand Up @@ -614,7 +615,11 @@ def test_module_base_props(self) -> None:
self.assertEqual(module.package, 0)
self.assertFalse(module.is_statement)
self.assertEqual(module.statement(), module)
self.assertEqual(module.statement(), module)
with pytest.warns(DeprecationWarning) as records:
module.statement()
assert len(records) == 1
with self.assertRaises(StatementMissing):
module.statement(future=True)

def test_module_locals(self) -> None:
"""test the 'locals' dictionary of an astroid module"""
Expand Down
7 changes: 7 additions & 0 deletions tests/unittest_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
AstroidBuildingError,
AstroidSyntaxError,
AttributeInferenceError,
StatementMissing,
)
from astroid.nodes.node_classes import (
AssignAttr,
Expand Down Expand Up @@ -626,6 +627,12 @@ def _test(self, value: Any) -> None:
self.assertIs(node.value, value)
self.assertTrue(node._proxied.parent)
self.assertEqual(node._proxied.root().name, value.__class__.__module__)
with self.assertRaises(AttributeError):
with pytest.warns(DeprecationWarning) as records:
node.statement()
assert len(records) == 1
with self.assertRaises(StatementMissing):
node.statement(future=True)

def test_none(self) -> None:
self._test(None)
Expand Down
2 changes: 2 additions & 0 deletions tests/unittest_scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ def test_default_value(self) -> None:
def test_navigation(self) -> None:
function = self.module["global_access"]
self.assertEqual(function.statement(), function)
self.assertEqual(function.statement(future=True), function)
l_sibling = function.previous_sibling()
# check taking parent if child is not a stmt
self.assertIsInstance(l_sibling, nodes.Assign)
Expand Down Expand Up @@ -821,6 +822,7 @@ def test_instance_special_attributes(self) -> None:
def test_navigation(self) -> None:
klass = self.module["YO"]
self.assertEqual(klass.statement(), klass)
self.assertEqual(klass.statement(future=True), klass)
l_sibling = klass.previous_sibling()
self.assertTrue(isinstance(l_sibling, nodes.FunctionDef), l_sibling)
self.assertEqual(l_sibling.name, "global_access")
Expand Down