fix(types): decorator typing fails

[henryiii]

• fixes <nothing> not callable error in Mypy with Click 8.1.0 #2227

I've tried the fix suggested in the issue above by @Dreamsorcerer, and it seems to fix the typing issue locally.

Checklist:

• Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
• Add or update relevant docs, in the docs folder and in code.
• Add an entry in CHANGES.rst summarizing the change and linking to the issue.
• Add .. versionchanged:: entries in any relevant code docs.
• Run pre-commit hooks and fix any issues.
• Run pytest and tox, no tests failed.

[dlax]

This attemps to resolves another issue (similar to #2227, but not that one, and I don't know if it got reported).

import click

@click.group("grp")
def grp() -> None:
    ...

@grp.command
def cmd() -> None:
    ...

reveal_type(cmd)
# (before) Revealed type is "Union[def (def (*Any, **Any) -> Any) -> click.core.Command, click.core.Command]"
# (after, with suggested change)  Revealed type is "click.core.Command"

To resolve #2227, I believe command() in src/click/decorator.py should be adjusted.

[henryiii]

Hmm, that one already has overloads, though. Is the order important and off?

[dlax]

I'd suggest:

diff --git a/src/click/decorators.py b/src/click/decorators.py
index 05d1334..9923fb6 100644
--- a/src/click/decorators.py
+++ b/src/click/decorators.py
@@ -124,6 +124,13 @@ def pass_meta_key(
 CmdType = t.TypeVar("CmdType", bound=Command)
 
 
+@t.overload
+def command(
+    __func: t.Callable[..., t.Any],
+) -> Command:
+    ...
+
+
 @t.overload
 def command(
     name: t.Optional[str] = None,

which leads to:

import click

@click.command
def test() -> None:
    ...

reveal_type(test)
# (before) Revealed type is "<nothing>"
# (after) Revealed type is "click.core.Command"

hence resolving #2227.

[davidism]

@dlax that doesn't appear to be the correct annotation though, there is no __func parameter. It's the first parameter name that can be str, Callable, or None. If it works, I'd like to understand why, given that there are already overloads representing what it does.

[davidism]

I can't figure out why the overload is causing mypy to think the return value is "nothing". I tried shuffling the order around, or making it use the CmdType var everywhere, and neither worked.

Simplifying the overloads by removing the two for CmdType does make it work, at the expense of the type not being correct when passing cls. This suggests that the solution that @dlax suggested isn't correct because the existing overloads for str vs Callable work in that case.

[henryiii]

A double underscore parameter in mypy is a nameless parameter (stand-in for the / syntax before Python 3.8). I can verify that the @dlax solution didn't solve it for me (trying on the original problem statement). The problem is, I think, that the overloads are overlapping.

[henryiii]

Is it really considered "valid" to pass a callable any other way other than by "leaving off" the parenthesis? You can have more limited typing that what is technically allowed at runtime. This could also be a bit more enforced at runtime, too.

[davidism]

I agree this isn't intended to work with a callable and other args. Would that change what the annotation is and fix this?

[henryiii]

That's what I'm trying. Currently doesn't seem to, but I think it should, playing with it.

[davidism]

Is there someone from mypy we can ping to help out? I'm really out of my depth when it comes to these typing issues.

[henryiii]

I think I got it, just a minute.

[henryiii]

FYI, you can't run tox -e typing on macOS due to the unreachable setting. Until python/mypy#12286 is implemented, you can do this:

diff --git a/tox.ini b/tox.ini
index 056ca0d..8afb83e 100644
--- a/tox.ini
+++ b/tox.ini
@@ -17,7 +17,7 @@ commands = pre-commit run --all-files --show-diff-on-failure

 [testenv:typing]
 deps = -r requirements/typing.txt
-commands = mypy
+commands = mypy --platform=linux

 [testenv:docs]
 deps = -r requirements/docs.txt

I can add that in another PR if you think it's useful.

[henryiii]

By the way, having tests for typing would be nice (it feels weird to have "typing tests", so don't have much experience with best practices there).

[davidism]

Sure, I'd be fine addressing that with another PR. Maybe disable warn_unreachable instead? I think flake8 or a plugin can check for unreachable code too.

I have no idea how to write typing tests, maybe add typing to the test suite itself and run mypy against it as well? Seems like it could be good for the sprint at at PyCon.

[henryiii]

MyPy's unreachable is very powerful, though, and can catch things that others can't, like code after types have been fully narrowed, etc.

I've mostly avoided typing tests, since tests often literally break typing constructs in order to test errors (or at least they should). Enabling pytest in typing also slows down typing a bit - I tend to use mypy via pre-commit. But maybe that would be worthwhile; it would likely expose things like this. I'd probably not make the tests "strict", though, (no need to type every test function) and I've had little success in 'turning off' aspects of strictness in specific directories. But maybe that would be a good way to do it.

Typing examples might be more useful, though often people don't put types on them to make them "friendly" to beginning programmers. (Does it really? I don't know, I find types make code much easier to read).

[davidism]

Yeah, that's been my experience too with typing tests. Thanks for working on this, I will merge and make a release in a bit.

[dlax]

Last changes seem to fix things as far as I can tell, thanks @henryiii

[dlax]

Type-checking the test suite would indeed help. To avoid the burden of adding annotations to all test functions, while keeping mypy strict mode, one can use:

[mypy-tests.*]
check_untyped_defs = true
disallow_untyped_defs = false
disallow_untyped_calls = false

Then running mypy tests finds 41 errors in 11 files (checked 22 source files). Not so much, I'd say.

[henryiii]

Let me know if you see anything that you'd like different or added!

[henryiii]

You cannot pass other arguments if you use it as a decorator without parenthesis. I've added assert statements requiring this be true. If you want to add other arguments, it should be used in the traditional way as a factory. And mypy certainly should not encourage that even if was supported at runtime!

[Dreamsorcerer]

By the way, having tests for typing would be nice (it feels weird to have "typing tests", so don't have much experience with best practices there).

Best approach is simply to type check tests and examples, as these are the places where you actually use the API.

I've mostly avoided typing tests, since tests often literally break typing constructs in order to test errors (or at least they should).

While this is true in the case of checking that an exception is raised or similar, I find that it's not that frequent, and is easily managed with the occasional type: ignore[...]. When I find lots of typing errors in some test code, it's usually because someone has done a bunch of monkey patching instead of using mock.patch(), which is just bad practice.

I've had little success in 'turning off' aspects of strictness in specific directories.

We do this in the aio-libs projects. One thing that might catch people out, is legacy pytest behaviour discouraged having a tests/__init__.py (whereas future pytest behaviour appears to require it). Mypy config will only work with modules, so that file must exist.

Typing examples might be more useful, though often people don't put types on them to make them "friendly" to beginning programmers. (Does it really? I don't know, I find types make code much easier to read).

We usually have a quickstart in the readme or something which is not checked, but then everything under examples/ will be strictly typed.

If anybody wants some hints setting any of this up, just ping me in another PR. Also feel free to look at one of our config files as an example: https://github.com/aio-libs/aiohttp-jinja2/blob/master/.mypy.ini

[henryiii]

That sounds good - as I've said, haven't had much experience typing tests, so didn't now how well it might work. I expect the module __init__.py is the problem - placing that in examples and tests sounds dangerous, brings back memories of packages that include a tests "module" in the package due to package auto-discovery. Do you have a link the change that would require __init__.py in pytest? I'm quite sure that was the problem now that you mention it. MyPy does have some functionality for namespace packages that might be useable for examples and/or tests introduced pretty recently (0.930 or so?). I wouldn't mind examples being fully typed, but I know some packages strictly against adding any types to examples.

I'll be trying this out one of my packages soon, thanks!

[Dreamsorcerer]

Do you have a link the change that would require __init__.py in pytest?

The lack of __init__.py appears to only be supported via a legacy feature:

This is the classic mechanism, dating back from the time Python 2 was still supported.

While having __init__.py appears to be a requirement in future:

Users which require this should turn their tests into proper packages instead.

We intend to make importlib the default in future releases.

https://docs.pytest.org/en/6.2.x/pythonpath.html#import-modes

If you're worried about the possible side effects from using the legacy import with __init__.py, you can enable --import-mode=importlib and start using the new approach now, which doesn't suffer from those side effects.

MyPy does have some functionality for namespace packages that might be useable for examples and/or tests introduced pretty recently (0.930 or so?).

They changed the behaviour to start picking up directories which are not modules. But, the config format does not allow configuring these. I raised a bug when this happened: python/mypy#10045
Unless something has changed in the last year and that bug just hasn't been updated..

[henryiii]

No, that's only a requirement for importing from your own test suite (which is a bit of an antipattern in pytest, use contest.py and fixtures instead usually). Unless you need to import from your test module, you still don't need an __init__.py. I've got 1-2 projects that have an __init__.py in the tests specifically because they need to import something, but otherwise, I don't think tests should have one - they are not importable code, and should not be discovered as modules (especially by your packaging system!).

Ahh, but that bug in mypy is probably a problem... Though it looks to me you might be able to use the test file name pattern instead, such as test_*. (Doesn't help examples, but at least for tests).

[davidism]

I rebased to the 8.1.x maintenance branch, added a changelog, and changed the assertion wording a bit.

[davidism]

Click 8.1.1 is now available: https://pypi.org/project/click/8.1.1/