wrapt icon indicating copy to clipboard operation
wrapt copied to clipboard

When combined with @property methods, `instance` argument does not seem to match the docs

Open cableray opened this issue 4 years ago • 5 comments

My understanding from the documentation is that for decorated instance methods, you never should have to extract the self arg, that it is always in instance, not args, and that wrapped is already bound to instance. However, this does not seem true when combined with the property decorator:

from pytest import fixture
from wrapt import decorator

@decorator
def wrapper(wrapped, instance, args, kwargs):
    return {'instance':instance, 'args':args, 'kwargs': kwargs, 'wrapped':wrapped(*args, **kwargs)}

class Decorated:
    @property
    @wrapper
    def prop(self, *args, **kwargs):
        return {'self': self, 'args': args, 'kwargs':kwargs}

@fixture
def instance():  return Decorated()

@fixture
def result(instance):  return instance.prop

def test_instance_is_sent_to_wrapped(instance, result): # passes
    assert result['wrapped']['self'] is instance

def test_instance_arg_is_correct(instance, result): # fails
    assert result['instance'] is instance

def test_instance_is_not_i_args(instance, result): # fails
    assert instance not in result['args']

(running on python 3.7.8, wrapt 1.12.1, pytest 5.3.2)

This doesn't seem intentional, and I suspect is @property pulling some shenanigans. If these test don't use @property, and are modified to use a method, they pass. Also, if you grab the property descriptor, calling descriptor.fget requires an instance arg, it isn't bound.

Can this issue be fixed?

cableray avatar Jul 19 '21 21:07 cableray

I'll try and look later, but property does a lot of weird magic and has conventions that aren't going to work with a standard decorator. You would need to create a very special object wrapper just for property but it could only be applied around it, not under it. Alternatively, you can try and write it long hand, but still not sure how that will work as it is possible that property has same failing classmethod used to, which is that it doesn't honour the descriptor protocol for instance methods it is applied to and uses a short cut.

Anyway, play with the following:

class C:
    def __init__(self):
        self._x = None

    @decorator
    def getx(self):
        return self._x

    @decorator
    def setx(self, value):
        self._x = value

    @decorator
    def delx(self):
        del self._x

    x = property(getx, setx, delx, "I'm the 'x' property.")

If in all cases self is not passed as instance, then property has a similar problem to:

  • https://bugs.python.org/issue19072

That only took about 8 years to be fixed and even then they recently think the fix isn't entirely correct.

GrahamDumpleton avatar Jul 19 '21 22:07 GrahamDumpleton

Would it be possible to normalize this some way, like providing an option to decorator that forces self into args instead of binding? Or is it sufficient to do something like:

if instance is None:
  instance = args[0]
  normalized_args = args[1:]
…
return wrapped(*args, **kwargs)

cableray avatar Jul 21 '21 17:07 cableray

It would be easier for you do this yourself for a specific case where it is required. I don't see that having this in wrapt itself would be a good idea.

You haven't really explained the overall goal of what you are trying to achieve and are focusing instead on what you think is the solution.

As I already said you may instead want to create a very special object wrapper just for property which takes into consideration properly the differences between getter, setter and deleter.

So if you can explain properly what it is you are trying to do and why then can perhaps suggest better way of handling it.

GrahamDumpleton avatar Jul 22 '21 00:07 GrahamDumpleton

Here's an example of my use case:

def with_data_item(data_key):
  @decorator
  def wrapper(wrapped, instance, args, kwargs):
    def adapter(*_args, **_kwargs):  # is this adapter needed? I am removing an arg from the decorated signature 
      data_value = instance.get(data_key)
      return wrapped(data_value, *_args, **_kwargs)
    return adapter(*args, **kwargs)
  return wrapper


class DataAdapter:
  def __init__(self, data_source):
    self.data_source = data_source

  def get(self, data_key):
    return self.data_item[data_key]

  @property
  @with_data_item('foo')
  def foo(self, data_item):
    return datai_tem+1

  @with_data_item('bar')
  def calculate_bar(self, data_item):
    do_something_slow_with(data_item)

It's similar to functools.partial_method, but I need more flexibility.

cableray avatar Jul 22 '21 16:07 cableray

Old issue related to this with more thorough description of how to create a wrapper object for properties can be found at:

  • https://github.com/GrahamDumpleton/wrapt/issues/44

GrahamDumpleton avatar Aug 05 '21 00:08 GrahamDumpleton