Require build class parent #2557
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2557 +/- ##
=======================================
Coverage 93.05% 93.05%
=======================================
Files 93 93
Lines 11050 11050
=======================================
Hits 10283 10283
Misses 767 767
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -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): | |||
There was a problem hiding this comment.
Why is this necessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes, right now it's always Uninferable. I'll change to test specifically for that.
There was a problem hiding this comment.
Ah, one test fails on pypy with InferenceError. I'd rather go with the initial pytest.raises clause then
| @@ -178,6 +178,13 @@ def function_to_method(n, klass): | |||
| return n | |||
|
|
|||
|
|
|||
| def attach_to_parent(node: NodeNG, name: str, parent: NodeNG): | |||
There was a problem hiding this comment.
| def attach_to_parent(node: NodeNG, name: str, parent: NodeNG): | |
| 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) |
There was a problem hiding this comment.
This feels like a crucial change that we don't have a test for. Should we add one?
There was a problem hiding this comment.
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.
temyurchenko
force-pushed
the
require-build_class-parent
branch
3 times, most recently
from
4b2d28a
to
4f9e72f
Compare
We also remove `add_local_node` to avoid redundancy. Instead we do the attachment to the parent scope in the constructor of `ClassDef`. We append a node to the body of the frame when it is also the parent. If it's not a parent, then the node should belong to the "body" of the parent if it existed. An example is a definition within an "if", where the parent is the If node, but the frame is the whole module. it's a part of the campaign to get rid of non-module roots
temyurchenko
force-pushed
the
require-build_class-parent
branch
from
4f9e72f
to
80d7ab0
Compare
A part of getting rid of non-Module roots, see #2536