in reify also copy wrapped function __name__

[Psycojoker]

Hello,

The reify class decorator already copy the __doc__ attribute of the wrapped method but is missing the __name__ attribute. This breaks introspection code that tries to grab a callable name by accessing the __name__ of the callable (like some decorators) for example and according to python functools.update_wrapper (used by @wraps) this is one of the needed magical attribute to copy: https://github.com/python/cpython/blob/145bf269df3530176f6ebeab1324890ef7070bf8/Lib/functools.py#L32-L37

I've only backported __name__ but I can also update to this PR to uses functools.WRAPPER_ASSIGNMENTS to make the code more generic and robust if wanted.

Kind regards and thanks for your work,

[mmerickel]

I'm fine merging this PR. This is coming only shortly after https://github.com/Pylons/pyramid/pull/3657 which stopped using update_wrapper to avoid marking the reified property as a function for introspection. I guess it's worth looking through WRAPPER_ASSIGNMENTS and adding anything else we feel like we've lost to this PR.

[luhn]

This PR passes the isattributedescriptor test in #3657, so it should be okay to merge. (IIRC it's the __wrapped__ that causes the problem.)

The reason I just did __doc__ and nothing else is because that's what functools.cached_property does. I don't know why the Python dev team decided to do it that way.

[luhn]

This breaks introspection code that tries to grab a callable name by accessing the name of the callable (like some decorators)

@Psycojoker, can you share an example of this? Because reify'd properties are specifically not callable, but I'm not quite sure the expected/desired properties of a descriptor.

[Psycojoker]

This breaks introspection code that tries to grab a callable name by accessing the name of the callable (like some decorators)

@Psycojoker, can you share an example of this? Because reify'd properties are specifically not callable, but I'm not quite sure the expected/desired properties of a descriptor.

The incriminated code right now uses the logilab.common.decorators.monkeypatch decorator to add the new method to a class, a method that use reify just after that.

The code is as follow:

@monkeypatch(SomeClass)
@reify
def stuff(self):
    ...

So it's before the instantiation of the class, the code was working before because reify was transmitting the __name__ attribute (monkeypatch reads it). I can understand that this situation is a bit specific (and I'm really not fan of monkeypatching, but you know, existing code base.)

[luhn]

Honestly I think adding __name__ makes sense, but everywhere I look there's no __name__, all done by people smarter than me, so I'm leaning towards leaving it as-is. Core devs, any opinions?

Logilab itself provides a similar decorator that does not have a __name__.

Also Pyramid's built-in @property doesn't have a name.

>>> class A:
...     @property
...     def foo(self):
...             return 'bar'
...
>>> A.foo.__name__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'property' object has no attribute '__name__'

[ztane]

The proper thing to test is functools.cached_property - which does not have a __name__ either. Does functools.cached_property work with monkeypatch?

[ztane]

If you're using Python 3.8+ and functools.cached_property works, then the problem is with reify. In that case use functools.cached_property instead of reify. In other cases maybe it is the monkeypatch not accepting name that is the problem.

BTW, reify should be marked deprecated, functools.cached_property is a bit more complex but it is essentially the same less the locking to ensure that if instance is shared between threads that two threads do not race on the computation. It also has with __set_name__ so you do not need to have the method by exact same name:

class cached_property:
    def __init__(self, func):
        self.func = func
        self.attrname = None
        self.__doc__ = func.__doc__
        self.lock = RLock()

    def __set_name__(self, owner, name):
        if self.attrname is None:
            self.attrname = name
        elif name != self.attrname:
            raise TypeError(
                "Cannot assign the same cached_property to two different names "
                f"({self.attrname!r} and {name!r})."
            )

    def __get__(self, instance, owner=None):
        if instance is None:
            return self
        if self.attrname is None:
            raise TypeError(
                "Cannot use cached_property instance without calling __set_name__ on it.")
        try:
            cache = instance.__dict__
        except AttributeError:  # not all objects have __dict__ (e.g. class defines slots)
            msg = (
                f"No '__dict__' attribute on {type(instance).__name__!r} "
                f"instance to cache {self.attrname!r} property."
            )
            raise TypeError(msg) from None
        val = cache.get(self.attrname, _NOT_FOUND)
        if val is _NOT_FOUND:
            with self.lock:
                # check if another thread filled cache while we awaited lock
                val = cache.get(self.attrname, _NOT_FOUND)
                if val is _NOT_FOUND:
                    val = self.func(instance)
                    try:
                        cache[self.attrname] = val
                    except TypeError:
                        msg = (
                            f"The '__dict__' attribute on {type(instance).__name__!r} instance "
                            f"does not support item assignment for caching {self.attrname!r} property."
                        )
                        raise TypeError(msg) from None
        return val

That code itself requires Python 3.6.

[ztane]

Hmm actually coming to think of it that lock is actually a problem...

[ztane]

.. writing a bug in bugs.python.org. Strange that no one noticed this in 4 years.

[ztane]

https://bugs.python.org/issue43468

[luhn]

Hey @mmerickel, I’m not sure this should have been merged. The behavior doesn’t match with property and functools.cached_property, which reify should try to emulate.

[mmerickel]

Ok - I don't mind reverting it - it seemed harmless to me but I don't feel very strongly about it.