Skip to content
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

Avoid adding the name of a parent namedtuple to child's locals #1489

Merged
merged 1 commit into from
Mar 26, 2022

Conversation

jacobtylerwalls
Copy link
Member

Description

When subclassing a namedtuple, the behavior before d30592a was for astroid to avoid adding the name of the namedtuple to the child class's locals. Apparently doing so caused the crash in pylint-dev/pylint#5982.

Type of Changes

Type
🐛 Bug fix

Related Issue

Refs pylint-dev/pylint#5982

@jacobtylerwalls jacobtylerwalls added this to the 2.11.2 milestone Mar 26, 2022
Comment on lines +137 to +140
# A typical ClassDef automatically adds its name to the parent scope,
# but doing so causes problems, so defer setting parent until after init
# see: https://github.com/PyCQA/pylint/issues/5982
class_node.parent = node.parent
Copy link
Member Author

Choose a reason for hiding this comment

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

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2044799690

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0009%) to 91.489%

Totals Coverage Status
Change from base Build 2039365401: 0.0009%
Covered Lines: 9083
Relevant Lines: 9928

💛 - Coveralls

@jacobtylerwalls jacobtylerwalls added the pylint-tested PRs that don't cause major regressions with pylint label Mar 26, 2022
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.

👌

@Pierre-Sassoulas
Copy link
Member

I'm going to release the hot fix in 2.11.2 / 2.13.2, when @DanielNoord is available for review we can integrate his review in 2.12.

@DanielNoord
Copy link
Collaborator

I'm not sure this is correct. Doesn't this code check something tricky?

from collections import namedtuple

class X(namedtuple("X", ["a", "b", "c"])):
    pass

I believe X should be in the locals of the ClassDef as that X is referring to NamedTuple X.

For:

from collections import namedtuple

class Y(namedtuple("X", ["a", "b", "c"])):
    pass

X should also be in the locals of Y.

(Pdb) klass.locals
{'__module__': [<Const.str l.4 at 0x10ebbfa30>], '__qualname__': [<Const.str l.4 at 0x10ebbf940>], 'X': [<ClassDef.X l.4 at 0x10ebbfb80>]}
(Pdb) klass
<ClassDef.X l.4 at 0x10ebbfd60>
(Pdb) klass.locals["X"]
[<ClassDef.X l.4 at 0x10ebbfb80>]
(Pdb) klass.locals["X"] == klass
False

Note that these are two different ClassDefs. Their address doesn't match.

@jacobtylerwalls
Copy link
Member Author

Yes, they are different ClassDefs. But why should the name of the parent class be in the child class's locals? It's not even in the parent class's locals, is it?

>>> X = namedtuple("X", [])
>>> 'X' in dir(X)
False

So why would we want 'X' to be in the locals of Y when it's not even in X?

@DanielNoord
Copy link
Collaborator

Yes, they are different ClassDefs. But why should the name of the parent class be in the child class's locals? It's not even in the parent class's locals, is it?

>>> X = namedtuple("X", [])
>>> 'X' in dir(X)
False

So why would we want 'X' to be in the locals of Y when it's not even in X?

Ah yeah, didn't think about that.

It's getting late here, but are there any other examples where something gets defined in the "ancestors space" of a class definition? Do we add them as well?
I guess the fix is actually correct then, but it does feel a bit weird. Like, wouldn't it be better to handle this in the __init__ instead of arbitrary not passing parent?

@jacobtylerwalls
Copy link
Member Author

Like, wouldn't it be better to handle this in the init instead of arbitrary not passing parent?

Oh it definitely feels janky. I was just trying to restore status quo before the commit that caused the failure. You're right this could be audited and fixed in the __init__.

@DanielNoord
Copy link
Collaborator

Would you mind opening an issue for it? Then at least we don't forget about it :)

@jacobtylerwalls
Copy link
Member Author

done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants