Closed michelwi closed 9 months ago
but type checker
This looks like a bug in the type checker. isinstance(job, Job)
clearly does not mean that it is not a derived class.
I would not remove this assert just because of some buggy type checker.
I'm also curious whether Pyright / Pylance are maybe just misconfigured? I hesitate to believe they really get such basic thing wrong.
PyCharm gets this correct.
bug in the type checker [...] PyCharm gets this correct.
consider these lines of code
1. job = super(Job, cls).__new__(cls)
2. assert isinstance(job, Job)
3. job._sis_tags = tags
For line 1. both pycharm and pyright complain that 'JobSingleton' is not an instance or a subclass of 'Job'
.
Both infer the type of job
to be Any
at this point.
Then, after the assert in line 2., in line 3. pycharm infers the type to be Any
still and pyright infers Job
.
(I would argue that pyright is "more correct" here)
Then once one defines a subclass of Job and creates an instance of it,
from sisyphus import Job
class Foo(Job):
pass
foo = Foo()
pyright seems to stick with the Job
type, while pycharm reads the correct class from the assignment.
From the error in line 1. I assume that the "metaclass with overridden __call__
method magic" in JobSingleton
is too complicated for both type checkers. I have tried to come up with a reformulation of the JobSingleton
class but was not successful yet.
One thing we probably should change:
class JobSingleton(type):
def __call__(cls, *args, **kwargs):
to
class JobSingleton(type):
def __call__(cls: Type[T], *args, **kwargs) -> T:
(E.g. check mypy doc.)
Instead of just using T = TypeVar("T")
, maybe you want to use:
T = TypeVar("T", bound="Job")
Then, after the assert in line 2., in line 3. pycharm infers the type to be
Any
still and pyright infersJob
. (I would argue that pyright is "more correct" here)
You need to differentiate the cases when PyCharm or other type checkers think that job
has exactly this type (type(job) is Job
) or is inherited from it (isinstance(job, Job)
). I think when PyCharm infers the type to be Any
, this refers to the exact type, which is still any type, with the only additional information that it is inherited from Job
.
But I'm also not sure.
Yes, in any case, metaclasses and such logic will confuse/break most/all type checkers.
But I still would be hesitant to remove a correct assert check just because some type checker is buggy/broken/incomplete.
Can't you add some comment which would make this code ignored by Pyright / Pylance? Most software would ignore it when you add noqa
, like:
assert isinstance(job, Job) # noqa
@michelwi could you try if adding # noqa
solves your problem?
I agree with @albertz that keeping the assert would be better, so it would be great if we find a solution to keep it and to solve your problem.
And yes: Having more type hinds in Sisyphus would be great and it's of cause hard for any type checker to follow that metaclasses logic...
could you try if adding
# noqa
solves your problem?
It or any other pragma variant does not. But @albertz idea with TypeVar annotation seems to also solve my problem. I will then add the assert again together with the type hints.
This line breaks my type checker (Pyright / Pylance) and makes it think that all instances of specific jobs are of the parent class
Job
. e.g.The code works, but type checker complains that
sisyphus.Job
has no memberfoo
.Could we remove this line?