Detect functions that should use ParamSpec, but don't

[AlexWaygood]

No description provided.

[AlexWaygood]

I'll hold off filing a typeshed PR until folks have had a chance to review. From my perspective, they nearly all look like true positives (though we can't necessarily do much about a lot of them until we can use Concatenate in typeshed).

[sobolevn]

I haven't checked all output items, but this one looks like a false positive:

 ./stdlib/functools.pyi:105:9: Y032 Consider using ParamSpec to annotate function "singledispatchmethod.register"
./stdlib/functools.pyi:107:9: Y032 Consider using ParamSpec to annotate function "singledispatchmethod.register"

Link: https://github.com/python/typeshed/blame/master/stdlib/functools.pyi#L105-L107

[AlexWaygood]

I haven't checked all output items, but this one looks like a false positive:

 ./stdlib/functools.pyi:105:9: Y032 Consider using ParamSpec to annotate function "singledispatchmethod.register"
./stdlib/functools.pyi:107:9: Y032 Consider using ParamSpec to annotate function "singledispatchmethod.register"

Agreed -- but one or two false positives are fine, we can just # noqa them. There would have been even more true positives if you hadn't already cleaned up the stdlib stubs ;)

[AlexWaygood]

After python/typeshed#7050 and python/typeshed#7055 are merged, I don't think any more items output by typeshed-primer will be actionable. The remaining items will all need to be # noqad if this PR is accepted; they either need to wait until we have Concatenate support, or they're false positives.

[JelleZijlstra]

Let's see what the remaining damage is. Thanks for working on this!

[AlexWaygood]

[Edited following @JelleZijlstra's comment below]

There are 30 remaining entries. Of these:

• curses.wrapper, sqlalchemy.engine.base.Connection and sqlalchemy.engine.base.Engine are true positives, but nothing can be done about them until we have Concatenate support. That adds up to 6 entries.
• freezegun, functools.partial and functools.partialmethod all need Concatenate support, and might not be fully expressible in the current type system even with full implementations of PEP 612 and PEP 646. Those add up to 7 entries.
• unittest and singledispatch are special cases where we don't want to use ParamSpec. Those add up to 17 entries.

[JelleZijlstra]

Concatenate can't describe the type of functools.partial, because partial can also be used to partially apply keyword arguments.

[sobolevn]

See partial typing: https://github.com/dry-python/returns/blob/master/returns/contrib/mypy/_features/partial.py 😆

[AlexWaygood]

Concatenate can't describe the type of functools.partial, because partial can also be used to partially apply keyword arguments.

Er... could you do something like this?? (But very blue-sky stuff whatever the case; we obviously can't do anything about it for now...)

from typing import Any, Callable, Concatenate, Generic, ParamSpec, TypeVar, TypeVarTuple, overload

_P1 = ParamSpec("_P1")
_P2 = ParamSpec("_P2")
_T = TypeVar("_T")
_R_co = TypeVar("_R_co", covariant=True)
_Ts = TypeVarTuple("_Ts")

class partial(Generic[_P1, _P2, _T, _R_co, *_Ts]):
    @overload
    def __new__(cls, __func: Callable[_P1, _R_co]) -> partial[_P1, _P1, Any, _R_co]: ...
    @overload
    def __new__(cls, __func: Callable[Concatenate[*_Ts, _P2], _R_co], *args: *_Ts) -> partial[Concatenate[*_Ts, _P2], _P2, Any, _R_co, *_Ts]: ...
    @overload
    def __new__(cls, __func: Callable[_P1, _R_co], *args: *_Ts, **kwargs: _T) -> partial[_P1, ..., _T, _R_co, *_Ts]: ...
    def __call__(self, *args: _P2.args, **kwargs: _P2.kwargs) -> _R_co: ...
    func: Callable[_P1, _R_co]
    args: tuple[*_Ts]
    keywords: dict[str, _T]

[Akuli]

One corner case that I think wouldn't work: if you partial a keyword argument in _P1, it should be possible to specify the same keyword argument in _P2 to override what was specified in _P1.

>>> import functools
>>> foo = functools.partial(print, sep="-")
>>> foo(1, 2, 3)
1-2-3
>>> foo(1, 2, 3, sep="___")  # sep specified twice, not an error
1___2___3

I don't think it's possible to express "discard overlapping keyword arguments from _P1".

[AlexWaygood]

Anyway... partial typing and merge conflicts in the README aside, do we think this PR is mergeable? If so, I can file a typeshed PR adding the necessary # noqa comments :)

[JelleZijlstra]

I feel the false positive ratio might be too high, as with #135. Also, I feel like we're pretty likely to notice this in code review for new functions. I'm interested in others' opinions too.

[AlexWaygood]

I just pushed an alternative implementation where we only error for def func(arg1: Callable[..., _R], *args: Any, **kwargs: Any) -> _R. But, it looks like the false positives are still too numerous. So I'm closing this PR for now — we can come back to this if we develop a way to have a non-blocking GitHub Actions comment-bot :)

Thanks for the discussion everyone!