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

always use the local name when building definitions #2564

Conversation

temyurchenko
Copy link
Contributor

A part of getting rid of non-Module roots, see #2536

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

it's a part of the campaign to get rid of non-module roots

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.10%. Comparing base (eb88dfe) to head (0206036).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2564      +/-   ##
==========================================
- Coverage   93.10%   93.10%   -0.01%     
==========================================
  Files          93       93              
  Lines       11065    11064       -1     
==========================================
- Hits        10302    10301       -1     
  Misses        763      763              
Flag Coverage Δ
linux 92.98% <100.00%> (-0.01%) ⬇️
pypy 93.10% <100.00%> (-0.01%) ⬇️
windows 93.08% <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/raw_building.py 94.90% <100.00%> (-0.02%) ⬇️

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

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.

Now that I'm re-reviewing this. Could you add a link to the commit in the NB comment?

@temyurchenko
Copy link
Contributor Author

Now that I'm re-reviewing this. Could you add a link to the commit in the NB comment?

Hmm, the commit above answers the question «why have local name» with «because sometimes .__name__ is missing». However, my question is «why possibly we might want to use .__name__ over of local name».

@temyurchenko
Copy link
Contributor Author

temyurchenko commented Sep 13, 2024

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.

It doesn't exactly fail, it's just that in pypy they do stuff like list.__mul__ = list.__rmul__. And now even though list.__mul__ is accessed via __mul__ localname, it's actually the __rmul__ object with the corresponding .__name__.

@DanielNoord
Copy link
Collaborator

DanielNoord commented Sep 15, 2024

So I found the original bug report:
https://www.mail-archive.com/[email protected]/msg00009.html

I think the fix as in this PR is still correct: we should always use __name__ as that is the name of the object we are building. localname should only be used a fallback if __name__ isn't set due to a c-extension not behaving like regular Python code.

What do you think @temyurchenko?

Edit: Upon reading the code again I think the change to object_build_methoddescriptor is actually incorrect. We should still prefer __name__ there

@temyurchenko
Copy link
Contributor Author

So I found the original bug report: https://www.mail-archive.com/[email protected]/msg00009.html

I think the fix as in this PR is still correct: we should always use __name__ as that is the name of the object we are building. localname should only be used a fallback if __name__ isn't set due to a c-extension not behaving like regular Python code.

What do you think @temyurchenko?

Edit: Upon reading the code again I think the change to object_build_methoddescriptor is actually incorrect. We should still prefer __name__ there

Looking at the linked report, the issue was the following: symbols from C modules don't have __name__, thus we fail if we try to use __name__.

It's still not clear to me, why we should prefer __name__ over localname. I showed an example above where preferring __name__ over localname leads to a bug and is just incorrect logically. If the class chooses to use a symbol under a different name (i.e. the localname), that's how we should look it up. Otherwise, the symbol isn't going to be found.

Why should we prefer __name__?

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

it's a part of the campaign to get rid of non-module roots
@temyurchenko
Copy link
Contributor Author

Tested this one with pylint tests and the primer, no regressions.

@Pierre-Sassoulas Pierre-Sassoulas added the pylint-tested PRs that don't cause major regressions with pylint label Sep 25, 2024
@temyurchenko
Copy link
Contributor Author

Why should we prefer __name__?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants