enforce a non-None parent in build_function #2562
Conversation
temyurchenko
force-pushed
the
require-build_function-parent
branch
from
c65d836
to
f3242e8
Compare
|
Review #2564 first, please |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2562 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 93 93
Lines 11067 11054 -13
=======================================
- Hits 10319 10307 -12
+ Misses 748 747 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
This needs to be rebased and given a new name to the PR, but the localname change LGTM.
For posterity the localname stuff was introduced in 4cdfc82
If I understand it correctly, when doing the initial object_build we try to access getattr(obj, name).__name__ However, this can fail so the code was changed to also have a reference to name itself as a fallback.
The changes in this PR still allow this pattern so they look good. Please rebase so I can merge it.
|
Never mind my previous comment. The second commit in this PR was merged in #2557. The first commit was correctly split out to #2564 which I'll approve and merge shortly with the same comment. @temyurchenko Is a commit missing from this PR? Or should it be closed/ |
Hmm, no, that commit was about |
It doesn't exactly fail, it's just that in pypy they do stuff like |
temyurchenko
force-pushed
the
require-build_function-parent
branch
3 times, most recently
from
b9d298b
to
8c85892
Compare
1. The main reason is that a node might be assigned to its parent via an «alias»: Sometimes a class accesses a member by a different name than "__name__" of that member: in pypy3 `list.__mul__.__name__ == "__rmul__"`. As a result, in the example above we weren't able to find "list.__mul__", because it was recorded only as "list.__rmul__". 2. Sometimes we want to have a parent semantically, but we don't want to add it to the list of locals. For example, when inferring properties we are creating ad-hoc properties. We wouldn't want to add another symbol to the locals every time we do an inference. (actually, there's a very good question as to why we are doing those ad-hoc properties but that's out of scope) it's a part of the campaign to get rid of non-module roots
it's a part of the campaign to get rid of non-module roots
temyurchenko
force-pushed
the
require-build_function-parent
branch
from
8c85892
to
d85731d
Compare
A part of getting rid of non-Module roots, see #2536
We also remove
add_local_nodeto avoid redundancy. Instead we do theattachment to the parent scope in the constructor of
FunctionDef.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
Type of Changes
Description
Refs #XXXX
Closes #XXXX