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

Change frame and scope of NamedExpr for certain parents #1221

Merged
merged 7 commits into from
Oct 24, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ What's New in astroid 2.8.4?
Release date: TBA


cdce8p marked this conversation as resolved.
Show resolved Hide resolved
* Fix the `scope()` and `frame()` methods of `NamedExpr` nodes.
When these nodes occur in `Arguments`, `Keyword` or `Comprehension` nodes these
methods now correctly point to the outer-scope of the `FunctionDef`,
`ClassDef` or comprehension.
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved


What's New in astroid 2.8.3?
============================
Expand Down
40 changes: 40 additions & 0 deletions astroid/nodes/node_classes.py
Original file line number Diff line number Diff line change
Expand Up @@ -4225,6 +4225,46 @@ def postinit(self, target: NodeNG, value: NodeNG) -> None:
self.target = target
self.value = value

def frame(self):
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it was discussed with Marc (sorry I did not read all the comment), and adding typing here should be done later and the docstring is enough for now ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the docstring is enough for now. I will need some guidance from @cdce8p to actually fully type frame().

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, typing frame is a bit complicated due to the current class / mixin structure. We should tackle that separately.

"""The first parent frame node.

A frame node is a :class:`Module`, :class:`FunctionDef`,
or :class:`ClassDef`.

:returns: The first parent frame node.
"""
if not self.parent:
raise ParentMissingError(target=self)

# For certain parents NamedExpr evaluate to the scope of the parent
if isinstance(self.parent, (Arguments, Keyword, Comprehension)):
if not self.parent.parent:
raise ParentMissingError(target=self.parent)
if not self.parent.parent.parent:
raise ParentMissingError(target=self.parent.parent)
return self.parent.parent.parent.frame()
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved

return self.parent.frame()

def scope(self) -> "LocalsDictNodeNG":
"""The first parent node defining a new scope.
These can be Module, FunctionDef, ClassDef, Lambda, or GeneratorExp nodes.

:returns: The first parent scope node.
"""
if not self.parent:
raise ParentMissingError(target=self)

# For certain parents NamedExpr evaluate to the scope of the parent
if isinstance(self.parent, (Arguments, Keyword, Comprehension)):
if not self.parent.parent:
raise ParentMissingError(target=self.parent)
if not self.parent.parent.parent:
raise ParentMissingError(target=self.parent.parent)
return self.parent.parent.parent.scope()

return self.parent.scope()


class Unknown(mixins.AssignTypeMixin, NodeNG):
"""This node represents a node in a constructed AST where
Expand Down
111 changes: 111 additions & 0 deletions tests/unittest_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,117 @@ def hello(False):
builder.parse(code)


@pytest.mark.skipif(not PY38_PLUS, reason="needs assignment expressions")
class TestNamedExprNode:
"""Tests for the NamedExpr node"""

@staticmethod
def test_frame() -> None:
"""Test if the frame of NamedExpr is correctly set for certain types
of parent nodes.
"""
module = builder.parse(
"""
def func(var_1):
pass

def func_two(var_2, var_2 = (named_expr_1 := "walrus")):
pass

class MyBaseClass:
pass

class MyInheritedClass(MyBaseClass, var_3=(named_expr_2 := "walrus")):
pass

VAR = lambda y = (named_expr_3 := "walrus"): print(y)

def func_with_lambda(
var_5 = (
named_expr_4 := lambda y = (named_expr_5 := "walrus"): y
)
):
pass

COMPREHENSION = [y for i in (1, 2) if (y := i ** 2)]
"""
)
function = module.body[0]
assert function.args.frame() == function

function_two = module.body[1]
assert function_two.args.args[0].frame() == function_two
assert function_two.args.args[1].frame() == function_two
assert function_two.args.defaults[0].frame() == module
DanielNoord marked this conversation as resolved.
Show resolved Hide resolved

inherited_class = module.body[3]
assert inherited_class.keywords[0].frame() == inherited_class
assert inherited_class.keywords[0].value.frame() == module

lambda_assignment = module.body[4].value
assert lambda_assignment.args.args[0].frame() == lambda_assignment
assert lambda_assignment.args.defaults[0].frame() == module

lambda_named_expr = module.body[5].args.defaults[0]
assert lambda_named_expr.value.args.defaults[0].frame() == module

comprehension = module.body[6].value
assert comprehension.generators[0].ifs[0].frame() == module

@staticmethod
def test_scope() -> None:
"""Test if the scope of NamedExpr is correctly set for certain types
of parent nodes.
"""
module = builder.parse(
"""
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can create a fixture for the example? It seems the same example is used for frame and scope.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could, although that might also make these tests less flexible in the future. I can explore this, but personally I'm fine with keeping it like this.

Copy link
Member

Choose a reason for hiding this comment

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

The example is the same. I'm just not sure it does make much sense to create a fixture for it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it that way.

def func(var_1):
pass

def func_two(var_2, var_2 = (named_expr_1 := "walrus")):
pass

class MyBaseClass:
pass

class MyInheritedClass(MyBaseClass, var_3=(named_expr_2 := "walrus")):
pass

VAR = lambda y = (named_expr_3 := "walrus"): print(y)

def func_with_lambda(
var_5 = (
named_expr_4 := lambda y = (named_expr_5 := "walrus"): y
)
):
pass

COMPREHENSION = [y for i in (1, 2) if (y := i ** 2)]
"""
)
function = module.body[0]
assert function.args.scope() == function

function_two = module.body[1]
assert function_two.args.args[0].scope() == function_two
assert function_two.args.args[1].scope() == function_two
assert function_two.args.defaults[0].scope() == module

inherited_class = module.body[3]
assert inherited_class.keywords[0].scope() == inherited_class
assert inherited_class.keywords[0].value.scope() == module

lambda_assignment = module.body[4].value
assert lambda_assignment.args.args[0].scope() == lambda_assignment
assert lambda_assignment.args.defaults[0].scope()

lambda_named_expr = module.body[5].args.defaults[0]
assert lambda_named_expr.value.args.defaults[0].scope() == module

comprehension = module.body[6].value
assert comprehension.generators[0].ifs[0].scope() == module


class AnnAssignNodeTest(unittest.TestCase):
def test_primitive(self) -> None:
code = textwrap.dedent(
Expand Down