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

Function Docstrings Obscured By Decorators That Return Protocols #5840

Open
rmorshea opened this issue May 2, 2024 · 5 comments
Open

Function Docstrings Obscured By Decorators That Return Protocols #5840

rmorshea opened this issue May 2, 2024 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@rmorshea
Copy link

rmorshea commented May 2, 2024

Current Behavior

When implementing decorators it's often convenient to use a Protocol and ParamSpec to annotate the type they return. However, using a Protocol causes Pylance to hide the docstring of the decorated function even though, in most cases where a Protocol is generalized by a ParamSpec, it would make sense to display the docstring of the function the ParamSpec came from.

Consider the example below:

from typing import Callable, Concatenate, ParamSpec, Protocol, TypeVar

P = ParamSpec('P')
R = TypeVar('R')


class Options:
    """Options for a function"""


class RequiresOptions(Protocol[P, R]):
    """A protocol for functions that require options to be passed in."""

    def __call__(self, options: Options, *args: P.args, **kwargs: P.kwargs) -> R: ...


# annotated with a protocol
def use_options(func: Callable[P, R]) -> RequiresOptions[P, R]: ...


@use_options
def foo(x: int) -> int:
    """Some docstring"""
    ...


foo(Options(), 1)  # docstring hidden

image

Here, Pylance just shows the type of foo. One could argue that this is sensible given that the decorator changed the type of foo. That might imply that the foo's docstring wouldn't necessarily be meaningful anymore. However, if that's true, Pylance isn't consistent in its behavior since, if we change this decorator to use Concatenate instead of a Protocol, the docstring is preserved:

# annotated with concatonate
def use_options(func: Callable[P, R]) -> Callable[Concatenate[Options, P], R]: ...


@use_options
def bar(x: int) -> int:
    """Some docstring"""


bar(Options(), 1)  # docstring visible

image

Expected Behavior

When Pylance encounters decorators of the form:

P = ParamSpec('P')

class SomeProtocol(Protocol[P]):
    def __call__(self, *args: P.args, **kwargs: P.kwargs) -> Any: ...

def some_decorator(func: Callable[P, Any]) -> SomeProtocol[P]: ...

Pylance should preserve the docstrings of the functions it decorates. Further, the details of __call__ should not matter beyond the fact that it uses the ParamSpec. So for example, any of the following should still preserve docstrings:

class SomeProtocol(Protocol[P]):
    def __call__(self, added_arg: int, *args: P.args, **kwargs: P.kwargs) -> Any: ...

class SomeProtocol(Protocol[P]):
    @overload
    def __call__(self, *args: P.args, **kwargs: P.kwargs) -> Any: ...
    @overload
    def __call__(self, added_arg: int, *args: P.args, **kwargs: P.kwargs) -> Any: ...
    def __call__(self, *args: Any, **kwargs: Any) -> Any: ...

Given the same example above Jedi chooses to display the docstring of the decorated function:

image

Open Questions

It's a little unclear whether any Protocol instances that are generalized by a function's ParamSpec should adopt the docstring of that function. Perhaps only a Protocol that implements a __call__ that uses the ParamSpec should have this behavior.

For example, it might not make sense to copy the docstring in a case like:

class HasSomeMethod(Protocol[P]):
    def some_method(self, *args: P.args, **kwargs: P.kwargs) -> Any: ...

def has_some_method(func: Callable[P, Any]) -> HasSomeMethod[P]: ...

It's not particularly clear what the best thing to do in this situation is.

@github-actions github-actions bot added the needs repro Issue has not been reproduced yet label May 2, 2024
@erictraut
Copy link
Contributor

I don't think there's a "correct" answer here. I can point to cases where either choice is correct. The current behavior is at least internally consistent. If a callable object has a __call__ method with a docstring, that docstring takes precedence over any docstring that is captured by a ParamSpec used within that __call__ method's signature.

You can omit the docstring for the __call__ method if you don't want it to take precedence. Pyright/pylance will then use the docstring captured by the ParamSpec.

@rmorshea
Copy link
Author

rmorshea commented May 2, 2024

For reference, I've updated the description to note that Jedi appears to implement the behavior I'd like given the example I provided.

You can omit the docstring for the __call__ method if you don't want it to take precedence.

Perhaps I'm not on the latest version, but in my example, the docstring is not displayed even though RequiresOptions.__call__ lacks a docstring. Removing the docstring from RequiresOptions itself doesn't change that either.

I'd definitely be satisfied with this behavior though. For whatever reason I'm just not seeing it in my example.

@erictraut
Copy link
Contributor

erictraut commented May 2, 2024

I see what you mean. The docstring does appear in the signature help (when typing the arguments for the foo call), but it doesn't appear when hovering over foo. That's a current limitation of the hover provider — the language server module that provides the hover text. I agree that it would make sense for the hover provider to include the "adopted" decorator in this case in the same way that the signature help provider does.

Here's the signature help for foo. You can see that the "adopted" docstring does appear here.
image

In summary, I think the pyright type analyzer is doing the right thing here and preserving the docstring, but the hover provider (which is owned by the pylance team) is not using this information when the identifier is an instance of a callable object (i.e. an instance of a class that has a __call__ method). So I think this is a feature request for the Pylance team to add such support.

@rmorshea
Copy link
Author

rmorshea commented May 2, 2024

Great. Thanks for digging into the issue. I Look forward to having it fixed!

@heejaechang heejaechang added bug Something isn't working and removed needs repro Issue has not been reproduced yet labels May 2, 2024
@heejaechang
Copy link
Contributor

when we rework how we do doc string this sprint, we should make sure all features that use doc string use same code to make sure we have consistent behavior regardless where it is shown to users.

currently, we have slightly different code depends on each features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants