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

Fix decorator typing with _Decorator type alias #2559

Closed
wants to merge 1 commit into from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Jul 7, 2023

_Decorator[FC] is identical to t.Callable[[FC], FC]. However, mypy currently reads the first type incorrectly and supports the second one fully.

This fix will make mypy checking work correctly on click decorators, and should have no negative impact on other type checkers because the types are effectively "the same".

I plan to open a bug report against mypy to ask about this issue. It may be a duplicate (python/mypy#13250 ?), but we'll have a thread to pull on there to figure out why this isn't supported when there doesn't appear to be a meaningful difference between the two types.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.

I haven't added tests because any mypy testing will require some kind of new framework for testing types.
I think this would be valuable and not hard (e.g. here's some prior art in marshmallow which I helped setup), but it's beyond the scope of a small typing fix.

Instead, I tested mypy v1.4.1 (latest) before and after this change on the following small sample:

from click import option

def foo(a: int) -> str:
    return str(a)

reveal_type(option)
foo = option()(foo)
reveal_type(foo)

Before the change:

$ mypy click_example.py 
click_example.py:8: note: Revealed type is "def [FC <: Union[def (*Any, **Any) -> Any, click.core.Command]] (*param_decls: builtins.str, *, cls: Union[Type[click.core.Option], None] =, **attrs: Any) -> def (FC`-1) -> FC`-1"
click_example.py:9: error: Argument 1 has incompatible type "Callable[[int], str]"; expected <nothing>  [arg-type]
click_example.py:10: note: Revealed type is "<nothing>"
Found 1 error in 1 file (checked 1 source file)

After the change:

$ mypy click_example.py
click_example.py:8: note: Revealed type is "def (*param_decls: builtins.str, *, cls: Union[Type[click.core.Option], None] =, **attrs: Any) -> def [FC <: Union[def (*Any, **Any) -> Any, click.core.Command]] (FC`-1) -> FC`-1"
click_example.py:10: note: Revealed type is "def (a: builtins.int) -> builtins.str"
Success: no issues found in 1 source file
  • (N/A) 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.

I added a changelog section for 8.1.5 since this is the first change after 8.1.4. I'm happy to make tweaks and changes if this was not appropriate.

  • (N/A) Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@sirosen sirosen changed the base branch from main to 8.1.x July 7, 2023 14:33
`_Decorator[FC]` is identical to `t.Callable[[FC], FC]`. However,
`mypy` currently reads the first type incorrectly and supports the
second one fully.

This fix will make mypy checking work correctly on click
decorators, and should have no negative impact on other type checkers
because the types are effectively "the same".
@sirosen sirosen force-pushed the fix-decorator-annotations branch from 4540d77 to 8f4a92b Compare July 7, 2023 14:34
@sirosen
Copy link
Contributor Author

sirosen commented Jul 7, 2023

No need to file a separate mypy bug. I'm pretty sure this is python/mypy#13449 , and I've left a comment there to hopefully engage the maintainers there.

@davidism
Copy link
Member

davidism commented Jul 9, 2023

Thanks for working on this and the good writeup. Looks like #2562 fixes the issue more broadly. Can you check that as well before I close this one, just in case.

@sirosen
Copy link
Contributor Author

sirosen commented Jul 10, 2023

Yeah, it looks like we converged on the same solution (_Decorator[FC] => Callable[[FC], FC]), so there's no reason to keep this open if those typing tests are good (they look good to me).

Also worth mentioning as I close: mypy maintainers confirmed that this is a mypy bug and it's apparently fixed in their main branch. So this whole kerfuffle would have blown over if mypy had released that fix. 🥲

EDIT: I just reread the comment over there, and it seems like it's fixed but gated behind some kind of preview flag in mypy. So actually a mypy release wouldn't have helped.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typing changes in 8.1.4 cause failure of mypy check
2 participants