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 issue. #3862

Closed
wants to merge 2 commits into from

Conversation

rodrigogiraoserrao
Copy link
Contributor

@willmcgugan like I mentioned previously, this seems to be the issue and Pylance is happy about it but mypy complains in other places about the changes I made.

In particular, running mypy (both 1.7.1 and 1.9.0+dev.0567da99784c376e723907daf4f16df2f03368c1 0567da9) in our codebase introduces these type errors:

new mypy errors
src/textual/widgets/_log.py:120: error: Argument 1 has incompatible type "Callable[[Log, int, List[str]], None]"; expected "Union[Callable[[VarArg(Never), KwArg(Never)], Never], Callable[[VarArg(Never), KwArg(Never)], Coroutine[Any, Any, Never]]]"  [arg-type]
src/textual/widgets/_log.py:223: error: Invalid self argument "Self" to attribute function "_update_size" with type "Callable[[VarArg(Never), KwArg(Never)], Worker[Never]]"  [misc]
src/textual/widgets/_log.py:223: error: Argument 1 to "_update_size" of "Log" has incompatible type "int"; expected Never  [arg-type]
src/textual/widgets/_log.py:223: error: Argument 2 to "_update_size" of "Log" has incompatible type "List[str]"; expected Never  [arg-type]
src/textual/widgets/_directory_tree.py:182: error: Invalid self argument "DirectoryTree" to attribute function "_loader" with type "Callable[[VarArg(Never), KwArg(Never)], Worker[Never]]"  [misc]
src/textual/widgets/_directory_tree.py:396: error: Argument 1 has incompatible type "Callable[[DirectoryTree, TreeNode[DirEntry]], List[Path]]"; expected "Union[Callable[[VarArg(Never), KwArg(Never)], Never], Callable[[VarArg(Never), KwArg(Never)], Coroutine[Any, Any, Never]]]"  [arg-type]
src/textual/widgets/_directory_tree.py:414: error: Argument 1 has incompatible type "Callable[[DirectoryTree], Coroutine[Any, Any, None]]"; expected "Union[Callable[[VarArg(Never), KwArg(Never)], Never], Callable[[VarArg(Never), KwArg(Never)], Coroutine[Any, Any, Never]]]"  [arg-type]
src/textual/widgets/_directory_tree.py:426: error: Invalid self argument "DirectoryTree" to attribute function "_load_directory" with type "Callable[[VarArg(Never), KwArg(Never)], Worker[Never]]"  [misc]
src/textual/widgets/_directory_tree.py:426: error: Argument 1 to "_load_directory" of "DirectoryTree" has incompatible type "TreeNode[DirEntry]"; expected Never  [arg-type]
src/textual/command.py:810: error: Argument 1 has incompatible type "Callable[[CommandPalette, str], Coroutine[Any, Any, None]]"; expected "Union[Callable[[VarArg(Never), KwArg(Never)], Never], Callable[[VarArg(Never), KwArg(Never)], Coroutine[Any, Any, Never]]]"  [arg-type]
src/textual/command.py:948: error: Invalid self argument "CommandPalette" to attribute function "_gather_commands" with type "Callable[[VarArg(Never), KwArg(Never)], Worker[Never]]"  [misc]
src/textual/command.py:948: error: Argument 1 to "_gather_commands" of "CommandPalette" has incompatible type "str"; expected Never  [arg-type]

It does get rid of the errors in _work_decorator.py, both for mypy and pylance.

I'll leave it up to you if we want to roll with these changes, mark this as draft, or just reject the PR.

Extra context:

Fixes #3510 .

@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Dec 13, 2023
Comment on lines -26 to +34
Callable[DecoratorParamSpec, Coroutine[None, None, ReturnType]],
Callable[DecoratorParamSpec, Coroutine[Any, Any, ReturnType]],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coroutines like async def foo() -> T: ... have their type inferred as Coroutine[Any, Any, T], so I swapped all of the Coroutine[None, None, T] for Coroutine[Any, Any, T] under the assumption that the usage of None was a mistake.

@davep
Copy link
Contributor

davep commented Jan 2, 2024

Is it possible to get a concise explanation/example of what this addresses? I've followed the reference chain here but there's a lot of discussion and it's not clear to me what this finally changes and why. For example:

It does get rid of the errors in _work_decorator.py, both for mypy and pylance.

As I look at _work_decorator.py using pyright, I don't see any errors. What should I be looking for?

@rodrigogiraoserrao
Copy link
Contributor Author

What should I be looking for?

Running make typecheck (at the tip of main, 03fb1b1 at the time of writing) will surface one error here:

) -> Decorator[..., ReturnType]:

src/textual/_work_decorator.py:74: error: Type variable "textual._work_decorator.ReturnType" is unbound  [valid-type]
src/textual/_work_decorator.py:74: note: (Hint: Use "Generic[ReturnType]" or "Protocol[ReturnType]" base class to bind "ReturnType" inside a class)
src/textual/_work_decorator.py:74: note: (Hint: Use "ReturnType" in function signature to bind "ReturnType" inside a function)

Additionally, in the signature of the concrete definition of work,

def work(
method: Callable[FactoryParamSpec, ReturnType]
| Callable[FactoryParamSpec, Coroutine[None, None, ReturnType]]
| None = None,
*,
name: str = "",
group: str = "default",
exit_on_error: bool = True,
exclusive: bool = False,
description: str | None = None,
thread: bool = False,
) -> Callable[FactoryParamSpec, Worker[ReturnType]] | Decorator:

Pylance complains that

  1. the return type of work is partially unknown; and
  2. it expected type arguments for the generic type alias Decorator in the return type.

On top of this, typing in places where this decorator is used gets screwed up because inference breaks.

@rodrigogiraoserrao
Copy link
Contributor Author

@willmcgugan Feel free to take a look when you're ready. Might be worth considering #3937 as well.

@willmcgugan
Copy link
Collaborator

@rodrigogiraoserrao I don't think we can accept this if it causes other others. Are the other errors a direct consequence of this change, and could they be fixes relatively easily?

@rodrigogiraoserrao
Copy link
Contributor Author

They are direct consequences.
In principle they cannot be fixed easily because it looks like it surfaced a mypy bug.
At this point, the external PR #3937 supersedes this one.
I'm keen on closing this so as to not waste any more time on this specific PR.

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

Successfully merging this pull request may close these issues.

[typing] Inconsistent typing in the work decorator
3 participants