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

Audit the correctness of adding parent.name to child's locals in ClassDef.__init__() and FunctionDef.__init__() #1490

Closed
jacobtylerwalls opened this issue Mar 26, 2022 · 7 comments
Assignees
Milestone

Comments

@jacobtylerwalls
Copy link
Member

jacobtylerwalls commented Mar 26, 2022

Apparently, bugs can be caused by providing the parent argument to ClassDef.__init__(), because the name of that node (the ancestor) will be added to the locals of the child node, and this may not be intended (it wasn't intended in the case of subclassing a namedtuple. We need to audit whether it is happening anywhere else and if that behavior is correct.

See discussion at #1489 (comment).

@jacobtylerwalls
Copy link
Member Author

Caused another false positive at pylint-dev/pylint#6895

@jacobtylerwalls
Copy link
Member Author

I guess next step is to see what fails / depends on the current behavior.

@DanielNoord
Copy link
Collaborator

I guess next step is to see what fails / depends on the current behavior.

Commenting out the relevant lines crashes already in the bootstrapping of the str ClassDef. So, it seems like this this is necessary for 90% of the cases.

I wonder if we can come up with some sort of rule for the cases where it shouldn't be added. Is there some sort of commonality between the currently identified cases?

@jacobtylerwalls jacobtylerwalls added the Brain 🧠 Needs a brain tip label Jun 10, 2022
@jacobtylerwalls
Copy link
Member Author

I think it might only be a "gotcha" when creating fake nodes in the brain. When creating a fake ClassDef from scratch in the namespace brain, there's no logical parent, since we don't have the argparse module -- we could probably get away with not setting a parent at all.

@DanielNoord
Copy link
Collaborator

I think it might only be a "gotcha" when creating fake nodes in the brain. When creating a fake ClassDef from scratch in the namespace brain, there's no logical parent, since we don't have the argparse module -- we could probably get away with not setting a parent at all.

We have deprecated parent == None for any node other than nodes.Module. I think this is beneficial in pylint as it removes a lot of isinstance checks.

@jacobtylerwalls jacobtylerwalls added this to the 2.16.0 milestone Apr 15, 2023
@jacobtylerwalls jacobtylerwalls changed the title Audit the correctness of adding parent.name to child's locals in ClassDef.__init__() Audit the correctness of adding parent.name to child's locals in ClassDef.__init__() and FunctionDef.__init__() Apr 15, 2023
@jacobtylerwalls
Copy link
Member Author

Same problem with inferring calls to property(), which creates a Property (inherited from FunctionDef.

This elaborate workaround is not observed when inferring a property:

astroid/astroid/inference.py

Lines 1246 to 1267 in ee2d4bd

# When inferring a property, we instantiate a new `objects.Property` object,
# which in turn, because it inherits from `FunctionDef`, sets itself in the locals
# of the wrapping frame. This means that every time we infer a property, the locals
# are mutated with a new instance of the property. To avoid this, we detect this
# scenario and avoid passing the `parent` argument to the constructor.
parent_frame = self.parent.frame(future=True)
property_already_in_parent_locals = self.name in parent_frame.locals and any(
isinstance(val, objects.Property) for val in parent_frame.locals[self.name]
)
# We also don't want to pass parent if the definition is within a Try node
if isinstance(self.parent, (nodes.TryExcept, nodes.TryFinally, nodes.If)):
property_already_in_parent_locals = True
prop_func = objects.Property(
function=self,
name=self.name,
lineno=self.lineno,
parent=self.parent if not property_already_in_parent_locals else None,
col_offset=self.col_offset,
)
if property_already_in_parent_locals:
prop_func.parent = self.parent

@jacobtylerwalls
Copy link
Member Author

jacobtylerwalls commented Sep 30, 2024

Fixed in #2588, can remove any related workarounds later as Refs #1490...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants