Add ``future=True`` to all ``NodeNG.statement()`` calls by DanielNoord · Pull Request #5310 · pylint-dev/pylint

DanielNoord · 2021-11-15T14:15:46Z

Add yourself to CONTRIBUTORS if you are a new contributor.
Write a good description on what the PR does.

Type of Changes

	Type
✓	🔨 Refactoring

Description

Ref pylint-dev/astroid#1235

coveralls · 2021-11-15T14:45:14Z

Pull Request Test Coverage Report for Build 1586518271

18 of 18 (100.0%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.004%) to 93.646%

Files with Coverage Reduction	New Missed Lines	%
pylint/checkers/typecheck.py	1	94.88%

Totals
Change from base Build 1583088214:	-0.004%
Covered Lines:	14236
Relevant Lines:	15202

💛 - Coveralls

pylint/checkers/variables.py

DanielNoord · 2021-11-24T12:49:59Z

Opening and closing to run primer.

@Pierre-Sassoulas I put this in 2.12.2 but since we don't actually change any code here (only change AttributeError to StatementMissing + one new and node_scope.parent) this could also go in 2.12 without much risk of breaking things.
It would stop the warnings from displaying in our tests.

Pierre-Sassoulas

👍

Pierre-Sassoulas · 2021-11-24T13:08:27Z

primers bringing value again 😄

Pierre-Sassoulas · 2021-11-24T14:23:48Z

Moving to 2.13 because of the test fail (could be 2.12.x too).

DanielNoord · 2021-11-24T14:24:46Z

Looking into crash as we speak!

DanielNoord · 2021-11-24T15:37:11Z

Blocked by #5382.

pylint/checkers/format.py

DanielNoord · 2021-12-13T21:57:26Z

pylint/checkers/typecheck.py

                    continue
+            except exceptions.StatementMissing:
+                continue
            except AttributeError:


This is also catching another AttributeError besides the missing parent attribute that is now captured in exceptions.StatementMissing. Thanks to the primer for finding this potential crash before committing to main 😄

DanielNoord · 2021-12-13T21:58:48Z

pylint/checkers/variables.py

        scope = node.scope()
        if isinstance(scope, nodes.FunctionDef) and any(
-            asmt.statement().parent_of(scope) for asmt in astmts
+            asmt.scope().parent_of(scope) for asmt in astmts


We want to check if the scope of asmt is the parent scope of node's scope. Previously this relied on nodes.Module.statement() returning itself. Using scope seems to be more sensical here.

DanielNoord · 2021-12-13T22:00:05Z

pylint/checkers/variables.py

+                astmts[0].is_statement
+                or not isinstance(astmts[0].parent, nodes.Module)
+                and astmts[0].statement(future=True).parent_of(node)
+            )


This was particularly trigger to get right, but this is testing what we want. If parent is the root Module and the parent of node we don't do _astmts = astmts[:1]. The other or is simply what we did previously.

Tbh I think you know more about this particular method than I do. It looks like it could work 😅

DanielNoord · 2021-12-13T22:00:37Z

tests/functional/r/regression_02/regression_node_statement.py

+
+
+else:
+    ellipsis = type(Ellipsis)


This is a regression test for code I found while working on this.

DanielNoord · 2021-12-13T22:13:07Z

pylint/checkers/format.py

        ):
            prev_line = node.parent.body[0].tolineno + 1
+        elif isinstance(node.parent, nodes.Module):
+            prev_line = 0


This is the same effect as doing node.parent.fromlineno but avoids the StatementMissing exception from calling statement.

DanielNoord · 2021-12-13T22:26:49Z

@cdce8p This is finally ready for review 🎉

Pierre-Sassoulas

LGTM, just some small suggestions.

pylint/checkers/variables.py

tests/functional/r/regression_02/regression_node_statement.py

Pierre-Sassoulas

LGTM, but I'm going to wait for @cdce8p review to merge.

cdce8p · 2021-12-15T23:23:22Z

pylint/checkers/typecheck.py


 import astroid
-from astroid import bases, nodes
+from astroid import bases, exceptions, nodes


I'm not sure we should use this import here.
There is a pylint.exceptions package too which is often imported with

from pylint import exceptions

Thus doing the same for astroid will lead to name conflicts.

At some point @Pierre-Sassoulas and I discussed this I think, although we didn't reach a conclusion for a good name. For the time being, I would recommend this

import astroid.exceptions try: ... except astroid.exceptions...: ...

It's a bit lengthy but already used elsewhere. Should be good enough util we find something else.

I have added an import astroid.exceptions line. There is already a import astroid statement, but astroid.exceptions.StatementMissing could not be resolved by VS Code (/ Pylance).. I think because of the wildcard import in __init__.py in astroid.

cdce8p · 2021-12-15T23:26:05Z

pylint/checkers/typecheck.py

+            except exceptions.StatementMissing:
+                continue


After the discussion about not adding the FrameMissing error in astroid, I've been thinking if we should deprecate / remove the StatementMissing error, too.

Is there a benefit here with it being more specific? I would probably recommend to just catch ParentMissingError here.

I'm not sure if there is any benefit, perhaps when doing something like [(i.frame(), j.statement()) for i,j in zip(nodes, other_nodes)] but that is really really convoluted...
On the other hand, we have already gone to the effort of adding it and I don't think it adds too much boilerplate to the astroid codebase. We never know when we might need it in the future so I'd say let's just keep it in?

Ok, let's keep it. Only because it's already in astroid 😉

cdce8p · 2021-12-15T23:38:31Z

pylint/checkers/variables.py

+                astmts[0].is_statement
+                or not isinstance(astmts[0].parent, nodes.Module)
+                and astmts[0].statement(future=True).parent_of(node)
+            )


Tbh I think you know more about this particular method than I do. It looks like it could work 😅

into statement-future

DanielNoord added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Nov 15, 2021

DanielNoord mentioned this pull request Nov 15, 2021

Add future argument to all NodeNG.statement() calls pylint-dev/astroid#1235

Merged

1 task

Pierre-Sassoulas added this to the 2.13.0 milestone Nov 15, 2021

Pierre-Sassoulas reviewed Nov 16, 2021

View reviewed changes

pylint/checkers/variables.py Show resolved Hide resolved

DanielNoord force-pushed the statement-future branch from 9db3e0f to 810d649 Compare November 16, 2021 09:07

DanielNoord modified the milestones: 2.13.0, 2.12.2 Nov 16, 2021

DanielNoord closed this Nov 24, 2021

DanielNoord reopened this Nov 24, 2021

Pierre-Sassoulas modified the milestones: 2.12.2, 2.12.0 Nov 24, 2021

Pierre-Sassoulas mentioned this pull request Nov 24, 2021

Update reporters to (allow) use of end_line and end_column #5372

Merged

4 tasks

Pierre-Sassoulas approved these changes Nov 24, 2021

View reviewed changes

Pierre-Sassoulas modified the milestones: 2.12.0, 2.13.0 Nov 24, 2021

DanielNoord mentioned this pull request Nov 24, 2021

Crash on combination of decorators and properties #5382

Closed

DanielNoord marked this pull request as draft November 24, 2021 15:36

DanielNoord added the Blocked 🚧 Blocked by a particular issue label Nov 24, 2021

Pierre-Sassoulas added Blocked 🚧 Blocked by a particular issue and removed Blocked 🚧 Blocked by a particular issue labels Dec 3, 2021

DanielNoord force-pushed the statement-future branch from a92e5dc to 74bda43 Compare December 4, 2021 11:05

Add future=True to all NodeNG.statement() calls

0bd10b0

DanielNoord force-pushed the statement-future branch from 77c959e to 0bd10b0 Compare December 13, 2021 21:34

Potentially fix bugs?

d0bef5a

DanielNoord commented Dec 13, 2021

View reviewed changes

Update pylint/checkers/format.py

3ca5398

DanielNoord commented Dec 13, 2021

View reviewed changes

DanielNoord removed the Blocked 🚧 Blocked by a particular issue label Dec 13, 2021

DanielNoord requested a review from cdce8p December 13, 2021 22:26

DanielNoord marked this pull request as ready for review December 13, 2021 22:27

Pierre-Sassoulas requested changes Dec 14, 2021

View reviewed changes

pylint/checkers/variables.py Show resolved Hide resolved

tests/functional/r/regression_02/regression_node_statement.py Outdated Show resolved Hide resolved

DanielNoord added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Dec 15, 2021

Update tests/functional/r/regression_02/regression_node_statement.py

673a74a

Pierre-Sassoulas approved these changes Dec 15, 2021

View reviewed changes

Merge branch 'main' into statement-future

6e17580

cdce8p requested changes Dec 15, 2021

View reviewed changes

cdce8p removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Dec 15, 2021

DanielNoord added 2 commits December 16, 2021 09:20

Code review

fe4b5ab

Merge branch 'statement-future' of https://github.com/DanielNoord/pylint

0d18355

into statement-future

cdce8p approved these changes Dec 16, 2021

View reviewed changes

DanielNoord merged commit c285fae into pylint-dev:main Dec 16, 2021

DanielNoord deleted the statement-future branch December 31, 2021 13:27

Uh oh!

Conversation

DanielNoord commented Nov 15, 2021

Type of Changes

Description

Uh oh!

coveralls commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1586518271

💛 - Coveralls

Uh oh!

Uh oh!

DanielNoord commented Nov 24, 2021

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas commented Nov 24, 2021

Uh oh!

Pierre-Sassoulas commented Nov 24, 2021

Uh oh!

DanielNoord commented Nov 24, 2021

Uh oh!

DanielNoord commented Nov 24, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielNoord commented Dec 13, 2021

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Nov 15, 2021 •

edited

Loading