Fixed #33098 -- Optimized keep_lazy's internal wrapper for single argument functions.

[kezabelle]

May as well find out if it passes CI, and if the ticket (33098) isn't accepted I'll just close it down.

The check within keep_lazy_single_argument_wrapper is slightly more involved than one might think, to support the use-case of functions being called using keyword arguments only, even when there's only 1 argument.

eg: given escape('1') and escape(text='1') we want both to work and raise a TypeError correctly if the kwarg is given as test instead of text

We can't know the name of the kwarg at wrapper definition time (I don't think...) without exec() to generate the function, so it consumes *args and **kwargs as the multiple argument wrapper does, expecting there to be only one value to look at.

[felixxm]

@kezabelle Do you have time to keep working on this?

[kezabelle]

Yeah, can do. Though not without some input I suspect:

• Firstly, I think the implementation is (IIRC) roughly correct/working, so is probably representative of the additional complexity being introduced. Is the impact (600ns?) worth it, to yours & Carlton's eyes? I don't want to drop a bunch more complexity in for you if neither of you think it's worth it.
• Secondly, I couldn't think of a good way to test which wrapper function was used (single or multiple) to demonstrate covering all the scenarios in a correct manner. If anyone has input on how I might do so; I'd be all ears.

[kezabelle]

Circling back round to this:

@ngnpope I've incorporated many/most of your proposed changes locally, I agree that they're slightly less convoluted. The only real change I had to make was this line:

arg = args[0] if args else kwargs[first_parameter.name]
if isinstance(arg, Promise):

which doesn't work out based on the tests I smashed in - giving func(x=1) where func() actually takes the kwarg y would raise a KeyError instead of the expected TypeError. I managed to make it work by adjusting to:

gathered_args = [*args, *kwargs.values()]
if gathered_args:  # Given we're in the "one value" branch, this should only ever be 0 or 1 length, I think...
    if isinstance(gathered_args[0], Promise):

which is slightly slower but correctly handles both the invalid kwarg and the zero-arguments given exceptions correctly. So where your version was giving me roughly 1.1-1.2us (of course yours pulled out better numbers 🙄 😄 ) I'm now seeing 1.3-1.4us but the pass-through works.

---

What I'm still having trouble figuring out how to test is:

• Which wrapper it went through. I can pdb manually and check the right one is being dispatched to, and I can give the wrappers different names (rather than wrapper) and maybe test the repr (bleh) but .... struggling. Given the whole thing is about dispatching to different inner functions, failing to test that is a big sticking point.
• Whether the lazy_func was even used. Again I can pdb for my own sanity, but that doesn't really fly...Is it enough to check the return value isinstance(Promise)? I'm not sure. I can do that, but is it enough?

If anyone wants to weigh in on these 2 points specifically, and un-cloud my fogginess around how to fully demonstrate & exercise what's occurring, I'd be grateful.

[collinanderson]

Copying my comment here from https://github.com/django/django/pull/15456#discussion_r812393287:

Maybe escape() gets called so much that it might be worth just hand-coding the keep_lazy code. There's only one arg to check.

Maybe the function could check its own args for Promise and call lazy(itself) if Promise found. That way functions could just be hand-optimized and they wouldn't need to be wrapped at all if no promises are passed in, which means fewer function calls.

Example for django.utils.html.escape(), instead of @keep_lazy(SafeString) wrapper, just do this:

def escape(text):
    if isinstance(text, Promise):
        return _lazy_escape(text)

    return SafeString(html.escape(str(text)))

_lazy_escape = lazy(escape, SafeString)

It means if a lazy value is passed in, there's an extra Promise check, but at least it's faster for the non-promise case, which I assume is more common.

[felixxm]

Closing due to inactivity.