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

autodoc shows undecorated signature for decorated functions #7650

Open
tk0miya opened this issue May 10, 2020 · 2 comments
Open

autodoc shows undecorated signature for decorated functions #7650

tk0miya opened this issue May 10, 2020 · 2 comments

Comments

@tk0miya
Copy link
Member

tk0miya commented May 10, 2020

Describe the bug
autodoc shows undecorated signature for decorated functions

To Reproduce

def wrapper(fn):
    @functools.wraps(fn)
    def func():
        return fn(1, 2, 3)

    return func


@wrapper
def foo(x, y, z):
    return x + y + z
.. automodule:: example
   :members:
   :undoc-members:

This is converted to foo(x, y, z).

Expected behavior
The function foo should be shown as foo(). That is what users see.

Your project
No

Screenshots
No

Environment info

  • OS: Mac
  • Python version: 3.8.2
  • Sphinx version: 3.1.0dev
  • Sphinx extensions: sphinx.ext.autodoc
  • Extra tools: No

Additional context
No

@tk0miya tk0miya added this to the 3.1.0 milestone May 10, 2020
tk0miya added a commit to tk0miya/sphinx that referenced this issue May 10, 2020
tk0miya added a commit to tk0miya/sphinx that referenced this issue May 10, 2020
tk0miya added a commit to tk0miya/sphinx that referenced this issue May 10, 2020
tk0miya added a commit to tk0miya/sphinx that referenced this issue May 10, 2020
tk0miya added a commit that referenced this issue May 16, 2020
Fix #7650: autodoc: undecorated signature is shown for decorated functions
@ppwwyyxx
Copy link
Contributor

ppwwyyxx commented Jul 4, 2020

I would argue that this is actually not a bug, and the old behavior is usually better than the new one, especially when functools.wraps is used.

In fact, functools.wraps literally copies attributes such as __doc__ to the new function, which means the decorated function is meant to be called with the same arguments as the original function. However, a decorator is usually implemented with *args, **kwargs, like this:

def wrapper(fn):
    @functools.wraps(fn)
    def func(*args, **kwargs):
        do something with arguments
        call fn with arguments
        do something with results
        return results
    return func

@wrapper
def foo(x, y, z):
    return x + y + z

To see that this is the common pattern, one can search for functools.wraps in any large python code base to verify, e.g.: https://github.com/tensorflow/tensorflow/search?q=%40functools.wraps&unscoped_q=%40functools.wraps https://github.com/django/django/search?q=functools.wraps&unscoped_q=functools.wraps

With the current "bugfix", all decorated function will be rendered with *args, **kwargs, which is not useful and worse than the old behavior.

I think the solution can be the following:

  1. no choice is perfect - it eventually depends on what user does. So some way to let user config the option might be useful, for example, let users to set func.__sphinx_use_new_signature__ = True.
  2. If (1) is too hard, I think it's a better decision to keep the old behavior when using functools.wraps, because this follows the semantics of functools.wraps and the word wrapping. This can be done by checking hasattr(func, __wrapped__).
  3. For other decorators, I'm not sure which choice is better. But in general I guess it's less likely that the decorator has useful signatures than the decorated function, so I still prefer the original behavior.

This problem has already puzzled other projects, such as optuna/optuna#1353, which actually called this bugfix as a bug.

@tk0miya tk0miya reopened this Jul 4, 2020
@tk0miya
Copy link
Member Author

tk0miya commented Jul 4, 2020

Thank you for comment. I understand this change causes trouble. I agree to revert it ASAP.

If (1) is too hard, I think it's a better decision to keep the old behavior when using functools.wraps, because this follows the semantics of functools.wraps and the word wrapping. This can be done by checking hasattr(func, wrapped).

I don't think functools.wraps() is not related with this issue. It does not related to its signature. It only copies attributes like __name__, __doc__, etc. to decorated object. In addition, __wrapped__ is commonly used attribute, not only for functools.wraps(). So it is hard to check the object is decorated via functools.wraps().

For other decorators, I'm not sure which choice is better. But in general I guess it's less likely that the decorator has useful signatures than the decorated function, so I still prefer the original behavior.

I don't think the decorators not modifying signature are "in general". But I agree they are major. So it would be better to behave like before by default. And it's perfect if we can change the behavior optionally.

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