Skip to content

Fix regression on ClassDef inference #1181

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

Merged
merged 2 commits into from
Sep 17, 2021
Merged

Conversation

DanielNoord
Copy link
Collaborator

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

Type of Changes

Type
βœ“ πŸ› Bug fix

Related Issue

Closes pylint-dev/pylint#5030
Closes pylint-dev/pylint#5036

@DanielNoord
Copy link
Collaborator Author

I'm not sure about the location of the unit test. Might need to be placed in a different place, but I did not know where.

@DanielNoord DanielNoord added this to the 2.8.1 milestone Sep 17, 2021
Comment on lines 181 to 182
if node.keywords:
args += [n.accept(self) for n in node.keywords]
Copy link
Member

Choose a reason for hiding this comment

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

node.keywords should never be None in the first place. It should always be a list, even if empty.
This should be fixed in ClassDef.postinit with

if keywords is not None:
    self.keywords = keywords

https://github.com/PyCQA/astroid/blob/28411ee8575562b7d7d1d0639d4e61f91ed1586b/astroid/nodes/scoped_nodes.py#L2099

--

ClassDef(identifier name,
             expr* bases,
             keyword* keywords,
             stmt* body,
             expr* decorator_list)

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.

Before merging this, do we want to identify why this regressed? Or is fixing the postinit good enough in your opinion?

Copy link
Member

Choose a reason for hiding this comment

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

The fix is good enough for me. Did some digging nevertheless.

That was an existing issue that only with the last update started causing problems. #979 added inference for Compare nodes. The inference method uses a clever workaround to get to the actual value of a node.
https://github.com/PyCQA/astroid/blob/28411ee8575562b7d7d1d0639d4e61f91ed1586b/astroid/inference.py#L811-L814

Calling as_string on any NodeNG should be safe. Multiple ClassDef.postinit calls in astroid/brain/brain_typing.py don't pass a keywords argument and thus use the default None.
https://github.com/PyCQA/astroid/blob/28411ee8575562b7d7d1d0639d4e61f91ed1586b/astroid/brain/brain_typing.py#L350-L354

I wouldn't add keywords=[] to these calls as a default value is expected to work fine. Thus this was an error in the postinit method which is now fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the deep-dive! Feels good to know that we are not using a work-around to fix a problem but rather a fixing an actual problem, which was what I was afraid of!

Copy link
Member

Choose a reason for hiding this comment

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

I agree

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.

LGTM!

@cdce8p cdce8p merged commit 7eedd68 into pylint-dev:main Sep 17, 2021
@DanielNoord DanielNoord deleted the fix-as-string branch September 17, 2021 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash 'NoneType' object is not iterable Fatal error with dataclass after 2.11
2 participants