add missing generic types to Worker apis

[arvidfm]

makes pyright stop yelling at me

before:

> poetry run pyright
error: Type of "get_current_worker" is partially unknown
    Type of "get_current_worker" is "() -> Worker[Unknown]" (reportUnknownMemberType)
error: Type of "worker" is partially unknown
    Type of "worker" is "Worker[Unknown]" (reportUnknownVariableType)
error: Type of "get_current_worker" is partially unknown
    Type of "get_current_worker" is "() -> Worker[Unknown]" (reportUnknownMemberType)
error: Type of "worker" is partially unknown
    Type of "worker" is "Worker[Unknown]" (reportUnknownVariableType)
error: Type of "get_current_worker" is partially unknown
    Type of "get_current_worker" is "() -> Worker[Unknown]" (reportUnknownMemberType)
error: Type of "worker" is partially unknown
    Type of "worker" is "Worker[Unknown]" (reportUnknownVariableType)
error: Type of "cancel_node" is partially unknown
    Type of "cancel_node" is "(node: DOMNode) -> list[Worker[Unknown]]" (reportUnknownMemberType)
error: Type of "get_current_worker" is partially unknown
    Type of "get_current_worker" is "() -> Worker[Unknown]" (reportUnknownMemberType)
error: Type of "worker" is partially unknown
    Type of "worker" is "Worker[Unknown]" (reportUnknownVariableType)
error: Type of "cancel_node" is partially unknown
    Type of "cancel_node" is "(node: DOMNode) -> list[Worker[Unknown]]" (reportUnknownMemberType)
error: Type of "cancel_node" is partially unknown
    Type of "cancel_node" is "(node: DOMNode) -> list[Worker[Unknown]]" (reportUnknownMemberType)
11 errors, 0 warnings, 0 informations 

after:

> poetry run pyright
0 errors, 0 warnings, 0 informations

[rodrigogiraoserrao]

Thanks for this.

A couple of the typing changes can be improved and there are also a couple of situations where I think we can do better.

For reference, this is related to #3862.

[rodrigogiraoserrao]

If I'm reading the code correctly, this worker instance comes from _load_directory, so it would be a Worker[list[Path]], which is a bit more specific than Worker[Any].

[arvidfm]

_directory_content is called with the result of get_current_worker() which returns (and can only return) a Worker[Any], so this would cause an error in _load_directory

[rodrigogiraoserrao]

Of course I might be misunderstanding something, but what I'm suggesting is that we narrow the type ourselves, which should work.

E.g., the code below typechecks:

from typing import Any

def any_factory() -> Any:
    ...

integer: int = any_factory()

get_current_worker is being called inside a worker, so we know that the current worker is the function we're in, so we know its type, right?
That was my reasoning.

[arvidfm]

Sorry for not getting back to you earlier, it's been a hectic week! But yes, you're right. I have a bad habit of thinking of Any as the base type, in which case this would have not worked due to invariance, but it's actually an opt-out of the typing system. Personally I always avoid implicit casts from Any, since it means the cast goes completely unchecked and it makes it easy to introduce errors while still being under a false sense of security.

Incidentally, this reminded me of something that I'd had in the back of my mind for a while as something I've been wanting to try in practice, namely this note on Any vs object from this page on typing best practices:

Generally, use Any when a type cannot be expressed appropriately with the current type system or using the correct type is unergonomic.

If a function accepts every possible object as an argument, for example because it’s only passed to str(), use object instead of Any as type annotation. Similarly, if the return value of a callback is ignored, annotate it with object

I actually did have a go this week to see if I could eliminate Any in favour of object in my own codebase for better typing safety, and while I wasn't able to remove all uses due to limitations in Python's typing system (notably the lack of wildcards), I did have decent success and I'm definitely more confident in the type checking now.

If that's something you would be interested in I'd be happy to update this PR to use Worker[object] instead of Worker[Any] where applicable, which would provide better type safety for users of the code and make clear where explicit casts need to be made. It would mean users having to add an explicit cast if they want to get the result of get_current_worker() as a Worker[int] or whatever, but I don't think that's an issue - what would you even use the result type for from the worker itself before the result has been returned? is_cancelled can be checked even if you don't have the specific type.

Anyway! To get back to the point of your comment, there are three options I see here, in descending order of personal preference:

• Keep the argument as Worker[Any] (or better, Worker[object])
  • We're only using the worker to access is_cancelled, and I don't think there's any real reason to add more specialisation than we actually need for the function - less casting means more type safety
• Have get_current_worker return Worker[object] but let _directory_content take a Worker[Path], with an explicit cast to silence the type checker
  • Would at least make it clear that there is casting going on, and it makes it easy to do a search for cast later if one wanted to eliminate casting for the sake of stronger typing guarantees in the future
• Stick to Worker[Any] for get_current_worker and rely on implicit casting to Worker[Path]
  • Not a fan personally! Hard to spot and easy for issues to slip in. Only real way of tracking this down in the future is to run Mypy with --disallow-any-expr which is impossibly noisy

Let me know your thoughts!

[rodrigogiraoserrao]

Thanks for your comments!
I wasn't aware of the best practice of using object (instead of Any) in the contexts listed but it does make sense.
I think we'd better leave it as Worker[Any] to be in line with the remainder of the code base.

I'll muster the courage to review this PR and then I'll rope someone else in to also take a look!

[rodrigogiraoserrao]

@davep sorry to bother, but given that this is related to my PR 3862 and that the errors I see with mypy/Pylance differ from the ones you see with pyright, could you check that this PR does reduce the number of pyright errors, please?

[davep]

@rodrigogiraoserrao I had a look at this a couple of days back but couldn't see any of the errors mentioned, so made a mental note to return to this after I've had another once-over of my Emacs/eglot/pyright setup to see if there's some extra level of checking I'm missing out on (generally I see all the issues I'd expect with assorted Python code, and a failing mypy run generally tends to coincide with similar complaints from pyright).

All of which is to say: I didn't see the errors in the first place.

[arvidfm]

Sorry, to clarify, the errors are from my own project - Pyright is complaining about the fact that I have code that calls third-party functions that are missing type parameters. Also, I'm not sure reportUnknown*Type are on by default, but it should be trivial to reproduce by writing any external code that calls one of these functions and then type checking with Pyright in strict mode.

[arvidfm]

I went ahead and fixed up the WorkType type parameters mentioned above.

I also decided to improve the typing of textual.work, as a lot of type information was getting lost with the current type hints. In particular, the following errors are now caught at typecheck time:

• Decorating a module-level function with @work
• Decorating a method in a non-DOMNode class with @work
• Decorating a non-asynchronous method with @work(thread=False)
• Passing in the wrong number of arguments to workers
• Passing in the wrong argument types to workers
• Assigning the result of a worker to a variable of the wrong type

However, there is a caveat that I couldn't manage to find a solution for. The following is inherently ambiguous:

class MyClass(Widget):
    @work(thread=True)
    async def my_worker() -> int:
        return 0

Since both asynchronous and synchronous functions are allowed when thread=True, decorating a coroutine like this can result in the following type inferences for my_worker, both technically valid:

• Callable[[], Coroutine[None, None, ReturnType]] where ReturnType is inferred as int
• Callable[[], ReturnType] where ReturnType is inferred as Coroutine[None, None, int]

In order to handle this ambiguity I ended up adding a new is_async argument to work() which (thanks to a new @overload) instructs the type checker to use the first inference above. My assumption was that thread=True is mainly used for synchronous functions, which is why I made that the default.

Still, I think the added type safety is worth it, as demonstrated in the following gist, showing the issues that Mypy and Pyright failed to report previously, and how the inferred types of the worker factories were not very informative: https://gist.github.com/arvidfm/a57eb3f451ab76c26b881c3dd515f3bc. Besides, if the user can't be bothered about type checking they don't need to specify the is_async argument anyway, it'll work either way. (Also, it seems Pyright is actually able to infer it correctly even without the argument? Not sure if they've got some special heuristics for that.)

[rodrigogiraoserrao]

Thanks for the extra work put into the work decorator as well.
I've requested a review from my colleagues so that we get an extra set of eyes on this.

Note: at this point, this PR supersedes #3862.

[willmcgugan]

I'm afraid we can't accept this. Too many changes makes it hard to understand what the intent was. Suggest breaking it up in to smaller updates, ideally with a discussion beforehand so we can better understand the problem(s).