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.

Sign up for GitHub

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

Jump to bottom

Require build class parent #2557

Merged
merged 2 commits into from
Sep 13, 2024
Merged

Require build class parent #2557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80d7ab0
enforce a non-None parent in build_class
temyurchenko Sep 11, 2024
b4b7947
fixup! enforce a non-None parent in build_class
temyurchenko Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions astroid/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@ def safe_infer(


def _build_proxy_class(cls_name: str, builtins: nodes.Module) -> nodes.ClassDef:
proxy = raw_building.build_class(cls_name)
proxy.parent = builtins
proxy = raw_building.build_class(cls_name, builtins)
return proxy


Expand Down
9 changes: 8 additions & 1 deletion astroid/nodes/scoped_nodes/scoped_nodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,13 @@ def function_to_method(n, klass):
return n


def _attach_to_parent(node: NodeNG, name: str, parent: NodeNG):
frame = parent.frame()
frame.set_local(name, node)
if frame is parent:
frame._append_node(node)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like a crucial change that we don't have a test for. Should we add one?

Copy link
Contributor Author

@temyurchenko temyurchenko Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So, usually classes get added to the body of the parent in the positinit constructor. There is only one exception to my knowledge: raw_building._add_dunder_class. It was added solely to support some pyqt inference and it is creating (and attaching) adhoc classes for the purposes of being later detected by the transform process.

That situation is already tested by the three qt tests (which fail without this change). In other situations it's useless, afaics.

Overall, the process described above seems very perverse, the "dunder" class shouldn't be recreated within a function each time. It is its own class that is defined outside. We should rather consult the type annotation in the transform process. However, it is a question for another PR.



class Module(LocalsDictNodeNG):
"""Class representing an :class:`ast.Module` node.

Expand Down Expand Up @@ -1935,7 +1942,7 @@ def __init__(
parent=parent,
)
if parent and not isinstance(parent, Unknown):
parent.frame().set_local(name, self)
_attach_to_parent(self, name, parent)

for local_name, node in self.implicit_locals():
self.add_local_node(node, local_name)
Expand Down
25 changes: 12 additions & 13 deletions astroid/raw_building.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,15 @@ def _attach_local_node(parent, node, name: str) -> None:
parent.add_local_node(node)


def _add_dunder_class(func, member) -> None:
def _add_dunder_class(func, parent: nodes.NodeNG, member) -> None:
"""Add a __class__ member to the given func node, if we can determine it."""
python_cls = member.__class__
cls_name = getattr(python_cls, "__name__", None)
if not cls_name:
return
cls_bases = [ancestor.__name__ for ancestor in python_cls.__bases__]
doc = python_cls.__doc__ if isinstance(python_cls.__doc__, str) else None
ast_klass = build_class(cls_name, cls_bases, doc)
ast_klass = build_class(cls_name, parent, cls_bases, doc)
func.instance_attrs["__class__"] = [ast_klass]


Expand Down Expand Up @@ -97,7 +97,10 @@ def build_module(name: str, doc: str | None = None) -> nodes.Module:


def build_class(
name: str, basenames: Iterable[str] = (), doc: str | None = None
name: str,
parent: nodes.NodeNG,
basenames: Iterable[str] = (),
doc: str | None = None,
) -> nodes.ClassDef:
"""Create and initialize an astroid ClassDef node."""
node = nodes.ClassDef(
Expand All @@ -106,7 +109,7 @@ def build_class(
col_offset=0,
end_lineno=0,
end_col_offset=0,
parent=nodes.Unknown(),
parent=parent,
)
node.postinit(
bases=[
Expand Down Expand Up @@ -343,7 +346,7 @@ def object_build_methoddescriptor(
getattr(member, "__name__", None) or localname, doc=member.__doc__
)
node.add_local_node(func, localname)
_add_dunder_class(func, member)
_add_dunder_class(func, node, member)


def _base_class_object_build(
Expand All @@ -359,9 +362,8 @@ def _base_class_object_build(
class_name = name or getattr(member, "__name__", None) or localname
assert isinstance(class_name, str)
doc = member.__doc__ if isinstance(member.__doc__, str) else None
klass = build_class(class_name, basenames, doc)
klass = build_class(class_name, node, basenames, doc)
klass._newstyle = isinstance(member, type)
node.add_local_node(klass, localname)
try:
# limit the instantiation trick since it's too dangerous
# (such as infinite test execution...)
Expand Down Expand Up @@ -603,14 +605,11 @@ def _astroid_bootstrapping() -> None:

for cls, node_cls in node_classes.CONST_CLS.items():
if cls is TYPE_NONE:
proxy = build_class("NoneType")
proxy.parent = astroid_builtin
proxy = build_class("NoneType", astroid_builtin)
elif cls is TYPE_NOTIMPLEMENTED:
proxy = build_class("NotImplementedType")
proxy.parent = astroid_builtin
proxy = build_class("NotImplementedType", astroid_builtin)
elif cls is TYPE_ELLIPSIS:
proxy = build_class("Ellipsis")
proxy.parent = astroid_builtin
proxy = build_class("Ellipsis", astroid_builtin)
else:
proxy = astroid_builtin.getattr(cls.__name__)[0]
assert isinstance(proxy, nodes.ClassDef)
Expand Down
3 changes: 2 additions & 1 deletion tests/brain/test_brain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1705,7 +1705,8 @@ def test_infer_dict_from_keys() -> None:
)
for node in bad_nodes:
with pytest.raises(InferenceError):
next(node.infer())
if isinstance(next(node.infer()), util.UninferableBase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this one, I don't know exactly, but my intuition was the following. Before, when we had Unknown for a parent, when inference encountered Unknown, it would raise (InferenceError). Now, there is no Unknown, but the calls still aren't correct. So, we just fail to infer and yield Uninferable.

However, both responses kind of seem to make sense, so I opted to allow either of the responses (InferenceError or Uninferable) to pass the tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we be certain that it is either of the two? So, just assert that it is always Uniferable or always the exception? If so, I'd prefer just testing for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, right now it's always Uninferable. I'll change to test specifically for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

@temyurchenko temyurchenko Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, one test fails on pypy with InferenceError. I'd rather go with the initial pytest.raises clause then

raise InferenceError

# Test uninferable values
good_nodes = astroid.extract_node(
Expand Down
3 changes: 1 addition & 2 deletions tests/test_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ def _extract(self, obj_name: str) -> ClassDef:
return self.builtins.getattr(obj_name)[0]

def _build_custom_builtin(self, obj_name: str) -> ClassDef:
proxy = raw_building.build_class(obj_name)
proxy.parent = self.builtins
proxy = raw_building.build_class(obj_name, self.builtins)
return proxy

def assert_classes_equal(self, cls: ClassDef, other: ClassDef) -> None:
Expand Down
4 changes: 3 additions & 1 deletion tests/test_raw_building.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
build_module,
)

DUMMY_MOD = build_module("DUMMY")


class RawBuildingTC(unittest.TestCase):
def test_attach_dummy_node(self) -> None:
Expand All @@ -48,7 +50,7 @@ def test_build_module(self) -> None:
self.assertEqual(node.parent, None)

def test_build_class(self) -> None:
node = build_class("MyClass")
node = build_class("MyClass", DUMMY_MOD)
self.assertEqual(node.name, "MyClass")
self.assertEqual(node.doc_node, None)

Expand Down
Loading