Fix frame() error on inferred node #1263

tushar-deepsource · 2021-11-20T11:13:48Z

Description

Ran into this issue today. The parent of an inferred node was None, so node.frame() was crashing. This should fix that.

Type of Changes

	Type
✓	🐛 Bug fix

DanielNoord · 2021-11-20T11:27:37Z

On which commit of astroid did you experience this issue? We have recently worked on NodeNG.frame() in #1225 and #1221, but it seems we have not fixed all issues.

In any case, whenever NodeNG.parent is missing we should raise the new ParentMissingError, or perhaps a new FrameMissing inheriting from it as we did for NodeNG.statement() in #1217.

This was quite difficult to do correctly and took me some tries to get right, but you're more than welcome to start working towards it! I'm happy to provide support a long the way and answer any questions you might have!

tushar-deepsource · 2021-11-20T14:51:33Z

I encountered this on 2.7.3, with pylint 2.10.2 I believe.

tushar-deepsource · 2021-11-20T18:03:46Z

Actually, in my case the latest master fixes this issue. Thanks though!

tushar-deepsource · 2021-11-21T17:50:51Z

It came back. I needed this fix to make inference work locally again today.

tushar-deepsource · 2021-11-21T17:58:58Z

Does this seem good?
Should I do the same for .statement() as well?

DanielNoord

Thanks for reopening the issue!

Would you mind adding a test for this as well? You can probably base it on the code that is currently throwing an error for you or look at how we tested StatementMissing in #1217!

DanielNoord · 2021-11-22T16:33:02Z

astroid/nodes/node_ng.py

@@ -311,6 +310,13 @@ def frame(

        :returns: The first parent frame node.
        """
+        if self.parent is None:
+            if isinstance(


I don't think this isinstance check should be here. If self is either of these nodes we shouldn't be in this function anyway as they have their frame() defined in their class definitions.

I think just raising the exception is fine.

I looked at NamedExpr.frame() which seems to raise ParentMissingError. I think if we go ahead with raising FrameMissing we might use this exception there as well. It shouldn't be a breaking change since FrameMissing inherits from ParentMissingError.

The isinstance check is indeed unnecessary. Just this would be enough:

if not self.parent: raise ParentMissingError(target=self) return self.parent.frame()

--
Technically this is a small breaking change as users could be fine with catching AttributeError.
We added future=True to statement() so just for completeness we should probably do that here, too. You can take a look at #1217 as example.

cdce8p

Should I do the same for .statement() as well?

@tushar-deepsource What do you mean with that? @DanielNoord mentioned it already but we reworked statement() in #1217. With future=True this should already work.

This change will also need a changelog entry and tests.

Would you feel confident doing these changes? It should be relatively straightforward if you follow the lead of #1217. Please let us know if you need help or if we should take this one over. For a first PR this is quite complex.

cdce8p · 2021-12-07T22:28:21Z

astroid/exceptions.py

+class FrameMissing(ParentMissingError):
+    """Raised when a call to node.frame() 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 frame().
+
+    Standard attributes:
+        target: The node for which the parent lookup failed.
+    """
+
+    def __init__(self, target: "nodes.NodeNG") -> None:
+        # pylint: disable-next=bad-super-call
+        super(ParentMissingError, self).__init__(
+            message=f"Frame not found on {target!r}"
+        )


I'm wondering if it's actually necessary to create a new exception subclass. All existing frame() and scope() methods raise just a ParentMissingError.

@DanielNoord I don't actually remember the exact details why we decided to add StatementMissingError maybe you do? Was there an actual need to have a more specific error or was it just an idea that we adopted?

I think this followed from the logic that we were catching the AttributeError ourselves so it sort of naturally followed that we had to create a new error to catch.
It might be a bit overkill, but I think it makes sense to make a new subclass: the more fine grained the error is the better users will know how to handle it.

cdce8p · 2021-12-07T22:32:02Z

astroid/nodes/node_ng.py

@@ -311,6 +310,13 @@ def frame(

        :returns: The first parent frame node.
        """
+        if self.parent is None:
+            if isinstance(


The isinstance check is indeed unnecessary. Just this would be enough:

if not self.parent: raise ParentMissingError(target=self) return self.parent.frame()

--
Technically this is a small breaking change as users could be fine with catching AttributeError.
We added future=True to statement() so just for completeness we should probably do that here, too. You can take a look at #1217 as example.

tushar-deepsource · 2021-12-14T10:40:09Z

To double check: I'd have to essentially replicate the type stubs and tests for statement, for frame in this PR?

If the changes so far make sense, I can go ahead and do that.

DanielNoord · 2021-12-14T10:49:35Z

To double check: I'd have to essentially replicate the type stubs and tests for statement, for frame in this PR?

If the changes so far make sense, I can go ahead and do that.

I'll wait for @cdce8p to comment on whether he thinks that subclassing ParentMissingError makes sense here. Either way we should raise something for if not node.parent.

Other than that I think the following todo list should bring this PR a long way towards getting merged:

Add keyword only argument future to frame()
Add check for future argument and raise Frame/ParentMissingError for if not node.parent
Add DeprecationWarning and TODO for calls to frame() which raise AttributeError (also raise AttributeError)
Add overloads to frame()
Add tests for all variants of frame() introduced by this PR. See Add typing to NodeNG.statement #1217 for examples.

Follow-up PR:
6) Add future keyword to all internal calls to frame() (see #1235)

For all of these changes #1217 and #1235 and the discussions within those PR could help.

cdce8p · 2021-12-14T13:18:17Z

Thanks for the summery @DanielNoord! That looks about right to me.

I'll wait for @cdce8p to comment on whether he thinks that subclassing ParentMissingError makes sense here.

Usually just subclassing an exception doesn't really hurt much. However in this case it also doesn't add any more information. I personally don't really see a need to be even more specific than ParentMissingError already is. Adding FrameMissing just introduces unnecessary complexity both for users and us. So I would recommend to stick with the ParentMissingError.

DanielNoord · 2021-12-14T13:19:44Z

Usually just subclassing an exception doesn't really hurt much. However in this case it also doesn't add any more information. I personally don't really see a need to be even more specific than ParentMissingError already is. Adding FrameMissing just introduces unnecessary complexity both for users and us. So I would recommend to stick with the ParentMissingError.

Fine with me!

@tushar-deepsource Let me know if you need any help with implementing this. Happy to help as always 😄

for more information, see https://pre-commit.ci

…atch-1

for more information, see https://pre-commit.ci

astroid/nodes/node_ng.py

tushar-deepsource · 2021-12-24T22:13:08Z

Should be good now

for more information, see https://pre-commit.ci

astroid/nodes/node_ng.py

cdce8p

Looks good overall!

@Pierre-Sassoulas @DanielNoord What do you guys think, is a changelog entry necessary? We didn't add one for #1217. In theory everyone that depends on the AttributeError should get a DeprecationWarning. We can (or maybe should ?) include a note when we eventually release 3.0.0.

cdce8p · 2021-12-27T23:08:24Z

astroid/nodes/node_ng.py

@@ -314,7 +313,7 @@ def statement(
        return self.parent.statement(future=future)

    def frame(
-        self,
+        self, *, future: Literal[None, True] = None


Turns out the overload idea doesn't actually work as I though. Sorry for the confusion. The default with None is needed for the parent calls.

... return self.parent.frame(future=future)

I've already opened another PR to fix the node.statement calls #1317.

Pierre-Sassoulas

is a changelog entry necessary?

I think so, could you add a notice about the deprecation in the changelog @tushar-deepsource , please ? :)

tusharsadhwani · 2021-12-28T06:46:05Z

sure

Pierre-Sassoulas

Thank you ! This is blocked by the release of 2.9.1, we'll merge as soon as 2.9.1 is out :)

DanielNoord · 2021-12-28T13:15:18Z

@Pierre-Sassoulas This should go in 2.9.1 as it is technically a bug fix 😉

Pierre-Sassoulas

Thanks for the comment @DanielNoord :) Could you move the changelog to 2.9.1 @tushar-deepsource ? I can't suggest/apply it in interface.

Pierre-Sassoulas

👌

cdce8p · 2021-12-29T00:19:12Z

I think we could merge this one as is. The test errors are unrelated to the changes made here.

Pierre-Sassoulas added this to the 2.9.0 milestone Nov 20, 2021

Pierre-Sassoulas added Bug 🪳 crash labels Nov 20, 2021

tushar-deepsource closed this Nov 20, 2021

tushar-deepsource deleted the tushar-deepsource-patch-1 branch November 20, 2021 18:03

DanielNoord removed this from the 2.9.0 milestone Nov 21, 2021

tushar-deepsource restored the tushar-deepsource-patch-1 branch November 21, 2021 17:50

tushar-deepsource reopened this Nov 21, 2021

Pierre-Sassoulas added this to the 2.9.1 milestone Nov 21, 2021

cdce8p self-requested a review November 21, 2021 18:52

DanielNoord reviewed Nov 22, 2021

View reviewed changes

DanielNoord added the Waiting on author label Dec 1, 2021

cdce8p requested changes Dec 7, 2021

View reviewed changes

cdce8p modified the milestones: 2.9.1, 2.10.0 Dec 7, 2021

cdce8p removed the crash label Dec 7, 2021

tushar-deepsource requested review from cdce8p and DanielNoord December 14, 2021 10:37

tushar-deepsource and others added 3 commits December 20, 2021 14:06

Fix frame() error on inferred node

fff4db4

Update node_ng.py

2cab065

[pre-commit.ci] auto fixes from pre-commit.com hooks

8952c64

for more information, see https://pre-commit.ci

tushar-deepsource and others added 3 commits December 25, 2021 03:35

Merge remote-tracking branch 'upstream/main' into tushar-deepsource-p…

b9f0ee0

…atch-1

[pre-commit.ci] auto fixes from pre-commit.com hooks

42b787e

for more information, see https://pre-commit.ci

Fix imports

9be8a89

tushar-deepsource commented Dec 24, 2021

View reviewed changes

astroid/nodes/node_ng.py Outdated Show resolved Hide resolved

astroid/nodes/node_ng.py Outdated Show resolved Hide resolved

tushar-deepsource added 2 commits December 25, 2021 03:42

Apply suggestions from code review

08bcd08

Update astroid/nodes/node_ng.py

79bb6c2

pre-commit-ci bot and others added 2 commits December 24, 2021 22:13

[pre-commit.ci] auto fixes from pre-commit.com hooks

76faa99

for more information, see https://pre-commit.ci

Docstring

1f1f7e7

tushar-deepsource commented Dec 24, 2021

View reviewed changes

astroid/nodes/node_ng.py Outdated Show resolved Hide resolved

Update astroid/nodes/node_ng.py

c68db00

cdce8p approved these changes Dec 27, 2021

View reviewed changes

Pierre-Sassoulas requested changes Dec 28, 2021

View reviewed changes

Add changelog entry

8bbba41

tushar-deepsource requested a review from Pierre-Sassoulas December 28, 2021 08:09

Pierre-Sassoulas approved these changes Dec 28, 2021

View reviewed changes

Pierre-Sassoulas added the Blocked 🚧 A PR or issue blocked by another PR or issue label Dec 28, 2021

Pierre-Sassoulas removed the Blocked 🚧 A PR or issue blocked by another PR or issue label Dec 28, 2021

Pierre-Sassoulas modified the milestones: 2.10.0, 2.9.1 Dec 28, 2021

Pierre-Sassoulas requested changes Dec 28, 2021

View reviewed changes

Move changelog

7ad182c

tushar-deepsource requested a review from Pierre-Sassoulas December 28, 2021 13:26

whitespace

1fe3c11

Pierre-Sassoulas approved these changes Dec 28, 2021

View reviewed changes

Pierre-Sassoulas merged commit 12e2e8c into pylint-dev:main Dec 29, 2021

tushar-deepsource deleted the tushar-deepsource-patch-1 branch January 4, 2022 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix frame() error on inferred node #1263

Fix frame() error on inferred node #1263

tushar-deepsource commented Nov 20, 2021 •

edited

Loading

DanielNoord commented Nov 20, 2021 •

edited

Loading

tushar-deepsource commented Nov 20, 2021

tushar-deepsource commented Nov 20, 2021

tushar-deepsource commented Nov 21, 2021

tushar-deepsource commented Nov 21, 2021

DanielNoord left a comment

DanielNoord Nov 22, 2021 •

edited

Loading

cdce8p Dec 7, 2021

cdce8p left a comment

cdce8p Dec 7, 2021

DanielNoord Dec 8, 2021

cdce8p Dec 7, 2021

tushar-deepsource commented Dec 14, 2021

DanielNoord commented Dec 14, 2021

cdce8p commented Dec 14, 2021

DanielNoord commented Dec 14, 2021

tushar-deepsource commented Dec 24, 2021

cdce8p left a comment

cdce8p Dec 27, 2021

Pierre-Sassoulas left a comment

tusharsadhwani commented Dec 28, 2021

Pierre-Sassoulas left a comment

DanielNoord commented Dec 28, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

Pierre-Sassoulas left a comment

cdce8p commented Dec 29, 2021

Fix frame() error on inferred node #1263

Fix frame() error on inferred node #1263

Conversation

tushar-deepsource commented Nov 20, 2021 • edited Loading

Description

Type of Changes

DanielNoord commented Nov 20, 2021 • edited Loading

tushar-deepsource commented Nov 20, 2021

tushar-deepsource commented Nov 20, 2021

tushar-deepsource commented Nov 21, 2021

tushar-deepsource commented Nov 21, 2021

DanielNoord left a comment

Choose a reason for hiding this comment

DanielNoord Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

cdce8p Dec 7, 2021

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p Dec 7, 2021

Choose a reason for hiding this comment

DanielNoord Dec 8, 2021

Choose a reason for hiding this comment

cdce8p Dec 7, 2021

Choose a reason for hiding this comment

tushar-deepsource commented Dec 14, 2021

DanielNoord commented Dec 14, 2021

cdce8p commented Dec 14, 2021

DanielNoord commented Dec 14, 2021

tushar-deepsource commented Dec 24, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p Dec 27, 2021

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

tusharsadhwani commented Dec 28, 2021

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Dec 28, 2021 • edited Loading

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

cdce8p commented Dec 29, 2021

tushar-deepsource commented Nov 20, 2021 •

edited

Loading

DanielNoord commented Nov 20, 2021 •

edited

Loading

DanielNoord Nov 22, 2021 •

edited

Loading

DanielNoord commented Dec 28, 2021 •

edited

Loading