Fix crash on overriding class level type alias #5662
Conversation
| @@ -1923,6 +1934,10 @@ def lvalue_type_from_base(self, expr_node: Var, | |||
| if base_var: | |||
| base_node = base_var.node | |||
| 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.
| elif isinstance(original_node, (FuncDef, OverloadedFuncDef)): | ||
| original_type = self.function_type(original_node) | ||
| 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.
| @@ -1360,16 +1360,22 @@ def check_method_override_for_base_with_name( | |||
| # 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).
| @@ -1845,6 +1851,11 @@ def check_compatibility_all_supers(self, lvalue: RefExpr, lvalue_type: Optional[ | |||
|
|
|||
| 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.
| 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.
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. |
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:
TypeAliasandVar, forVarI just returnAnysince there are no way to determine type if deferring failed (currently there is fixed number of type-checking passes).