GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.04k stars 230 forks source link

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

Open cableray opened 3 years ago

cableray commented 3 years ago

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?

GrahamDumpleton commented 3 years ago

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:

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

cableray commented 3 years ago

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)
GrahamDumpleton commented 3 years ago

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.

cableray commented 3 years ago

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.

GrahamDumpleton commented 3 years ago

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