-
-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add future
argument to all NodeNG.statement()
calls
#1235
Merged
Pierre-Sassoulas
merged 12 commits into
pylint-dev:main
from
DanielNoord:statement-future
Nov 24, 2021
Merged
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
b9b4d47
Add ``future`` argument to all ``NodeNG.statement()`` calls
DanielNoord 44c87f0
Update tests/unittest_builder.py
DanielNoord 1c8344f
Fix `_filter_stmts`
DanielNoord 6bd03c9
Merge branch 'statement-future' of https://github.com/DanielNoord/ast…
DanielNoord 2d9d2cd
Merge branch 'main' of https://github.com/PyCQA/astroid into statemen…
DanielNoord 9ea8f01
Add quotes to NoReturn
DanielNoord e5ae4e1
Change import
DanielNoord 3c05efc
Add typing from ``statement()``
DanielNoord 8f5e8e9
Update astroid/nodes/node_classes.py
DanielNoord d97b34e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 4a6c601
Merge branch 'main' of https://github.com/PyCQA/astroid into statemen…
DanielNoord e717b73
Merge branch 'statement-future' of https://github.com/DanielNoord/ast…
DanielNoord File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdce8p These are the lines I referred to. Both calls to
self.statement()
will raise aStatementMissing
exception when called on anodes.Module
. By adding a check forself.parent
, whichnodes.Module
don't have, and atry ... except
we can catch this.However, similar to the actual change in #1217 I wonder if
mystmt
should actually become anodes.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 doisinstance()
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some time to think about it. Did I make made a mistake by suggesting to raise an error if
statement()
is called onModule
?Some points to consider
statement()
is often used together withframe()
and thus doesn't quite represent an actualast.Statement
. It's rather used to compare if thestatement()
of a node is also equal to theframe()
. Thus returningModule
made sense in that context.pylint
run you didn't encounter anAttributeError
as a parent had always been defined or it was aModule
which returned itself. To updatepylint
, we would need to add exception handling in at least some / most (?) cases.@DanielNoord What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as this PR my response is WIP. But;
I have looked at
_filter_stmts
, which is the only function causing problems inastroid
withfuture=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 whenmystmt
is in fact anodes.Module
.https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L448
nodes.Module.fromlineno
is always0
. Therefore, this check will never beTrue
.https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L465
stmt
will never be anodes.Module
since_filter_stmts
is only called withself.locals[name]
. This will never include anodes.Module
so this check will never beTrue
.https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L476
_get_filtered_stmts
is not defined onnodes.Module
. All three definitions of_get_filtered_stmts
rely on comparing theself
of_get_filtered_stmts
tomystmt
or callingself.statement()
and comparing tomystmt
. Sinceself
is nevernodes.Module
this will never returnTrue
anddone
will never beTrue
.https://github.com/PyCQA/astroid/blob/30711bc012592844a902d1d5912b3b2b6f2dce0f/astroid/nodes/node_classes.py#L569
Since
nodes.Module
doesn't have aparent
this will never beTrue
.So, to me it seems that the return of
self
fornodes.Module
is done to avoid theAttributeError
but is not meaningful in any way. Initialisingmystmt
asNone
and only reassigningif self.parent
as I do in the commit I just added seems fine forastroid
.I tested these changes locally with
pylint
and all tests pass.Then there is the issue of whether the behaviour of
future
is compatible withpylint
. I am going to investigate this now and see whether we really needstatement()
to returnnodes.Module
. I agree that addingtry ... except
everywhere would not be good, but seeing as the change toastroid
is relatively simple I think we might get away with it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pylint-dev/pylint#5310 shows that the effect of no longer returning
nodes.Module
forpylint
is minimal as well.That PR adds one
isinstance(nodes.Module)
(related tonodes.Module.fromlineno
), oneif XXX.parent
and changes oneexcept AttributeError
toexcept StatementMissing
. I tested that PR against the latest commit ofastroid
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 returnnodes.Module
, both inpylint
and inastroid
. There is no if statement in any of the projects that evaluates toTrue
whenever the method does return anodes.Module
. I think thereturn self
was added to avoid creating crashes whenAttributeError
was raised or might have served a purpose it no longer does.