Skip to content

Fix regression for _is_only_type_assignment#5163

Merged
Pierre-Sassoulas merged 14 commits intopylint-dev:mainfrom
DanielNoord:regression-5162
Oct 23, 2021
Merged

Fix regression for _is_only_type_assignment#5163
Pierre-Sassoulas merged 14 commits intopylint-dev:mainfrom
DanielNoord:regression-5162

Conversation

@DanielNoord
Copy link
Collaborator

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

Type of Changes

Type
🐛 Bug fix

Description

Not sure if parent is the right name for the variable, but couldn't come up with a better one.

This closes #5162

@DanielNoord DanielNoord added Regression Crash 💥 A bug that makes pylint crash labels Oct 16, 2021
@DanielNoord DanielNoord added this to the 2.12.0 milestone Oct 16, 2021
@DanielNoord DanielNoord requested a review from cdce8p October 16, 2021 11:00
@coveralls
Copy link

coveralls commented Oct 16, 2021

Pull Request Test Coverage Report for Build 1375829491

  • 17 of 17 (100.0%) changed or added relevant lines in 1 file are covered.
  • 111 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.03%) to 93.277%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/typecheck.py 25 95.01%
pylint/checkers/variables.py 35 95.53%
pylint/checkers/classes.py 51 94.57%
Totals Coverage Status
Change from base Build 1352191945: 0.03%
Covered Lines: 13666
Relevant Lines: 14651

💛 - Coveralls

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 'englobing_scope' ?

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the test case you've added with global.
Would you mind adding two more, both with and without error?

  • Comprehension inside a function, similar to #5162
  • Decorator with inner function, similar to #5163 (comment)

--
Lastly, I needed some time to come up this that but assignment expressions aren't covered yet. Maybe that would be better in a new PR though.

def func():
    var: int
    if (var := var ** 2):
        print(var)

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Oct 22, 2021

-- Lastly, I needed some time to come up this that but assignment expressions aren't covered yet. Maybe that would be better in a new PR though.

def func():
    var: int
    if (var := var ** 2):
        print(var)

I have opened #5197 to track this. We need control-flow for this, as we currently can't recognise whether a NamedExpr has been called.
I have added some basic tests for NamedExpr which do work.

Edit: Oops, forgot to separate the assignment expressions tests into a >=3.8 file.

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening the issue. That's definitely not something we can fix at the moment.

Left some comments. One more thing: I noticed that you used nodes.Statement as type annotation for defstmt. Technically this might be correct, but currently it would be an error as it's only recognized as NodeNG. We need to work on the astroid type annotations some more to fix that.

DanielNoord and others added 4 commits October 22, 2021 17:42
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
Comment on lines 1559 to 1561
def _is_only_type_assignment(
node: nodes.Name, defstmt: Union[nodes.Module, nodes.Statement]
) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _is_only_type_assignment(
node: nodes.Name, defstmt: Union[nodes.Module, nodes.Statement]
) -> bool:
def _is_only_type_assignment(node: nodes.Name, defstmt: nodes.NodeNG) -> bool:

I would suggest we use NodeNG for now. Can always update that later. That way we can merge this sooner.
Haven't looked at pylint-dev/astroid#1217 yet, but I feel like this should never be Module in the first place.

https://docs.python.org/3.10/library/ast.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I then add a TODO that links to the astroid issue? I fear we might forget about this in the future if we don't make some comment about it now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would work 🤷🏻‍♂️

DanielNoord and others added 2 commits October 23, 2021 17:55
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks, @DanielNoord 🚀

@Pierre-Sassoulas Pierre-Sassoulas merged commit 8eb7ff1 into pylint-dev:main Oct 23, 2021
@DanielNoord DanielNoord deleted the regression-5162 branch October 23, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Crash 💥 A bug that makes pylint crash Regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash in VariablesChecker._is_only_type_assignment

4 participants