-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix crash on overriding class level type alias #5662
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
# Overriding a type alias is always an error, it is static by nature. | ||
self.msg.cannot_overide_alias(original_node.name(), base.name(), defn) | ||
return False | ||
if original_type is None: | ||
if self.pass_num < self.last_pass: | ||
# If there are passes left, defer this node until next pass, | ||
# otherwise try reconstructing the method type from available information. | ||
# If there are passes left, defer this node until next pass... | ||
self.defer_node(defn, defn.info) | ||
return True | ||
# ...otherwise try reconstructing the supertype from available information. | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
else: | ||
assert False, str(base_attr.node) | ||
if isinstance(original_node, (FuncDef, OverloadedFuncDef)): | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
# Overriding a type alias is always an error, it is static by nature. | ||
self.msg.cannot_overide_alias(base_node.name(), base.name(), lvalue) | ||
return True | ||
|
||
if base_type: | ||
assert base_node is not None | ||
if not self.check_compatibility_super(lvalue, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Slightly unrelated: Could you update the docstring to describe the return value? Now it's not immediately clear why this is correct. |
||
|
||
if isinstance(base_node, Decorator): | ||
base_node = base_node.func | ||
base_type = base_node.type | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -611,3 +611,33 @@ except E as e: | |
reveal_type(e) # E: Revealed type is '__main__.E' | ||
[builtins fixtures/exception.pyi] | ||
[out] | ||
|
||
[case testNoOverrideAliasInSubclassMethod] | ||
from typing import Any, Callable | ||
|
||
class Parent: | ||
foo = Callable[..., int] | ||
|
||
class Child(Parent): | ||
def foo(self, val: int) -> int: ... # E: Cannot override type alias "foo" defined in base class "Parent" | ||
[out] | ||
|
||
[case testNoOverrideAliasInSubclassVar] | ||
from typing import Any, Callable | ||
|
||
class Parent: | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
[out] | ||
|
||
[case testNoOverrideAliasInSubclassAlias] | ||
from typing import Any, Callable | ||
|
||
class Parent: | ||
foo = Callable[..., int] | ||
|
||
class Child(Parent): | ||
foo = Callable[..., int] # E: Cannot override type alias "foo" defined in base class "Parent" | ||
[out] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).