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

reportGeneralTypeIssues when using pep612 ParamSpec #2758

Closed
CaselIT opened this issue Dec 23, 2021 · 14 comments
Closed

reportGeneralTypeIssues when using pep612 ParamSpec #2758

CaselIT opened this issue Dec 23, 2021 · 14 comments
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working

Comments

@CaselIT
Copy link
Contributor

CaselIT commented Dec 23, 2021

Note: if you are reporting a wrong signature of a function or a class in the standard library, then the typeshed tracker is better suited for this report: https://github.com/python/typeshed/issues.

Describe the bug
A clear and concise description of what the bug is.

I'm trying to start typing SQLAlchemy for v2, and I'm currently facing an issue typing the generative decorator it uses to make a method of a class generative.
The current wip is at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/3423.

Below is an test script that reproduces the error outside sqlalchemy

To Reproduce
Steps to reproduce the behavior.

import typing

_Fn = typing.TypeVar("_Fn", bound=typing.Callable)
_Ret = typing.TypeVar("_Ret")
_Args = typing.ParamSpec("_Args")
_Self = typing.TypeVar("_Self", bound="_GenerativeType")

def decorator(
    target: typing.Callable[typing.Concatenate[_Fn, _Args], _Ret]
) -> typing.Callable[[_Fn], typing.Callable[_Args, _Ret]]:
    def decorate(fn: _Fn):
        def wrapper(*a: _Args.args, **k: _Args.kwargs) -> _Ret:
            return target(fn, *a, **k)

        return wrapper

    return decorate

class _GenerativeType(typing.Protocol):
    def _generate(self: "_Self") -> "_Self":
        ...

def generative(
    fn: typing.Callable[typing.Concatenate[_Self, _Args], None]
) -> typing.Callable[typing.Concatenate[_Self, _Args], _Self]:
    @decorator
    def _generative(
        fn: typing.Callable[typing.Concatenate[_Self, _Args], None],
        self: _Self,
        *args: _Args.args,
        **kw: _Args.kwargs
    ) -> _Self:
        """Mark a method as generative."""

        self = self._generate()
        x = fn(self, *args, **kw)
        assert x is None, "generative methods must have no return value"
        return self

    decorated = _generative(fn)
    # decorated.non_generative = fn
    return decorated

running pyright on this snipped I get the following error:

No configuration file found.
No pyproject.toml file found.
stubPath path\to\foo\typings is not a valid directory.
Assuming Python platform Windows
Searching for source files
Found 1 source file
path\to\foo\test.py
  path\to\foo\test.py:45:12 - error: Expression of type "(self: _Self@generative, **_Args@generative) -> _Ret@decorator" cannot be assigned to return type "(_Self@generative, **_Args@generative) -> _Self@generative"
    Type "(self: _Self@generative, **_Args@generative) -> _Ret@decorator" cannot be assigned to type "(_Self@generative, **_Args@generative) -> _Self@generative"
      Function return type "_Ret@decorator" is incompatible with type "_Self@generative"
        Type "_Ret@decorator" cannot be assigned to type "_GenerativeType" (reportGeneralTypeIssues)
1 error, 0 warnings, 0 infos
Completed in 0.725sec

Trial and error seems to point to the ParamSpec in the inner _generative. If I replace it with this version not error is found:

def generative(
    fn: typing.Callable[typing.Concatenate[_Self, _Args], None]
) -> typing.Callable[typing.Concatenate[_Self, _Args], _Self]:
    @decorator
    def _generative(
        fn: typing.Callable[typing.Concatenate[_Self, _Args], None],
        self: _Self,
    ) -> _Self:
        """Mark a method as generative."""

        self = self._generate()
        x = fn(self)
        assert x is None, "generative methods must have no return value"
        return self

    decorated = _generative(fn)
    # decorated.non_generative = fn
    return decorated

Expected behavior
A clear and concise description of what you expected to happen.

I believe the original type is correct, so pyright should type check correctly

Screenshots or Code
see above

VS Code extension or command-line
Are you running pyright as a VS Code extension or a command-line tool? Which version? You can find the version of the VS Code extension by clicking on the Pyright icon in the extensions panel.

command line;

> pyright --version
pyright 1.1.199

Additional context
Add any other context about the problem here.

cc @zzzeek @erictraut microsoft/pylance-release#840 (comment)

@erictraut
Copy link
Collaborator

Thanks for the bug report. I'm able to repro the problem, and I agree it looks like a bug. I'll investigate further.

@erictraut erictraut added the bug Something isn't working label Dec 23, 2021
@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 23, 2021

thanks for looking into it.

Note that this is not a priority for SQLAlchemy at the moment, since it seems that using this typing makes vscode lose the documentation on the decorated method, and that's something we would like to avoid, so we will probably go this the easier (but incorrect) type that preserves it:

import typing

_Fn = typing.TypeVar("_Fn", bound=typing.Callable)

def generative(fn: _FN) -> _FN:
    @decorator
    def _generative(fn, self, *args, **kw):
        """Mark a method as generative."""

        self = self._generate()
        x = fn(self, *args, **kw)
        assert x is None, or x is self "generative methods must have no return value"
        return self

    decorated = _generative(fn)
    # decorated.non_generative = fn
    return decorated

and making each decorated method return self so that the overall typing is correct

@erictraut
Copy link
Collaborator

I've found the cause of the problem. It will be fixed in the next release of pyright.

You make a good point about preserving docstrings in a decorator. Perhaps a ParamSpec should attempt to preserve the docstrings for the function associated with the original signature? I can think of situations where that heuristic might fail. I'll think about it more.

@erictraut erictraut added the addressed in next version Issue is fixed and will appear in next published version label Dec 23, 2021
@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 23, 2021

I've found the cause of the problem. It will be fixed in the next release of pyright.

thanks, great news!

You make a good point about preserving docstrings in a decorator. Perhaps a ParamSpec should attempt to preserve the docstrings for the function associated with the original signature? I can think of situations where that heuristic might fail. I'll think about it more.

I agree that it may be helpful to preserve doc strings, but as you mention that's not always correct.
I don't know if there is a way of enabling this behavior or not, but maybe is better do have the opposite behavior (assume doc strings are preserved even if something they aren't?) but I really don't know what's the most common case here.

@erictraut
Copy link
Collaborator

@CaselIT, I've updated pyright to attempt to preserve docstrings for ParamSpecs in cases where it makes sense. It involves some heuristics, and we may need to make adjustments to these heuristics based on feedback from users, but I think the initial attempt provides a good usability improvement.

@erictraut
Copy link
Collaborator

This is addressed in pyright 1.1.200, which I just published. It will also be included in the next release of pylance.

@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 25, 2021

Thanks for the update and the fast turnaround. I've tried 1.1.200 and I the type is now correctly handled.

I've yet to try the docstring change, since I think I'll need to wait for pylance to be updated. (or is there a feature to tell pyright to print it? something similar to reveal_type ?)

@erictraut
Copy link
Collaborator

The docstring support is available only through the language server features (signature help, hover text, etc.), not through the command-line version of pyright. You can either wait for the next release of pylance (which will probably be the first week of January) or temporarily uninstall pylance and install the pyright VS Code extension in its place. To do this, you'd need to add the following to your VS Code settings:
"python.languageServer": "None"

@CaselIT
Copy link
Contributor Author

CaselIT commented Dec 25, 2021

Thanks, I'll will try it

@zzzeek
Copy link

zzzeek commented Dec 29, 2021

Here's a similar thing we are trying to do with docstrings that does not seem to work. should this work?

import typing
from typing import ParamSpec, TypeVar, Optional, Callable


_TE = TypeVar("_TE")

_P = ParamSpec("_P")


def public_factory(target: Callable[_P, _TE]) -> Callable[_P, _TE]:
    """Produce a wrapping function for the given cls or classmethod."""

    # decoration is normally applied here.  but for example,
    # just return the same function
    return target


class Widget:
    @classmethod
    def _create(cls, x: str) -> "Widget":
        """Create a new Widget

        :param x: the string

        """
        w = Widget.__new__(Widget)
        w.x = x
        return w


widget = public_factory(Widget._create)

# shows docstring on hover
w1 = Widget._create("widget 1")

# does not show docstring on hover
w2 = widget("widget 2")

# this seems to work
if typing.TYPE_CHECKING:
    reveal_type(w1)

    reveal_type(w2)

@erictraut
Copy link
Collaborator

Yes, this should work. You'll see that the docstring appears in the signature help (e.g. if you type widget(). There is a bug in the hover provider that causes it not to appear on hover. I'll fix that in the next release.

@zzzeek
Copy link

zzzeek commented Dec 29, 2021

hey eric -

we were just talking about this. This is an interesting problem because what about the general case like this?

def do_a_thing() -> None:
    """do a thing"""


dt2 = do_a_thing

there's no docstring when hovering over "dt2", because it's a "variable", not a "function".

what makes an assigned member like that be considered "function" vs. "variable"?

thanks for the amazing response time!

@zzzeek
Copy link

zzzeek commented Dec 29, 2021

on this end "widget()" is not showing the docstring in the popup box if I'm just typing the word, but i think my vscode might not be the latest

@erictraut
Copy link
Collaborator

erictraut commented Dec 29, 2021

@zzzeek, yes, your do_a_thing example demonstrates the same hover bug that I just fixed. This will work once the next version of pyright and pylance are published.

sqlalchemy-bot pushed a commit to sqlalchemy/sqlalchemy that referenced this issue Dec 31, 2021
Good new is that pylance likes it and copies over the
singature and everything.
Bad news is that mypy does not support this yet python/mypy#8645
Other minor bad news is that non_generative is not typed. I've tried using a protocol
like the one in the comment but the signature is not ported over by pylance, so it's
probably best to just live without it to have the correct signature.

notes from mike:  these three decorators are at the core of getting
the library to be typed, more good news is that pylance will
do all the things we like re: public_factory, see
microsoft/pyright#2758 (comment)
.

For @_generative, we will likely move to using pep 673 once mypy
supports it which may be soon.  but overall having the explicit
"return self" in the methods, while a little inconvenient, makes
the typing more straightforward and locally present in the files
rather than being decided at a distance.   having "return self"
present, or not, both have problems, so maybe we will be able
to change it again if things change as far as decorator support.
As it is, I feel like we are barely squeaking by with our decorators,
the typing is already pretty out there.

Change-Id: Ic77e13fc861def76a5925331df85c0aa48d77807
References: #6810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addressed in next version Issue is fixed and will appear in next published version bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants