GrahamDumpleton / wrapt

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

Reliably Positional-Only `self` in `__call__` #214

Closed mentalisttraceur closed 1 year ago

mentalisttraceur commented 2 years ago

The C version of CallableObjectProxy.__call__ already has self be a positional-only argument, but the Python version doesn't.

Here's a good way to fix that which works on all old Python versions that wrapt still supports:

 class CallableObjectProxy(ObjectProxy):
-    def __call__(self, *args, **kwargs):
+    def __call__(*args, **kwargs):
+        def __call__(self, *args):
+            return self, args
+        self, args = __call__(*args)
         return self.__wrapped__(*args, **kwargs)

(Using a nested function with the same name is a good way to get a native exception with the same message that you normally would if you go out of your way to call CallableObjectProxy.__call__ directly.)

GrahamDumpleton commented 2 years ago

Can you please provide a working code example which exercises the problem you are experiencing?

On first look the change you are suggesting looks very wrong, would break the normal Python object model calling conventions, and that the problem is more to do with how you are trying to use it. I can't investigate further unless you can provide a test case showing your problem.

mentalisttraceur commented 2 years ago

Umm... wow.

>>> import wrapt, importlib, os
>>> os.environ['WRAPT_DISABLE_EXTENSIONS'] = 'x'
>>> _ = importlib.reload(wrapt.wrappers)
>>> wrapt.wrappers.CallableObjectProxy is wrapt._wrappers.CallableObjectProxy
False
>>> dict(self='works')
{'self': 'works'}
>>> wrapt._wrappers.CallableObjectProxy(dict)(self='works')
{'self': 'works'}
>>> wrapt.wrappers.CallableObjectProxy(dict)(self='breaks')
Traceback (most recent call last):
  ...
TypeError: CallableObjectProxy.__call__() got multiple values for argument 'self'
>>> def __call__(*args, **kwargs):
...     def __call__(self, *args):
...         return self, args
...     self, args = __call__(*args)
...     return self.__wrapped__(*args, **kwargs)
...
>>> wrapt.wrappers.CallableObjectProxy.__call__ = __call__
>>> wrapt.wrappers.CallableObjectProxy(dict)(self='fixed')
{'self': 'fixed'}
GrahamDumpleton commented 2 years ago

You shouldn't ever be trying to supply self as a named keyword argument. Doing so would see strange behaviour in pure Python applications, so not surprised.

What is the real world use case for this?

It seems to me that if code is trying to supply self as a named keyword argument it is doing something wrong whereby it most likely should be using the descriptor protocol (__get__() method) to properly do method binding.

mentalisttraceur commented 2 years ago

...you do realize that most people use a very similar reasoning shape to dismiss the value of Correctly(tm) implementing decorators, the descriptor protocol, and so on, right?

Do you not see the parallels between this and BPO-19072? Quick, what's a compelling real-world use-case where it's really necessary for classmethod to be fully transparent to the descriptor protocol? I've yet to see a business need in my career where I couldn't work around that deficit. But somehow that didn't stop me from immediately realizing that it is classmethod's failure to be fully transparent that needed justification, not the other way around.

Do you not see how "passes through any argument" has the same exact abstracted value as "fully implements the descriptor protocol" and "is a transparent object proxy"? Universal correctness and regularity.

Inability to see a real-world need is never in itself a good reason for an interface irregularity which contributes nothing but surprisingly or frustratingly breaking in some edge case.

What's the use-case for passing self by name? If PEP-570 happened back in 2.6 instead of 3.8, or had always been part of the language, would you be arguing that it is better for self to not be a positional-only argument? Apparently not, since you didn't go out of your way to let self be a keyword argument in the C implementation. It's not like pure Python instance methods being unable to handle a self keyword is a feature of the language - even the name self is just convention, it could be any name that breaks in kwargs - this is all just an artifact of what was simple and good enough for most cases.

And what's the use-case for the C and Python implementations being inconsistent? For any code to exist that works fine, until one day the C extension isn't available on some user's machine and boom, exception the developer has never seen before. And not even an exception due to anything they're doing, but an exception because someone else's code passed a "self" kwarg through their wrapt-wrapped callable.

Anyway, the real-world use-case I can currently remember is decorating functions and function-like class instances like partial and compose (whether mine, or the one from toolz, or some other one). I don't see a good reason for my APIs to have a specific corner-case breakage just because somewhere in a chain of calls passing through my stuff, someone found a real-world use-case that I didn't conceive of but presumed to justifiably break. Specifically to wrapt, I've seen two independent feature requests for compose-providing libraries to also provide a way to compose using an operator overload, and near as I can tell, the best way to do that involves a callable wrapt object proxy subclass which implements the relevant operator overloads. But the whole point is that there's probably other use-cases I haven't even conceived of yet.

Again, would it be a good idea for me to write opaque decorators just because I can't conceive of how anyone composing my decorator with other code might ever benefit from full "proper" transparency?

Look... are you sure you want to defend a pothole and a lack of universal passthrough transparency in a composable interface? Or is your problem really elsewhere?

Because if you want to argue that what I proposed is a cryptic way of doing it, raises too many mental alarms that you don't think developers should desensitize, or is hard for most developers to understand and verify, etc etc, then I'd say that's a fair point. You could totally reasonably argue that the readability and clarity cost is not worth the benefit. There are much more readable and clear ways of achieving almost the same thing - for example, in my projects, before I switched to this trick of approximating the native error for a missing required positional argument, I used to just check len(args) and manually raise TypeError instead, which is much more readable. You could also just one-liner it with self, args = args[0], args[1:] if you don't mind getting an IndexError instead of a TypeError in the extremely unlikely and inherently incorrect edge case where someone calls .__call__ directly off the class with no arguments for some reason - which I think is reasonably justified for a polyfill backport like this. Even if you just said

"if we ever drop support for Python <3.8 or ship multiple variants for different Python versions, then we'll do __call__(self, /, *args, **kwargs) in the new stuff, but I don't want to pervert code with manual backports of positional-only arguments for older versions"

then I'd give you a 👍 and move on still feeling good about how you're steering wrapt.

GrahamDumpleton commented 2 years ago

It will take me a while to read and digest that wall of text. Am not in the right frame of mind right now so haven't even tried to read it through.

One point I will make is that there are various cases where things implemented as pure Python don't work the same as when done using C APIs. That is just how CPython is. I am not going out of my way to make them different, it just happens when you do things in the normally accepted ways using the respective APIs, they will not behave the same or it can't be done in the same way, which in the case of trying to support both is always a problem as far as uniformity. I have tried very hard to make them work the same, but there are still some cases where it isn't possible because the C API lacks features, or has strange limitations.

Anyway, I have perhaps not been understanding what the problem is you are trying to solve here, which is why was asking for the actual use cases. On first quick glance it seemed to be you were trying to side step binding semantics which is always a red flag for something being done wrong.

So I will look at this some more, I just will not get to it immediately.

mentalisttraceur commented 2 years ago

By the way, to me your first and second reply felt dismissively and condescendingly presumptious - as if I couldn't possibly be any righter in bringing this up than a confused doing-things-wrong developer.

I don't handle that well, so I think my replies afterwards were probably harsher, more negative in tone, and less constructive than they probably needed to be. I'm sorry for that.

I want you to know that for years I've thought highly of you, and I still do, both for making the wrapt library to do transparent decorators and object proxies thoroughly correctly, and for the explanations you've written about why it's valuable for people to make their decorators that way.

mentalisttraceur commented 2 years ago

In retrospect, your reaction makes sense. I probably should've been more sympathetic to it. After all, this is very relatable to me:

On first quick glance it seemed to be you were trying to side step binding semantics which is always a red flag for something being done wrong.

You've probably seen more than your fair share of developers trying to do something wrong, and have felt more than your fair share of the pain and problems that causes.

I also didn't need to interpret your initial replies so negatively or take them so personally - the way I parsed them is at least partially a problem on my end.

mentalisttraceur commented 2 years ago

Actually I want to take emphasis off CallableObjectProxy.__call__ and refocus onto stuff like FunctionWrapper.__call__.

CallableObjectProxy.__call__ is so simple that I can trivially make sure my code polyfills the desired behavior on top of the pure-Python version:

from wrapt import CallableObjectProxy as _CallableObjectProxy

...

try:
    _CallableObjectProxy(lambda self: None)(self=None)
except TypeError:
    class _CallableObjectProxy(_CallableObjectProxy):
        def __call__(self, /, *args, **kwargs):
            return self.__wrapped__(*args, **kwargs)

But FunctionWrapper.__call__ has much more logic, and seems like the kind of problem where consumer code can't self-provide a fix without duplicating all of __call__.

GrahamDumpleton commented 1 year ago

Closing this as believe it should have all been dealt with in version 1.15.0 of wrapt.

mentalisttraceur commented 1 year ago

Thank you for eventually reading and considering my points, and deciding to implement it! I really appreciate that.

mentalisttraceur commented 1 year ago

Also! Thank you in general for wrapt and all the work that you've put into it.

It's a really great library, probably in my top five Python libraries, maybe even top three (I can only think of two others off the top of my head which I respect as much for their thorough design, attention to correctness, and replacing tedious error-prone boilerplate with a well-paved clear and concise path for stuff we should ideally be doing almost always - Trio and attrs.) After discovering wrapt I've almost never felt satisfied with any other decorator implementation. I keep wanting libraries to use wrapt for all their decorators and the only thing holding me back is that I know lots of people don't value the benefits enough to add a dependency for it.

I'm sorry once again for any negative experiences I caused for you by how I came at you in this issue.