Add typing to `scope()` #1170

DanielNoord · 2021-09-13T12:28:47Z

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
Write a good description on what the PR does.

Description

After stumbling across this in pylint-dev/pylint#4995 I thought I might as well make a start with this.

One of the problems I foresee is that in some cases we return None in node_ng.py. I am not sure if this ever happens in a real scenario, but this will trip up mypy for pylint as it will want us to assert that any call to NodeNG.scope() did not return None.
Based on the docstring of that function that might be (legacy) code which is never triggered, but I am not sure. Perhaps a maintainer knows more about this.
In any case, if we can remove the Optional from the return typing this would be much more useful for pylint.

Type of Changes

	Type
✓	🔨 Refactoring

Related Issue

cdce8p

Added a few comments. Unfortunately, they will probably result in more work than you expected, sorry about that.

astroid/nodes/scoped_nodes.py

astroid/nodes/node_classes.py

cdce8p · 2021-09-13T14:18:49Z

astroid/nodes/node_ng.py

@@ -246,7 +259,7 @@ def frame(self):
        """
        return self.parent.frame()

-    def scope(self):
+    def scope(self) -> Optional["LocalsDictNodeNG"]:


Regarding LocalsDictNodeNG: I fear that at some point it will just create another circular dependency. Maybe we should start looking into Protocol classes for that 🤔 Haven't really done so myself, thus don't know for certain if it would work.

https://docs.python.org/3/library/typing.html#typing.Protocol

--
Another issue: If at some point some scoped node inherits from another mixin class (not LocalsDictNodeNG, this will break.

Are you sure? Doesn't using "Class" prevent circular dependency?

The current code is fine. As you said, using a string as type annotation does prevent the circular dependency.
It's probably just that I'm not really a fan of the class name itself (LocalsDictNodeNG). Maybe that should be changed at some point 🤔

Anyway, let's leave it for now.

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

astroid/exceptions.py

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

astroid/exceptions.py

…cope

DanielNoord · 2021-09-13T23:07:13Z

Is this failing because of the recent update to pre-commit? 😕

cdce8p · 2021-09-13T23:16:34Z

Is this failing because of the recent update to pre-commit? 😕

No. main is fine. It seems to be something in this PR.

Update: I can reproduce the issue when running pylint locally.

DanielNoord · 2021-09-13T23:19:29Z

It has something to do with the import of nodes. Removing it fixes the issue.
Will investigate tomorrow!

cdce8p · 2021-09-13T23:22:18Z

It has something to do with the import of nodes. Removing it fixes the issue.
Will investigate tomorrow!

I would suspect some kind of caching issue. Might not be that easy to track down.

DanielNoord · 2021-09-14T06:43:10Z

I'm not sure if this solution works for you, but adding disable to the new error messages and removing on the unused places seems to solve the issue.
no-member has numerous false positives reported so I presumed this is likely a pylint issue rather than an issue with the actual code. Please correct if I'm wrong or if you would like another solution!

astroid/nodes/node_classes.py

Pierre-Sassoulas · 2021-09-15T07:51:50Z

astroid/nodes/scoped_nodes.py

@@ -77,6 +77,8 @@
    {"classmethod", "staticmethod", "builtins.classmethod", "builtins.staticmethod"}
 )

+T = TypeVar("T")


I don't understand this part, what does T means ?

I don't know how it works under the hood. But for

def scope(self: T) -> T: """The first parent node defining a new scope. :returns: The first parent scope node. :rtype: Module or FunctionDef or ClassDef or Lambda or GenExpr """ return self

It makes the function return with the correct typing. It sets the return type to the type of self.

Ok I've read the documentation and I think T = TypeVar("T") is the example for TypeVar from the python doc, but I guess we can change the name to something else ? Maybe Scope (Scope = TypeVar("Scope")) ?

#1170 (comment)

This is something for you to discuss with Marc 😄

@cdce8p what do you think, isn't it better to have an explicit name here ?

Sorry for the discrepancies @DanielNoord I started to not review some MR that Marc is already reviewing because there is a lot to review :)

No worries! With the typing PRs it never hurts to have an additional pair of eyes.

It would be possible to rename the TypeVar, I just wouldn't recommend it in this case. T is the usual convention. Additionally, it doesn't have any constraints or bounds. Thus it could be reused in another function later.

That is, for example, different in the TypeVar we used in pylint-dev/pylint#4978.

astroid/nodes/node_classes.py

astroid/nodes/node_ng.py

cdce8p · 2021-09-15T10:48:10Z

astroid/nodes/scoped_nodes.py

@@ -1606,6 +1608,7 @@ def blockstart_tolineno(self):

        :type: int
        """
+        # pylint: disable-next=no-member


I'm a bit hesitant about these pylint: disable statements. Maybe it would be better to remove the annotations in astroid/exceptions.py for now.
They don't add much as the error is only used in two locations anyway.

I'm going to fix this in another PR. I'll tag you!

I see now! These are actual bugs, only Arguments should be assigned to self.args not a list.
I was under the impression that these are pylint bugs.

In that case, these are ok.

astroid/exceptions.py

DanielNoord · 2021-09-15T11:42:27Z

Note to self after merging of Fix documentation after changes to astroid.nodes #1175, the linkt to LocalsDictNodeNG needs to be changed in doc/api/astroid.nodes.rst

cdce8p · 2021-09-15T11:44:36Z

Note to self after merging of Fix documentation after changes to astroid.nodes #1175, the linkt to LocalsDictNodeNG needs to be changed in doc/api/astroid.nodes.rst

You don't need to wait for it. That merge conflict should be easy to resolve.

cdce8p

Some final comments. Besides that, this LGTM!
Thanks @DanielNoord 🐬

Can be merged if @Pierre-Sassoulas is ok with using T for the generic TypeVar.

astroid/nodes/node_classes.py

astroid/nodes/node_ng.py

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p · 2021-09-15T13:58:06Z

@DanielNoord I've just merged the #1174 and #1175. Can you update this one so it can be merged as well? Let me know if I can help.

…cope

…into typing-scope

DanielNoord · 2021-09-15T14:07:06Z

@cdce8p Should be good to go after these tests pass!

cdce8p

Thanks @DanielNoord 🚀

DanielNoord · 2021-09-15T14:24:58Z

@cdce8p The check on this latest commit failed although main seems to be fine. Might be because I removed the branch mid-check, but thought I would notify you about this.

cdce8p · 2021-09-15T14:59:36Z

@DanielNoord Thanks. Yeah, this isn't an issue. main is fine.

Add typing to scope()

ff7b948

DanielNoord marked this pull request as draft September 13, 2021 12:28

cdce8p requested changes Sep 13, 2021

View reviewed changes

cdce8p added Maintenance Discussion or action around maintaining astroid or the dev workflow Work in progress High effort 🏋 Difficult solution or problem to solve and removed High effort 🏋 Difficult solution or problem to solve labels Sep 13, 2021

cdce8p mentioned this pull request Sep 13, 2021

Remove most typing.cast() calls pylint-dev/pylint#4995

Merged

2 tasks

DanielNoord and others added 2 commits September 13, 2021 20:58

Implement code review

1c553e8

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

e1f3fd9

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

DanielNoord marked this pull request as ready for review September 13, 2021 19:01

cdce8p reviewed Sep 13, 2021

View reviewed changes

astroid/exceptions.py Outdated Show resolved Hide resolved

DanielNoord and others added 2 commits September 14, 2021 00:43

Implement code reviw

6e265d5

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

2d3f8ed

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

DanielNoord commented Sep 13, 2021

View reviewed changes

astroid/exceptions.py Show resolved Hide resolved

cdce8p reviewed Sep 13, 2021

View reviewed changes

astroid/exceptions.py Outdated Show resolved Hide resolved

DanielNoord added 2 commits September 14, 2021 00:56

Update after code review

be92085

Merge branch 'main' of https://github.com/PyCQA/astroid into typing-s…

2260c12

…cope

Changes to pylint disable comments

84ddf63

Pierre-Sassoulas added this to the 2.8.1 milestone Sep 14, 2021

DanielNoord requested a review from Pierre-Sassoulas September 15, 2021 07:23

Pierre-Sassoulas reviewed Sep 15, 2021

View reviewed changes

Add LocalsDictNodeNG to astroid.nodes

e3c1471

DanielNoord commented Sep 15, 2021

View reviewed changes

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

Update astroid/nodes/node_classes.py

c5bd321

cdce8p reviewed Sep 15, 2021

View reviewed changes

astroid/exceptions.py Show resolved Hide resolved

Code review changes

b9e12ab

DanielNoord mentioned this pull request Sep 15, 2021

Add Arguments typing to Lambda & FunctionDef #1174

Merged

2 tasks

Update documentation for LocalsDictNodeNG

946352b

cdce8p approved these changes Sep 15, 2021

View reviewed changes

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

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

cdce8p removed the Work in progress label Sep 15, 2021

Apply suggestions from code review

abb4033

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

Pierre-Sassoulas approved these changes Sep 15, 2021

View reviewed changes

DanielNoord added 3 commits September 15, 2021 16:03

Merge branch 'main' of https://github.com/PyCQA/astroid into typing-s…

38ad22c

…cope

Remove useless suppression

cdc150b

Merge branch 'typing-scope' of https://github.com/DanielNoord/astroid …

af720ae

…into typing-scope

cdce8p approved these changes Sep 15, 2021

View reviewed changes

cdce8p merged commit 2e26f03 into pylint-dev:main Sep 15, 2021

DanielNoord deleted the typing-scope branch September 15, 2021 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing to `scope()` #1170

Add typing to `scope()` #1170

DanielNoord commented Sep 13, 2021

cdce8p left a comment

cdce8p Sep 13, 2021

DanielNoord Sep 13, 2021

cdce8p Sep 13, 2021

DanielNoord commented Sep 13, 2021

cdce8p commented Sep 13, 2021 •

edited

DanielNoord commented Sep 13, 2021

cdce8p commented Sep 13, 2021

DanielNoord commented Sep 14, 2021

Pierre-Sassoulas Sep 15, 2021

DanielNoord Sep 15, 2021 •

edited by cdce8p

Pierre-Sassoulas Sep 15, 2021 •

edited

DanielNoord Sep 15, 2021

Pierre-Sassoulas Sep 15, 2021

Pierre-Sassoulas Sep 15, 2021

DanielNoord Sep 15, 2021 •

edited

cdce8p Sep 15, 2021

cdce8p Sep 15, 2021

DanielNoord Sep 15, 2021

DanielNoord Sep 15, 2021

cdce8p Sep 15, 2021

DanielNoord commented Sep 15, 2021 •

edited

cdce8p commented Sep 15, 2021

cdce8p left a comment

cdce8p commented Sep 15, 2021

DanielNoord commented Sep 15, 2021

cdce8p left a comment

DanielNoord commented Sep 15, 2021

cdce8p commented Sep 15, 2021

Add typing to scope() #1170

Add typing to scope() #1170

Conversation

DanielNoord commented Sep 13, 2021

Steps

Description

Type of Changes

Related Issue

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Sep 13, 2021

cdce8p commented Sep 13, 2021 • edited

DanielNoord commented Sep 13, 2021

cdce8p commented Sep 13, 2021

DanielNoord commented Sep 14, 2021

Choose a reason for hiding this comment

DanielNoord Sep 15, 2021 • edited by cdce8p

Choose a reason for hiding this comment

Pierre-Sassoulas Sep 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord Sep 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Sep 15, 2021 • edited

cdce8p commented Sep 15, 2021

cdce8p left a comment

Choose a reason for hiding this comment

cdce8p commented Sep 15, 2021

DanielNoord commented Sep 15, 2021

cdce8p left a comment

Choose a reason for hiding this comment

DanielNoord commented Sep 15, 2021

cdce8p commented Sep 15, 2021

Add typing to `scope()` #1170

Add typing to `scope()` #1170

cdce8p commented Sep 13, 2021 •

edited

DanielNoord Sep 15, 2021 •

edited by cdce8p

Pierre-Sassoulas Sep 15, 2021 •

edited

DanielNoord Sep 15, 2021 •

edited

DanielNoord commented Sep 15, 2021 •

edited