Change `frame` and `scope` of `NamedExpr` for certain parents #1221

DanielNoord · 2021-10-24T12:32:28Z

Steps

Write a good description on what the PR does.

Description

This changes the frame and scope methods of NamedExpr whenever they occur in FunctionDef or ClassDef.

I have tested against current main of pylint and did not experience any regressions.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

DanielNoord · 2021-10-24T12:34:53Z

Keep forgetting that assignment expressions are >3.8... Oops. Will fix!

cdce8p

I tested the change against HomeAssistant and didn't notice any regression. Seems ok.

Could you add one more test case for NamedExpr in Comprehensions?

[y for i in (1, 2) if (y := i ** 2)]

astroid/nodes/node_classes.py

tests/unittest_nodes.py

DanielNoord · 2021-10-24T13:31:17Z

I'll the extra test tonight.

Btw, do we consider this breaking? Do we expect users to be dependent on the fact that this wrong previously?

cdce8p · 2021-10-24T13:37:58Z

I'll the extra test tonight.

👍🏻

Btw, do we consider this breaking? Do we expect users to be dependent on the fact that this wrong previously?

Do you mean at what point does a bug become a feature? ;)
We should mention the fix in the changelog, but besides that I would think it's ok.

DanielNoord · 2021-10-24T13:38:47Z

😃

I'll add a changelog entry then as well!

DanielNoord · 2021-10-24T15:03:00Z

We didn't actually catch the Comprehension example. You might want to retest with HomeAssistant.

cdce8p

I did test again. Seems to work still. Left some minor last comments. Let me know what you think.

ChangeLog

tests/unittest_nodes.py

Co-authored-by: Marc Mueller <[email protected]>

ChangeLog

cdce8p

LGTM! Thanks, @DanielNoord 🐬

ChangeLog

Co-authored-by: Marc Mueller <[email protected]>

DanielNoord · 2021-10-24T18:33:22Z

Thanks for the help @cdce8p!

Btw, if you ever get the time to do it I would appreciate it if you could take a look at the typing of frame. I tried to do it before but didn't know how such cases should be handled normally.

The problem is as follows.

Normally frame returns self.parent.frame()
Unless self == LocalsDictNodeNG, which returns self
Unless self == ComprehensionScope which returns self.parent.frame() again. ComprehensionScope inherits from LocalsDictNodeNG

Adding -> Union[Module, FunctionDef, ClassDef] to the first case seems most sensical.
Then for 2 I would think we would something similar as we did for scope() with def frame(self: T) -> T:.
However, then for 3 we would need -> Union[Module, FunctionDef, ClassDef] again, which raises a method not same as base class error.

So we have BaseClass, Inheritor and SubInheritor where the return type of BaseClass and SubInheritor should be similar but Inheritor and some of its other inheritors (Module, FunctionDef and ClassDef) have a different annotation. Is this something that can be easily/cleanly fixed with annotation?

I could also open an issue to track this. I think it would be helpful in pylint if we could add typing to frame.

ChangeLog

Co-authored-by: Marc Mueller <[email protected]>

Pierre-Sassoulas · 2021-10-24T20:36:24Z

astroid/nodes/node_classes.py

@@ -4225,6 +4225,46 @@ def postinit(self, target: NodeNG, value: NodeNG) -> None:
        self.target = target
        self.value = value

+    def frame(self):


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 ?

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

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

Pierre-Sassoulas · 2021-10-24T20:39:26Z

tests/unittest_nodes.py

+        of parent nodes.
+        """
+        module = builder.parse(
+            """


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

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.

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

Let's keep it that way.

cdce8p · 2021-10-24T21:05:48Z

Btw, if you ever get the time to do it I would appreciate it if you could take a look at the typing of frame. I tried to do it before but didn't know how such cases should be handled normally.

@DanielNoord This is indeed a bit complicated. At the moment I can only think of two options

A large refactoring. frame should move to a new mixin class. That will however create other typing issues.
Or adding type: ignore for it.

I guess 2 would be much easier to do.

DanielNoord · 2021-10-24T21:09:28Z

Is was thinking of removing frame from LocalsDictNodeNG and adding it to Module, FunctionDef and ClassDef separately.
Although this would create some duplicate code it might also make the code more maintainable and would remove the need for a type: ignore. Currently it is not immediately apparent which node actually return self because Comprehension is inherited by other classes itself.

Is that something you would consider?

cdce8p · 2021-10-24T21:20:40Z

Is that something you would consider?

By removing it from LocalsDictNodeNG, you should be aware that any function that currently returns LocalsDictNodeNG can't access frame() afterwards. That's mostly scope() if I'm not mistaken, so might or might not be an issue.

Just an idea:

Create a new class FrameNode which inherits from LocalsDictNodeNG
Move frame method to FrameNode class
Module, ClassDef, and FunctionDef should inherit from from FrameNode instead
scope() should return a union of FrameNode, ComprehensionScope, Lambda, ... (?)

Pierre-Sassoulas · 2021-10-24T21:23:54Z

Maybe we can create an issue with the current frame typing discussion so it's not lost after the merge ?

DanielNoord · 2021-10-24T21:24:11Z

Is that something you would consider?

By removing it from LocalsDictNodeNG, you should be aware that any function that currently returns LocalsDictNodeNG can't access frame() afterwards. That's mostly scope() if I'm not mistaken, so might or might not be an issue.

LocalsDictNG inherits from NodeNG:
https://github.com/PyCQA/astroid/blob/1620fb33e852e3d36244244cd99ee2d91aedfc5a/astroid/nodes/scoped_nodes.py#L206

Which has a frame method defined:
https://github.com/PyCQA/astroid/blob/1620fb33e852e3d36244244cd99ee2d91aedfc5a/astroid/nodes/node_ng.py#L261-L270

So, I would assume LocalsDictNG can still access frame right? Or am I missing something in the inheritance structure here (probably 😅 )

Just an idea:

Create a new class FrameNode which inherits from LocalsDictNodeNG

Move frame method to FrameNode class

Module, ClassDef, and FunctionDef should inherit from from FrameNode instead

scope() should return a union of FrameNode, ComprehensionScope, Lambda, ... (?)

I like this idea as well. I am happy to create a PR that implements this, but is that necessary with the knowledge that LocalsDictNG does have access to NodeNG's frame?

cdce8p · 2021-10-24T21:31:32Z

I like this idea as well. I am happy to create a PR that implements this, but is that necessary with the knowledge that LocalsDictNG does have access to NodeNG's frame?

Good point. Don't know about typing though. If you like, try to see if you can make it work. Open a WIP PR and I can take a look.

Change frame and scope of NamedExpr for certain parents

247a3d8

DanielNoord added the Bug 🪳 label Oct 24, 2021

DanielNoord added this to the 2.8.4 milestone Oct 24, 2021

Add skip to test for <3.8

acd25f2

cdce8p self-requested a review October 24, 2021 12:45

cdce8p added the pylint-tested PRs that don't cause major regressions with pylint label Oct 24, 2021

cdce8p requested changes Oct 24, 2021

View reviewed changes

astroid/nodes/node_classes.py Show resolved Hide resolved

tests/unittest_nodes.py Outdated Show resolved Hide resolved

tests/unittest_nodes.py Outdated Show resolved Hide resolved

tests/unittest_nodes.py Outdated Show resolved Hide resolved

Fix the scope() and frame() methods of NamedExpr nodes

d9b6028

cdce8p reviewed Oct 24, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

tests/unittest_nodes.py Show resolved Hide resolved

Update ChangeLog

e445e3c

Co-authored-by: Marc Mueller <[email protected]>

cdce8p reviewed Oct 24, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

Remove empty line

c19b2a3

cdce8p approved these changes Oct 24, 2021

View reviewed changes

cdce8p reviewed Oct 24, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

Update ChangeLog

d9a8dbd

Co-authored-by: Marc Mueller <[email protected]>

cdce8p reviewed Oct 24, 2021

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

Update ChangeLog

f9e2e19

Co-authored-by: Marc Mueller <[email protected]>

DanielNoord requested a review from Pierre-Sassoulas October 24, 2021 18:47

Pierre-Sassoulas reviewed Oct 24, 2021

View reviewed changes

Pierre-Sassoulas approved these changes Oct 24, 2021

View reviewed changes

DanielNoord merged commit b90714e into pylint-dev:main Oct 24, 2021

DanielNoord deleted the scope branch October 24, 2021 21:42

DanielNoord mentioned this pull request Nov 20, 2021

Fix frame() error on inferred node #1263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `frame` and `scope` of `NamedExpr` for certain parents #1221

Change `frame` and `scope` of `NamedExpr` for certain parents #1221

DanielNoord commented Oct 24, 2021 •

edited

Loading

DanielNoord commented Oct 24, 2021

cdce8p left a comment

DanielNoord commented Oct 24, 2021

cdce8p commented Oct 24, 2021

DanielNoord commented Oct 24, 2021

DanielNoord commented Oct 24, 2021

cdce8p left a comment

cdce8p left a comment

DanielNoord commented Oct 24, 2021 •

edited

Loading

Pierre-Sassoulas Oct 24, 2021

DanielNoord Oct 24, 2021

cdce8p Oct 24, 2021

Pierre-Sassoulas Oct 24, 2021

DanielNoord Oct 24, 2021

cdce8p Oct 24, 2021

Pierre-Sassoulas Oct 24, 2021

cdce8p commented Oct 24, 2021

DanielNoord commented Oct 24, 2021 •

edited

Loading

cdce8p commented Oct 24, 2021

Pierre-Sassoulas commented Oct 24, 2021

DanielNoord commented Oct 24, 2021 •

edited

Loading

cdce8p commented Oct 24, 2021

Change frame and scope of NamedExpr for certain parents #1221

Change frame and scope of NamedExpr for certain parents #1221

Conversation

DanielNoord commented Oct 24, 2021 • edited Loading

Steps

Description

Type of Changes

Related Issue

DanielNoord commented Oct 24, 2021

cdce8p left a comment

Choose a reason for hiding this comment

DanielNoord commented Oct 24, 2021

cdce8p commented Oct 24, 2021

DanielNoord commented Oct 24, 2021

DanielNoord commented Oct 24, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

DanielNoord commented Oct 24, 2021 • edited Loading

Pierre-Sassoulas Oct 24, 2021

Choose a reason for hiding this comment

DanielNoord Oct 24, 2021

Choose a reason for hiding this comment

cdce8p Oct 24, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Oct 24, 2021

Choose a reason for hiding this comment

DanielNoord Oct 24, 2021

Choose a reason for hiding this comment

cdce8p Oct 24, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas Oct 24, 2021

Choose a reason for hiding this comment

cdce8p commented Oct 24, 2021

DanielNoord commented Oct 24, 2021 • edited Loading

cdce8p commented Oct 24, 2021

Pierre-Sassoulas commented Oct 24, 2021

DanielNoord commented Oct 24, 2021 • edited Loading

cdce8p commented Oct 24, 2021

Change `frame` and `scope` of `NamedExpr` for certain parents #1221

Change `frame` and `scope` of `NamedExpr` for certain parents #1221

DanielNoord commented Oct 24, 2021 •

edited

Loading

DanielNoord commented Oct 24, 2021 •

edited

Loading

DanielNoord commented Oct 24, 2021 •

edited

Loading

DanielNoord commented Oct 24, 2021 •

edited

Loading