Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEP 649: functools.update_wrapper implementation #124342

Open
JelleZijlstra opened this issue Sep 23, 2024 · 4 comments
Open

PEP 649: functools.update_wrapper implementation #124342

JelleZijlstra opened this issue Sep 23, 2024 · 4 comments
Assignees

Comments

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Sep 23, 2024

The current implementation of PEP-649 and PEP-749 on main simply copies functions' __annotate__ in functools.update_wrapper (and thence functools.wraps):

>>> def f(x: y): pass
... 
>>> from functools import wraps
>>> @wraps(f)
... def g(x): pass
... 
>>> g.__annotate__ is f.__annotate__
True

But as @ncoghlan pointed out (larryhastings/co_annotations#21, https://discuss.python.org/t/pep-749-implementing-pep-649/54974/4), this may not be what we want in the case where the original function's __annotations__ have been modified:

>>> def f(x: int): pass
... 
>>> f.__annotations__["x"] = 42
>>> @wraps(f)
... def g(x): pass
... 
>>> f.__annotations__
{'x': 42}
>>> g.__annotations__
{'x': <class 'int'>}

Alyssa therefore suggests making update_wrapper create a wrapper __annotate__ function that looks in the original function's __annotations__ first. This doesn't fit neatly into update_wrapper's current structure of WRAPPER_ASSIGNMENTS and WRAPPER_UPDATES, so we should add a third bucket, maybe WRAPPER_DELEGATIONS.

Linked PRs

@JelleZijlstra
Copy link
Member Author

The PR failed a test in test_inspect. The test looks like this:

  • The inner function is in C (and therefore has no .__annotations__)
  • The wrapper functions (marked with @functools.wraps(inner)) has a return annotation -> int
  • The test asserts that the signature of the wrapper function includes the return annotation
  • But after the PR, there is now no return annotation on the wrapper.

This is because we now unconditionally set an __annotate__ attribute on the wrapper. The synthesized __annotate__ looks at the wrapped function and finds no annotations.

What should we do here?

  • Accept the change in behavior. I think the new behavior is more consistent: the wrapper's annotations are always overwritten, regardless of whether the wrapped function happens to be a C or a Python function. But it is a backward compatibility break.
  • Set __annotate__ only if the wrapped function has __annotate__. This would reduce the impact of the planned optimization to lazily create __annotate__ functions (PEP 649: Avoid creation of function objects for __annotate__ #124157).
  • Something else?

@JelleZijlstra
Copy link
Member Author

After thinking about this some more and looking at the examples @vstinner posted on the PR (#124346 (comment)), I now prefer to stick with the implementation that is currently on main, which is to copy __annotate__ from the wrapped to the wrapper function if it exists.

  • It's much simpler and doesn't require adding a new category of attributes to update_wrapper. Simple is good; it also means the behavior is easier to understand.
  • It preserves the invariant that calling __annotate__ returns a fresh dictionary every time.
  • It avoids the test failure from my message above.

On the negative side, it does not preserve the previous behavior in cases where the annotations dict is modified before the function is wrapped, like in @ncoghlan's example in larryhastings/co_annotations#21. But code that modifies annotations is always on thin ground; I think in the long term we're better off with a simpler implementation.

@ncoghlan
Copy link
Contributor

ncoghlan commented Sep 25, 2024

I wonder if we could add back the copying of already created annotations, but special-case it to avoid triggering evaluation of not-yet-created lazy annotations. That is:

  • __annotate__ is listed for assignment
  • __annotations__ is listed for updating
  • if __annotations__ is missing from the wrapped object's instance dictionary, it gets removed from the set of attributes to update

The generalised version of the idea would be a module level set specifying "known lazy attributes" that are only assigned/updated if they already exist in the instance dictionary.

I'm less concerned about the "annotations have been modified" case (that was just an easy demonstration) and more about the "annotations have been defined, but an annotate function has not" case.

Edit: fixed to make it clear that the wrapper's annotation dict is still separate from the wrapped object's annotations.

@JelleZijlstra
Copy link
Member Author

For functions, the annotations are not in the instance dictionary, so it can't work exactly as you described, and indeed there isn't a way to ask a function object "do you already have cached __annotations__?". Of course, we could add such a mechanism. However, I'm wary of having the behavior branch on whether __annotations__ already exists. That could lead to confusing behavior where some code is changed to access __annotations__ (without editing them), and now suddenly the annotations on a wrapper function are different.

__annotations__ is listed for updating

I don't think updating (as in dict.update) is the right thing to do for __annotations__. If you .update(), you may get more annotations than there are actual fields or parameters on the objects.

more about the "annotations have been defined, but an annotate function has not" case

This can never happen with builtin objects (classes, functions, modules); they always have both or neither. Of course, user-defined objects could do this.

One approach could be to copy __annotate__ if it exists, and if it does not, try to copy __annotations__ instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants