control setting nodes as local outside of the constructor #2588

temyurchenko · 2024-09-27T05:31:05Z

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".

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

No pylint regressions.

temyurchenko · 2024-09-27T05:33:41Z

Unfortunately, this PR turned out to be a bit big. A big part of changes is renaming name to alias in InspectBuilder.object_build. I felt that name was quite confusing as to what relation it had to the object being built. In fact, it is just an alias for an object within its parent. The object itself might have a completely different, very own name.

codecov · 2024-09-27T05:37:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.20%. Comparing base (32cb29e) to head (87305dc).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2588   +/-   ##
=======================================
  Coverage   93.20%   93.20%           
=======================================
  Files          93       93           
  Lines       11065    11052   -13     
=======================================
- Hits        10313    10301   -12     
+ Misses        752      751    -1

Flag	Coverage Δ
linux	`93.08% <100.00%> (+<0.01%)`	⬆️
pypy	`93.20% <100.00%> (+<0.01%)`	⬆️
windows	`93.18% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
astroid/nodes/scoped_nodes/scoped_nodes.py	`93.34% <ø> (+0.07%)`	⬆️
astroid/raw_building.py	`94.58% <100.00%> (-0.31%)`	⬇️
astroid/rebuilder.py	`98.21% <100.00%> (+<0.01%)`	⬆️

temyurchenko · 2024-09-27T06:05:40Z

Supersedes #2564, quoting my comment from there for clarity:

I've digged and I found an answer for why we have to prefer name for classes. The classes are cached in builder._done. So, when we build a class once with a localname, all the other builds reuse that class with the same localname. Which is incorrect.

There are ways to deal with that. I feel like the clearest one is to separate attachment to the locals of a parent from the constructor. Since any object can be attached to the parent by a variety of different names that have no relation to the object itself, it would be clearer to not make it the object's responsibility.

So, I am creating a different PR meant to supersede this one. In that PR, I split out adding symbols to the locals of a parent from the constructor to the outside. So, closing this one in favour of #2588.

DanielNoord

Changes make sense to me!

I have asked Jacob for review as he has looked into this pattern before and might know of edge cases that I don't know about.
Awesome work on the investigation!

jacobtylerwalls · 2024-09-30T12:44:21Z

Thanks for this! Does this close #1490? If so, can you add a changelog?

No pylint regressions.

Thank you for checking the pylint test suite. I think these are the kinds of PRs that would also benefit from a pylint primer run. I've toyed with adding a github action to astroid to run the pylint primer action. I need to just finish it up and get it in. Would you mind waiting for me to do that? I promise I won't hold this up for weeks.

temyurchenko · 2024-09-30T17:57:01Z

Thank you for checking the pylint test suite. I think these are the kinds of PRs that would also benefit from a pylint primer

I've ran the primer as well.

I need to just finish it up and get it in. Would you mind waiting for me to do that? I promise I won't hold this up for weeks.

There are a few other PRs dependent on this one (#2562, and, eventually #2536), but if it doesn't take too long, I'm happy to wait.

jacobtylerwalls · 2024-09-30T18:00:22Z

Oh that's great news that you ran the primer. No need to wait then.

temyurchenko · 2024-09-30T18:01:24Z

Oh that's great news that you ran the primer. No need to wait then.

Let me give it just one more try, to be safe

temyurchenko · 2024-09-30T18:07:00Z

Oh that's great news that you ran the primer. No need to wait then.

Let me give it just one more try, to be safe

Yep, all good

temyurchenko · 2024-09-30T18:23:05Z

Thanks for this! Does this close #1490? If so, can you add a changelog?

The wording of #1490 is confusing. It says that the problem is that the parent is added to the locals of the child, but the problem solved by this PR is that the child is added to the locals of the parent. Looking at #1489, indeed it is the latter that is the problem.

I initially intended for #2536 (comment) to close #1490, but this one can probably also be considered to be closing #1490.

What should changelog look like? Can I just add the following to the ./ChangeLog file?

* Control setting local nodes outside of the supposed local's constructor.

  Closes pylint-dev/astroid/issues/1490

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

Pierre-Sassoulas · 2024-09-30T18:39:40Z

Yes, only this syntax is used for pylint issues, for issues from astroid it can be simplified to:

* Control setting local nodes outside of the supposed local's constructor.

  Closes #1490

temyurchenko · 2024-09-30T18:40:27Z

Yes, only this syntax is used for pylint issues, for issues from astroid it can be simplified to:
* Control setting local nodes outside of the supposed local's constructor.

  Closes #1490

Thanks, I'll note this for the future.

Pierre-Sassoulas

I'm drowning in astroid myself, so take this approve for what it's worth but I think this change is pretty good and going into the right direction design wise.

jacobtylerwalls

Wonderful, I can remove the workarounds that stemmed from #1490 in another PR, and many thanks for noticing that the wording was wrong 👍 .

temyurchenko force-pushed the move-set_local-out-of-constructor branch from d7590bd to 17360c4 Compare September 27, 2024 05:57

temyurchenko mentioned this pull request Sep 27, 2024

always use the local name when building definitions #2564

Closed

DanielNoord requested a review from jacobtylerwalls September 30, 2024 07:01

DanielNoord previously approved these changes Sep 30, 2024

View reviewed changes

temyurchenko added 2 commits September 30, 2024 11:27

update changelog

579f7a9

temyurchenko dismissed DanielNoord’s stale review via 579f7a9 September 30, 2024 18:28

temyurchenko force-pushed the move-set_local-out-of-constructor branch from 17360c4 to 579f7a9 Compare September 30, 2024 18:28

Merge branch 'main' into move-set_local-out-of-constructor

87305dc

Pierre-Sassoulas approved these changes Sep 30, 2024

View reviewed changes

jacobtylerwalls approved these changes Sep 30, 2024

View reviewed changes

jacobtylerwalls merged commit 36094ed into pylint-dev:main Sep 30, 2024
20 checks passed

jacobtylerwalls mentioned this pull request Sep 30, 2024

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

Closed

temyurchenko deleted the move-set_local-out-of-constructor branch October 1, 2024 05:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

control setting nodes as local outside of the constructor #2588

control setting nodes as local outside of the constructor #2588

temyurchenko commented Sep 27, 2024

temyurchenko commented Sep 27, 2024

codecov bot commented Sep 27, 2024 •

edited

Loading

temyurchenko commented Sep 27, 2024

DanielNoord left a comment

jacobtylerwalls commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

jacobtylerwalls commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

temyurchenko commented Sep 30, 2024 •

edited

Loading

Pierre-Sassoulas commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

Pierre-Sassoulas left a comment

jacobtylerwalls left a comment

control setting nodes as local outside of the constructor #2588

control setting nodes as local outside of the constructor #2588

Conversation

temyurchenko commented Sep 27, 2024

temyurchenko commented Sep 27, 2024

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

temyurchenko commented Sep 27, 2024

DanielNoord left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

jacobtylerwalls commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

temyurchenko commented Sep 30, 2024 • edited Loading

Pierre-Sassoulas commented Sep 30, 2024

temyurchenko commented Sep 30, 2024

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

jacobtylerwalls left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2024 •

edited

Loading

temyurchenko commented Sep 30, 2024 •

edited

Loading