Repository navigation
Fix crash on overriding class level type alias - #5662
ilevkivskyi wants to merge 1 commit into
Conversation
Sorry, something went wrong.
JukkaL
left a comment
There was a problem hiding this comment.
I'm a little worried that doesn't fix all related issues (see comments).
Sorry, something went wrong.
| base_type = base_var.type | ||
|
|
||
| if isinstance(base_node, TypeAlias): | ||
| return None, base_node |
There was a problem hiding this comment.
Slightly unrelated: Could you update the docstring to describe the return value? Now it's not immediately clear why this is correct.
Sorry, something went wrong.
| elif isinstance(original_node, Decorator): | ||
| original_type = self.function_type(original_node.func) | ||
| elif isinstance(original_node, Var): | ||
| original_type = AnyType(TypeOfAny.implementation_artifact) |
There was a problem hiding this comment.
It's not clear to me how this is related to the title of the PR, but the description seems to indicate that this fixes another, related issue with Var nodes. Maybe tweak the title of the PR?
Also, this looks like it could cause false negatives. It's better than crashing, but maybe we should generate a "Cannot determine type" error instead? At least there should be a comment about this, and potentially a follow-up issue.
Sorry, something went wrong.
| # it can be checked for compatibility. | ||
| original_type = base_attr.type | ||
| original_node = base_attr.node | ||
| if isinstance(original_node, TypeAlias): |
There was a problem hiding this comment.
Should we reject other kinds of nodes as well, such as TypeInfo, TypeVarExpr or module reference? Some of these could cause crashes as well. It would be good to fix other related issues at the same time.
I think that it would be best to check all combinations of base class and subclass definitions, such as 1) type alias 2) class 3) function 4) variable 5) type variable 6) module reference 7) other magic assignments (NewType or NamedTuple or TypedDict). Some of these are already tested, so they can be left out (like function vs function).
Sorry, something went wrong.
|
|
||
| base_type, base_node = self.lvalue_type_from_base(lvalue_node, base) | ||
|
|
||
| if isinstance(base_node, TypeAlias): |
There was a problem hiding this comment.
Similar to above, should we check for other kinds of things that shouldn't be overridden?
Why do we this check in two places? If we need to have the check in two places, it may be slightly better to move it to a helper method instead of repeating the code, at least if the check becomes more complex.
Sorry, something went wrong.
| foo = Callable[..., int] | ||
|
|
||
| class Child(Parent): | ||
| foo: Callable[..., int] # E: Cannot override type alias "foo" defined in base class "Parent" |
There was a problem hiding this comment.
I'd merge all these test into a single test case. Having many test cases that test almost the same thing arguably doesn't help maintainability but is kind of expensive to run.
Sorry, something went wrong.
Yes, indeed, there are much more problems here:
I don't think however I want to open this can of worms, we can look at it later when we will have time. |
Sorry, something went wrong.
Fixes #5425
The solution is to just prohibit overriding type aliases in subclass. In some sense they are implicitly final, because we need to know them statically (they can be used as type sin annotations).
The assert that was triggered missed two cases for superclass node: TypeAlias and Var, for Var I just return Any since there are no way to determine type if deferring failed (currently there is fixed number of type-checking passes).