Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@            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              

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

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

So I found the original bug report:
https://www.mail-archive.com/python-projects@lists.logilab.org/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

So I found the original bug report: https://www.mail-archive.com/python-projects@lists.logilab.org/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__?

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

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.