Add ``future`` argument to all ``NodeNG.statement()`` calls by DanielNoord · Pull Request #1235 · pylint-dev/astroid

DanielNoord · 2021-11-07T21:52:37Z

Steps

Write a good description on what the PR does.

Description

Follow up to #1217

This makes all calls to NodeNG.statement use the future keyword argument added in #1217.

There is one case where this is not as straightforward because the code depends on nodes.Module.statement() returning itself. This is why I have marked this as a draft. I will comment on the relevant line.

Closes #1239

Type of Changes

	Type
✓	🔨 Refactoring

Related Issue

DanielNoord · 2021-11-07T21:55:51Z

astroid/nodes/node_classes.py

+        try:
+            mystmt = self.statement(future=True)
+        except StatementMissing:
+            mystmt = self


@cdce8p These are the lines I referred to. Both calls to self.statement() will raise a StatementMissing exception when called on a nodes.Module. By adding a check for self.parent, which nodes.Module don't have, and a try ... except we can catch this.
However, similar to the actual change in #1217 I wonder if mystmt should actually become a nodes.Module in the first place (as it would still be with this change). You probably have a better idea if this is the way to go about fixing this.

Note that nodes.Module can't be imported due to circular imports so we can't do isinstance() here.

I had some time to think about it. Did I make made a mistake by suggesting to raise an error if statement() is called on Module?

Some points to consider

statement() is often used together with frame() and thus doesn't quite represent an actual ast.Statement. It's rather used to compare if the statement() of a node is also equal to the frame(). Thus returning Module made sense in that context.

During a normal pylint run you didn't encounter an AttributeError as a parent had always been defined or it was a Module which returned itself. To update pylint, we would need to add exception handling in at least some / most (?) cases.

Just from looking at the changes here, I'm not sure it's an actual improvement.

@DanielNoord What do you think?

@DanielNoord Sorry to ping you again so soon. If we follow though and revert the change, I think if would be good to do so before pylint 2.12.0 is released. Reverting it would also address #1239.

/CC: @Pierre-Sassoulas

If we need to revert that we might as well do it quick as the deprecation warning exists in 2.8.5 that has been released, it's better if it's reverted fast. I did not follow this close enough to have an opinion about it, so I trust your decision on this. Re-releasing astroid with a revert is cheap.

I would recommend doing that. Would like to know what @DanielNoord thinks first as he also spend considerable time with the original change. One or two days more doesn't really matter too much, just the issue with the 2.12 release.

I haven't gotten the time to look at this sufficiently. Should be able to do so tomorrow.

My initial feeling is that we should probably try and find a way to allow the precious behaviour but not in statement. statement returning Modules is completely unexpected based on the methods name and that confusion is what caused this in the first place. I think it is valuable to avoid such unlogical behaviour.

I haven't looked into why we need to return modules exactly in the first place, @cdce8p already mentioned it but haven't fully read the initial comment. But if this is indeed a valid use case, perhaps we need a module_and_statement()?

I do agree. The name statement isn't ideal.
However as for changing it, I don't think it's worth it. We don't gain anything and just break code in the process. If there were a clear advantage, maybe. But even then there is a point to be made that the function should stay the same and we should instead add a new one that only returns actual Statement nodes. That would be backwards compatible.

Just as this PR my response is WIP. But;

I have looked at _filter_stmts, which is the only function causing problems in astroid with future=True.
https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L401

mystmt is used on some occasions, which I will discuss to show what happens when mystmt is in fact a nodes.Module.
https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L448

nodes.Module.fromlineno is always 0. Therefore, this check will never be True.

https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L465

stmt will never be a nodes.Module since _filter_stmts is only called with self.locals[name]. This will never include a nodes.Module so this check will never be True.

https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L476

_get_filtered_stmts is not defined on nodes.Module. All three definitions of _get_filtered_stmts rely on comparing the self of _get_filtered_stmts to mystmt or calling self.statement() and comparing to mystmt. Since self is never nodes.Module this will never return True and done will never be True.

https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L569

Since nodes.Module doesn't have a parent this will never be True.

So, to me it seems that the return of self for nodes.Module is done to avoid the AttributeError but is not meaningful in any way. Initialising mystmt as None and only reassigning if self.parent as I do in the commit I just added seems fine for astroid.

I tested these changes locally with pylint and all tests pass.

Then there is the issue of whether the behaviour of future is compatible with pylint. I am going to investigate this now and see whether we really need statement() to return nodes.Module. I agree that adding try ... except everywhere would not be good, but seeing as the change to astroid is relatively simple I think we might get away with it.

I think pylint-dev/pylint#5310 shows that the effect of no longer returning nodes.Module for pylint is minimal as well.
That PR adds one isinstance(nodes.Module) (related to nodes.Module.fromlineno), one if XXX.parent and changes one except AttributeError to except StatementMissing. I tested that PR against the latest commit of astroid in this branch and all tests passed.

Based on this I think we do not need to revert the earlier change.

We never need statement() to return nodes.Module, both in pylint and in astroid. There is no if statement in any of the projects that evaluates to True whenever the method does return a nodes.Module. I think the return self was added to avoid creating crashes when AttributeError was raised or might have served a purpose it no longer does.

tests/unittest_builder.py

…roid into statement-future

…t-future

Pierre-Sassoulas

Very nice ! I just realized that we should have done that before releasing 2.8.5 as we already had this issue signaled to us previousely

cdce8p

I don't have the bandwidth at the moment to think about this further. It looks like you have done the research and it'll work out. So let's finish it then.

Could you also add quotes around NoReturn in the two places linked below? That would fix #1239 https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_ng.py#L278
https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/scoped_nodes.py#L665

astroid/nodes/node_ng.py

cdce8p

Overall this looks good. Could you add a one line regarding the NoReturn fix? Either here or, as mentioned in the comment, in a new PR.

astroid/nodes/node_classes.py

astroid/nodes/node_ng.py

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

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

DanielNoord · 2021-11-17T23:27:17Z

@Pierre-Sassoulas Should we do this before the primer? Or do you want to wait?

…t-future

…roid into statement-future

DanielNoord · 2021-11-24T12:45:46Z

@Pierre-Sassoulas Now that primer is in place I think this can be merged! I'll leave that to you :)

…ev#1235) * Add ``future`` argument to all ``NodeNG.statement()`` calls * Add typing from ``statement()`` Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

Add future argument to all NodeNG.statement() calls

b9b4d47

DanielNoord added the Maintenance Discussion or action around maintaining astroid or the dev workflow label Nov 7, 2021

DanielNoord added this to the 2.8.5 milestone Nov 7, 2021

DanielNoord requested a review from cdce8p November 7, 2021 21:52

DanielNoord commented Nov 7, 2021

View reviewed changes

tests/unittest_builder.py Outdated Show resolved Hide resolved

Update tests/unittest_builder.py

44c87f0

Pierre-Sassoulas modified the milestones: 2.8.5, 2.8.6 Nov 12, 2021

DanielNoord added 3 commits November 15, 2021 14:26

Fix _filter_stmts

1c8344f

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

6bd03c9

…roid into statement-future

Merge branch 'main' of https://github.com/PyCQA/astroid into statemen…

2d9d2cd

…t-future

DanielNoord mentioned this pull request Nov 15, 2021

Add future=True to all NodeNG.statement() calls pylint-dev/pylint#5310

Merged

2 tasks

DanielNoord marked this pull request as ready for review November 15, 2021 14:22

Pierre-Sassoulas approved these changes Nov 15, 2021

View reviewed changes

cdce8p reviewed Nov 15, 2021

View reviewed changes

Add quotes to NoReturn

9ea8f01

cdce8p reviewed Nov 15, 2021

View reviewed changes

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

DanielNoord mentioned this pull request Nov 15, 2021

Make unused-import check all ancestors for typing guards pylint-dev/pylint#5316

Merged

4 tasks

DanielNoord added 2 commits November 15, 2021 22:46

Change import

e5ae4e1

Add typing from statement()

3c05efc

DanielNoord requested a review from cdce8p November 17, 2021 13:19

cdce8p reviewed Nov 17, 2021

View reviewed changes

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

astroid/nodes/node_ng.py Show resolved Hide resolved

DanielNoord and others added 2 commits November 18, 2021 00:06

Update astroid/nodes/node_classes.py

8f5e8e9

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

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

d97b34e

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

DanielNoord mentioned this pull request Nov 17, 2021

Change NoReturn to be encapsulated in strings #1252

Merged

2 tasks

DanielNoord added 2 commits November 18, 2021 09:29

Merge branch 'main' of https://github.com/PyCQA/astroid into statemen…

4a6c601

…t-future

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

e717b73

…roid into statement-future

Pierre-Sassoulas removed this from the 2.8.6 milestone Nov 21, 2021

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

Pierre-Sassoulas merged commit 4443159 into pylint-dev:main Nov 24, 2021

DanielNoord deleted the statement-future branch November 24, 2021 12:50

DanielNoord mentioned this pull request Dec 14, 2021

Fix frame() error on inferred node #1263

Merged

DanielNoord mentioned this pull request Dec 29, 2021

Add future=True to frame calls #1305

Merged

Uh oh!

Conversation

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

Uh oh!

Steps

Description

Type of Changes

Related Issue

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p Nov 13, 2021

Choose a reason for hiding this comment

Uh oh!

cdce8p Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

cdce8p Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

DanielNoord Nov 14, 2021

Choose a reason for hiding this comment

Uh oh!

cdce8p Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

DanielNoord Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DanielNoord commented Nov 17, 2021

Uh oh!

DanielNoord commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DanielNoord commented Nov 7, 2021 •

edited

Loading

DanielNoord Nov 7, 2021 •

edited

Loading

DanielNoord Nov 15, 2021 •

edited

Loading