Type hint improvements

[stephenfin]

These were spotted while working on adding type hints to debtcollector. I believe the simplified types are correct, as evidenced by me no longer needing type: ignore statements in debtcollector plus the reduced failures in the tests, but this should definitely receive the twice-over.

[GrahamDumpleton]

What is debtcollector and what static type checker tool are you targeting?

The type hints are the way they are because a more simplified approach doesn't work with various type checkers and results in various cases being flagged as problems when they aren't.

Would also have been helpful to see how you are using type hints in your usage of wrapt. The docs show using it in a specific way, again because of limitations in static type checkers.

[stephenfin]

I'm away without laptop this week so will have to follow up with a longer reply next week, but debtcollector is https://github.com/openstack/debtcollector and we're targeting mypy. I'm typing that as part of a larger effort to type various openstack libraries.

If you look at master of same, you'll find a TODO(stephenfin) that identifies the issue I'm attempting to fix here. I did not check with pyright etc. but in my experience, pyright is "cleverer" than mypy and generally needs last hand holding, so I'd be suprised if this fix didn't also work with that. I've no idea about ty, pyrefly, or zuban though.

[stephenfin]

Also, the fact that mypy at least is now able to correctly identify incorrect parameters and returns types rather than returning Never is a important signifier (IMO) that this is an improvement rather than a regression. I may have missed something however.

[GrahamDumpleton]

I can't find the TODO you reference.

[stephenfin]

Apologies, it hasn't merged yet.

https://review.opendev.org/c/openstack/debtcollector/+/972447

[GrahamDumpleton]

FWIW. For reference, prior type hint work on wrapt was based on testing against mypy 1.18.1. I see there is a 1.19 now. I don't know if that newer version addresses issues I see. From memory if plain Callable was used instead of complex overload I ended up using, mypy wouldn't work properly when wrapt decorators were applied to methods of a class, it just wouldn't handle self/cls argument properly and always complained was wrong number of arguments.

[GrahamDumpleton]

I could also have that round the wrong way, and it is pyright that didn't work without the mess of overloads. So making if just Callable could well work for mypy on either version, but then pylance in VS Code will not work properly. So unfortunately compromises are needed since these static type checkers tools are not fool proof and still a work in progress.

[GrahamDumpleton]

So some of the suggested changes for __get__() overloading on FunctionWrapper and BoundFunctionWrapper may be partly valid. Seems I missed at one point that mypy behaviour changed in one of the prior version updates and I incorporated the new errors in to the expected error messages for tests rather than realise that it showed an underlying issue that needed addressing.

Am not sure that:

        @overload
        def __get__(
            self, instance: None, owner: type[Any] | None = None
        ) -> "FunctionWrapper[P, R]": ...

is correct though in returning FunctionWrapper though, rather than BoundFunctionWrapper for the case of instance is None. Technically the code does still return the later for that.

I also need to understand why you use:

    _P = ParamSpec("_P")
    _R = TypeVar("_R")
    _T = TypeVar("_T")

and why existing:

    P = ParamSpec("P")
    R = TypeVar("R", covariant=True)
    T = TypeVar("T", bound=Any)

were not appropriate.

Still have big concerns over simplification of:

            callable: (
                Callable[P, R]
                | Callable[Concatenate[type[T], P], R]
                | Callable[Concatenate[Any, P], R]
                | Callable[[type[T]], R]
                | Descriptor
            ),
        ) -> FunctionWrapper[P, R]: ...

As seems to happen quite often to me, not having using VS Code for a couple of months for this, its type checking features are totally broken for pylance and don't know why, so hard to test things right now with it.

[GrahamDumpleton]

As usual, start tweaking stuff and other stuff just starts breaking. This is going to take a while to sort out as usual. 😩

[GrahamDumpleton]

Got pylance type checking working again. Simplifying from:

            callable: (
                Callable[P, R]
                | Callable[Concatenate[type[T], P], R]
                | Callable[Concatenate[Any, P], R]
                | Callable[[type[T]], R]
                | Descriptor
            ),

to:

            callable: (
                Callable[P, R]
            ),

breaks pylance.

So that is battle having to face.

So need to have a solution which can retain that yet things still work with mypy.

[GrahamDumpleton]

I may have found a solution. Perhaps can use:

class FunctionDecorator:

    @overload
    def __call__(
        self,
        callable: Callable[P, R]
                | Callable[Concatenate[type[T], P], R]
                # | Callable[Concatenate[Any, P], R] # Don't use, breaks mypy.
                | Callable[[type[T]], R]
    ) -> FunctionWrapper[P, R]: ...
    @overload
    def __call__(
        self,
        callable: Descriptor
    ) -> FunctionWrapper[P, Any]: ...

Descriptor case broken out into separate overload.

Can't have one of the argument alternatives (need to investigate implications of that on other things).

The FunctionDecorator no longer derives from Generic[P, R], with changes to where that is used.

[GrahamDumpleton]

FWIW, the changes also seem to make some things work (different to mypy issues) for ty as well which weren't working on basic test.

[GrahamDumpleton]

But all of mypy, pyrefly and ty break when decorating a method of a class. Only pypright works correctly and that relies on the more complicated overload on callable.

Only solution I can see right now is to extract a minimal self contained example of the problem and report issues against tools that cannot handle it.

[stephenfin]

Again, I'm away at the moment so won't reply in detail yet but on this point:

I also need to understand why you use:

_P = ParamSpec("_P")
_R = TypeVar("_R")
_T = TypeVar("_T")

I called this out in one of the commit messages, but it's because the non-underscored variants are bound to the wrapper function arguments, so the underscored variants are needed for the wrapped callable's arguments since those are different. Without this, you can't do something like the below since the paramspec of the decorator and decorated function differs (i.e. the expected case).

@mydecorator(a=123)
def foo(a: str, b: str) -> str:
    return a + b

This change alone gives a marked improvement in results.

[GrahamDumpleton]

Not sure what you are trying to say there without a full example.

As far as I can work out, the separate P and R are more to do with having a P which is distinct from T when binding occurs. This seems to be necessary for mypy, pyrefly and ty. It is not necessary for pyright which appears to work out binding for methods without it.

That said, even when incorporate it in certain cases in a way that works for those three tools and still works for pyright, it still does not work for decorators applied to staticmethod for mypy, pyrefly and ty. Only pyright works properly still for all method types.

So I don't really see it as directly related to difference between wrapped and wrapper arguments, so not sure what you are getting at.

I would need to see a more complete example of the corresponding wrapper part of the decorator for what you are talking about. As is, the docs steer one away from using explicit types on the wrapper and instead say to use:

@wrapt.decorator
def pass_through(
    wrapped: Callable[P, R],
    instance: Any,
    args: tuple[Any, ...],
    kwargs: dict[str, Any],
) -> R:
    return wrapped(*args, **kwargs)

Using P and R here like that seemed to be necessary to have things connect together more reliably. Being very precise on callable prototype for wrapped and the return type of the wrapper function tended to cause various complaints from the type checkers.

Anyway, am incrementally working through things and redoing how I test things so can check for all four static type checker tools and come up with a table of what things don't work for which tools. I will be doing a bit more on it over the weekend and if happy with progress may make a 2.1.0dev2 release some with changes which progress things a bit.

[GrahamDumpleton]

I have released 2.1.0dev2 to PyPi which incorporates your ideas but changes things partly back to how they were to ensure that pyright still works. When you get back see how far you can get with that version.

Current issues I still know of:

• For mypy and ty, they still don't handle properly when you have decorators applied to static methods of classes. I have found some discussion about how mypy struggles where have objects which behave as descriptors wrapping class and static methods, so looks like a known issue of sorts with mypy.
• For pyrefly, it has lots of problems with any sort of method of a class, ie., instance methods, class methods and static methods.

FWIW, the repo have been using to test new type hints behaviour with wrapt without needing to modify wrapt itself is:

• https://github.com/GrahamDumpleton/wrapt-type-hints