From 3cadb014ff5d164d9c72838cb6ee9371f7502388 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 26 Apr 2017 16:26:27 +0100 Subject: [PATCH 1/7] Type check arguments for "super" This is not quite as strict as we could be, but this is still better than what we had before. A full implementation will be somewhat complicated. Remaining issues: * Assume that the first argument refers to the enclosing class, even though that's not generally the case. * Assume that a type object as the second argument is always fine. * Don't validate some kinds of type object types as the first argument. * Single-argument variant is not supported. Fixes #526 (modulo the open issues). Also, I no longer think that defining messages as constants in `mypy.messages` is generally a good idea so I'm not following that pattern here. I'll officially update the convention in a separate PR. --- mypy/checkexpr.py | 54 ++++++++++++ mypy/fastparse.py | 2 +- mypy/fastparse2.py | 2 +- mypy/nodes.py | 4 +- mypy/semanal.py | 2 + mypy/treetransform.py | 2 +- test-data/unit/check-python2.test | 9 ++ test-data/unit/check-super.test | 139 +++++++++++++++++++++++++++++- 8 files changed, 206 insertions(+), 8 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index 41523a95b967b..ad8eeb5d62f2c 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -2066,9 +2066,63 @@ def infer_lambda_type_using_context(self, e: LambdaExpr) -> Tuple[Optional[Calla def visit_super_expr(self, e: SuperExpr) -> Type: """Type check a super expression (non-lvalue).""" + self.check_super_arguments(e) t = self.analyze_super(e, False) return t + def check_super_arguments(self, e: SuperExpr) -> None: + """Check arguments in a super(...) call.""" + if ARG_STAR in e.call.arg_kinds: + self.chk.fail('Varargs not supported with "super"', e) + elif e.call.args and set(e.call.arg_kinds) != {ARG_POS}: + self.chk.fail('"super" only accepts positional arguments', e) + elif len(e.call.args) == 1: + self.chk.fail('"super" with a single argument not supported', e) + elif len(e.call.args) > 2: + self.chk.fail('Too many arguments for "super"', e) + elif self.chk.options.python_version[0] == 2 and len(e.call.args) == 0: + self.chk.fail('Too few arguments for "super"', e) + elif len(e.call.args) == 2: + type_obj_type = self.accept(e.call.args[0]) + instance_type = self.accept(e.call.args[1]) + if isinstance(type_obj_type, FunctionLike) and type_obj_type.is_type_obj(): + type_info = type_obj_type.type_object() + elif isinstance(type_obj_type, TypeType): + item = type_obj_type.item + if isinstance(item, AnyType): + # Could be anything. + return + if not isinstance(item, Instance): + # A complicated type object type. Assume it's fine. + # TODO: Do something more clever here. + return + type_info = item.type + elif isinstance(type_obj_type, AnyType): + return + else: + self.chk.fail('First argument for "super" must be a type object', e) + return + + if isinstance(instance_type, (Instance, TupleType, TypeVarType)): + if isinstance(instance_type, TypeVarType): + # Needed for generic self. + instance_type = instance_type.upper_bound + if not isinstance(instance_type, (Instance, TupleType)): + # Too tricky, give up. + self.chk.fail('Unsupported argument 2 for "super"', e) + return + if isinstance(instance_type, TupleType): + # Needed for named tuples and classes derived from Tuple[...]. + instance_type = instance_type.fallback + if type_info not in instance_type.type.mro: + self.chk.fail('Argument 2 for "super" not an instance of argument 1', e) + elif isinstance(instance_type, TypeType) or (isinstance(instance_type, FunctionLike) + and instance_type.is_type_obj()): + # TODO: Check whether this is a valid type object here. + pass + elif not isinstance(instance_type, AnyType): + self.chk.fail('Unsupported argument 2 for "super"', e) + def analyze_super(self, e: SuperExpr, is_lvalue: bool) -> Type: """Type check a super expression.""" if e.info and e.info.bases: diff --git a/mypy/fastparse.py b/mypy/fastparse.py index 47fcde8c20cd4..d9119c39b3611 100644 --- a/mypy/fastparse.py +++ b/mypy/fastparse.py @@ -928,7 +928,7 @@ def visit_Attribute(self, n: ast3.Attribute) -> Union[MemberExpr, SuperExpr]: if (isinstance(n.value, ast3.Call) and isinstance(n.value.func, ast3.Name) and n.value.func.id == 'super'): - return SuperExpr(n.attr) + return SuperExpr(n.attr, self.visit(n.value)) return MemberExpr(self.visit(n.value), n.attr) diff --git a/mypy/fastparse2.py b/mypy/fastparse2.py index 6b2f8d562156f..e6d9f7ca3c001 100644 --- a/mypy/fastparse2.py +++ b/mypy/fastparse2.py @@ -892,7 +892,7 @@ def visit_Attribute(self, n: ast27.Attribute) -> Expression: if (isinstance(n.value, ast27.Call) and isinstance(n.value.func, ast27.Name) and n.value.func.id == 'super'): - return SuperExpr(n.attr) + return SuperExpr(n.attr, self.visit(n.value)) return MemberExpr(self.visit(n.value), n.attr) diff --git a/mypy/nodes.py b/mypy/nodes.py index 8afeb3f063cda..3e334b51e48b6 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -1465,9 +1465,11 @@ class SuperExpr(Expression): name = '' info = None # type: TypeInfo # Type that contains this super expression + call = None # type: CallExpr # The expression super(...) - def __init__(self, name: str) -> None: + def __init__(self, name: str, call: CallExpr) -> None: self.name = name + self.call = call def accept(self, visitor: ExpressionVisitor[T]) -> T: return visitor.visit_super_expr(self) diff --git a/mypy/semanal.py b/mypy/semanal.py index fd9b65d4e50b1..960fa58020395 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -3086,6 +3086,8 @@ def visit_super_expr(self, expr: SuperExpr) -> None: self.fail('"super" used outside class', expr) return expr.info = self.type + for arg in expr.call.args: + arg.accept(self) def visit_tuple_expr(self, expr: TupleExpr) -> None: for item in expr.items: diff --git a/mypy/treetransform.py b/mypy/treetransform.py index 1222dcc072000..08f914ee52335 100644 --- a/mypy/treetransform.py +++ b/mypy/treetransform.py @@ -392,7 +392,7 @@ def visit_reveal_type_expr(self, node: RevealTypeExpr) -> RevealTypeExpr: return RevealTypeExpr(self.expr(node.expr)) def visit_super_expr(self, node: SuperExpr) -> SuperExpr: - new = SuperExpr(node.name) + new = SuperExpr(node.name, cast(CallExpr, self.expr(node.call))) new.info = node.info return new diff --git a/test-data/unit/check-python2.test b/test-data/unit/check-python2.test index 24022f4b15d88..5f1258f3b7693 100644 --- a/test-data/unit/check-python2.test +++ b/test-data/unit/check-python2.test @@ -308,3 +308,12 @@ class A: [out] main:2: error: Invalid type for self, or extra argument type in function annotation main:2: note: (Hint: typically annotations omit the type for self) + +[case testSuper] +class A: + def f(self): # type: () -> None + pass +class B(A): + def g(self): # type: () -> None + super(B, self).f() + super().f() # E: Too few arguments for "super" diff --git a/test-data/unit/check-super.test b/test-data/unit/check-super.test index 8853e3ddace06..0d831a263ee40 100644 --- a/test-data/unit/check-super.test +++ b/test-data/unit/check-super.test @@ -92,10 +92,6 @@ B(1) B(1, 'x') [builtins fixtures/__new__.pyi] -[case testSuperOutsideMethodNoCrash] -class C: - a = super().whatever # E: super() outside of a method is not supported - reveal_type(C.a) # E: Revealed type is 'Any' [out] @@ -115,3 +111,138 @@ class B(A): def foo(self): super(B, self).foo() # Not an error [out] + +[case testSuperWithAny] +class B: + def f(self) -> None: pass +class C(B): + def h(self, x) -> None: + reveal_type(super(x, x).f) # E: Revealed type is 'def ()' + reveal_type(super(C, x).f) # E: Revealed type is 'def ()' + +[case testSuperInUnannotatedMethod] +class C: + def h(self): + super(C, self).xyz + +[case testSuperWithTypeType] +from typing import Type + +class A: + def f(self) -> None: pass + +class B(A): + @classmethod + def g(cls, x) -> None: + reveal_type(super(cls, x).f) # E: Revealed type is 'def ()' +[builtins fixtures/classmethod.pyi] + +[case testSuperWithTypeTypeAsSecondArgument] +class B: + def f(self) -> None: pass + +class C(B): + def __new__(cls) -> 'C': + super(C, cls).f + return C() + +[case testSuperWithGenericSelf] +from typing import TypeVar + +T = TypeVar('T', bound='C') + +class B: + def f(self) -> None: pass + +class C(B): + def f(self) -> None: pass + + def g(self: T) -> T: + reveal_type(super(C, self).f) # E: Revealed type is 'def ()' + return self + +[case testSuperWithTypeVarValues] +from typing import TypeVar, Generic + +T = TypeVar('T', 'C', 'D') +S = TypeVar('S', 'B', 'C') + +class B: + def f(self) -> None: pass + +class C(B): + def f(self) -> None: pass + + def g(self, x: T, y: S) -> None: + super(C, x).f + super(C, y).f # E: Argument 2 for "super" not an instance of argument 1 + +class D(C): pass + + +-- Invalid uses of super() +-- ----------------------- + + +[case testSuperOutsideMethodNoCrash] +class C: + a = super().whatever # E: super() outside of a method is not supported + +[case testSuperWithSingleArgument] +class B: + def f(self) -> None: pass +class C(B): + def __init__(self) -> None: + super(C).f() # E: "super" with a single argument not supported + +[case testSuperWithThreeArguments] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(C, self, 1).f() # E: Too many arguments for "super" + +[case testSuperWithNonPositionalArguments] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(C, x=self).f() # E: "super" only accepts positional arguments + super(**{}).f() # E: "super" only accepts positional arguments + +[case testSuperWithVarArgs] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(*(C, self)).f() # E: Varargs not supported with "super" + +[case testInvalidSuperArg] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(x, y).f # E: Name 'x' is not defined # E: Name 'y' is not defined + +[case testTypeErrorInSuperArg] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(1(), self).f # E: "int" not callable + super(C, ''()).f # E: "str" not callable + +[case testFlippedSuperArgs] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(self, C).f # E: First argument for "super" must be a type object + +[case testInvalidSecondArgumentToSuper] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(C, 1).f # E: Argument 2 for "super" not an instance of argument 1 + super(C, None).f # E: Unsupported argument 2 for "super" From de06c5f5539184daee5dd818365cad44b4df11fc Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 6 Sep 2017 13:54:41 +0100 Subject: [PATCH 2/7] Update tests based on feedback --- test-data/unit/check-super.test | 39 ++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/test-data/unit/check-super.test b/test-data/unit/check-super.test index 0d831a263ee40..694afc0b3ddb5 100644 --- a/test-data/unit/check-super.test +++ b/test-data/unit/check-super.test @@ -125,16 +125,21 @@ class C: def h(self): super(C, self).xyz -[case testSuperWithTypeType] +[case testSuperWithTypeObjects] from typing import Type class A: - def f(self) -> None: pass + def f(self) -> object: pass class B(A): + def f(self) -> int: pass + @classmethod def g(cls, x) -> None: - reveal_type(super(cls, x).f) # E: Revealed type is 'def ()' + reveal_type(super(cls, x).f) # E: Revealed type is 'def () -> builtins.object' + + def h(self, t: Type[B]) -> None: + reveal_type(super(t, self).f) # E: Revealed type is 'def () -> builtins.object' [builtins fixtures/classmethod.pyi] [case testSuperWithTypeTypeAsSecondArgument] @@ -152,17 +157,17 @@ from typing import TypeVar T = TypeVar('T', bound='C') class B: - def f(self) -> None: pass + def f(self) -> float: pass class C(B): - def f(self) -> None: pass + def f(self) -> int: pass def g(self: T) -> T: - reveal_type(super(C, self).f) # E: Revealed type is 'def ()' + reveal_type(super(C, self).f) # E: Revealed type is 'def () -> builtins.float' return self -[case testSuperWithTypeVarValues] -from typing import TypeVar, Generic +[case testSuperWithTypeVarValues1] +from typing import TypeVar T = TypeVar('T', 'C', 'D') S = TypeVar('S', 'B', 'C') @@ -179,6 +184,24 @@ class C(B): class D(C): pass +[case testSuperWithTypeVarValues2] +from typing import TypeVar, Generic + +T = TypeVar('T', 'C', 'D') +S = TypeVar('S', 'B', 'C') + +class B: + def f(self) -> None: pass + +class C(B, Generic[T, S]): + def f(self) -> None: pass + + def g(self, x: T, y: S) -> None: + super(C, x).f + super(C, y).f # E: Argument 2 for "super" not an instance of argument 1 + +class D(C): pass + -- Invalid uses of super() -- ----------------------- From cf4502b76e732556fbd2628d36163ef200850f8c Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 6 Sep 2017 14:17:18 +0100 Subject: [PATCH 3/7] Replace cast with assert --- mypy/treetransform.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/mypy/treetransform.py b/mypy/treetransform.py index 08f914ee52335..1343992149820 100644 --- a/mypy/treetransform.py +++ b/mypy/treetransform.py @@ -392,7 +392,9 @@ def visit_reveal_type_expr(self, node: RevealTypeExpr) -> RevealTypeExpr: return RevealTypeExpr(self.expr(node.expr)) def visit_super_expr(self, node: SuperExpr) -> SuperExpr: - new = SuperExpr(node.name, cast(CallExpr, self.expr(node.call))) + call = self.expr(node.call) + assert isinstance(call, CallExpr) + new = SuperExpr(node.name, call) new.info = node.info return new From ddac7bc6f3c8dfc8eb4bbd1fdb7a46abb9df8ae0 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 6 Sep 2017 14:17:31 +0100 Subject: [PATCH 4/7] Add test case and update confusing error message --- mypy/checkexpr.py | 7 +++++-- test-data/unit/check-super.test | 11 +++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index a1ad6c253c99e..ff359ebebc3f4 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -2145,9 +2145,12 @@ def analyze_super(self, e: SuperExpr, is_lvalue: bool) -> Type: self.chk.fail('super() outside of a method is not supported', e) return AnyType(TypeOfAny.from_error) args = self.chk.scope.top_function().arguments - # An empty args with super() is an error; we need something in declared_self + # super() in a function with empty args is an error; we + # need something in declared_self. if not args: - self.chk.fail('super() requires at least one positional argument', e) + self.chk.fail( + 'super() requires one or more positional arguments in ' + 'enclosing function', e) return AnyType(TypeOfAny.from_error) declared_self = args[0].variable.type return analyze_member_access(name=e.name, typ=fill_typevars(e.info), node=e, diff --git a/test-data/unit/check-super.test b/test-data/unit/check-super.test index 694afc0b3ddb5..b96039afe1434 100644 --- a/test-data/unit/check-super.test +++ b/test-data/unit/check-super.test @@ -269,3 +269,14 @@ class C(B): def h(self) -> None: super(C, 1).f # E: Argument 2 for "super" not an instance of argument 1 super(C, None).f # E: Unsupported argument 2 for "super" + +[case testSuperInMethodWithNoArguments] +class A: + def f(self) -> None: pass + +class B(A): + def g() -> None: # E: Method must have at least one argument + super().f() # E: super() requires one or more positional arguments in enclosing function + def h(self) -> None: + def a() -> None: + super().f() # E: super() requires one or more positional arguments in enclosing function From 7360246e36d8a6bda0379c44f65431c13d279aa8 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 6 Sep 2017 14:32:29 +0100 Subject: [PATCH 5/7] Update based on review --- mypy/checkexpr.py | 12 ++++++++---- mypy/messages.py | 1 + 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index ff359ebebc3f4..e020f30e8eed3 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -2094,9 +2094,12 @@ def check_super_arguments(self, e: SuperExpr) -> None: if isinstance(item, AnyType): # Could be anything. return + if isinstance(item, TupleType): + item = item.fallback # Handle named tuples and other Tuple[...] subclasses. if not isinstance(item, Instance): - # A complicated type object type. Assume it's fine. + # A complicated type object type. Too tricky, give up. # TODO: Do something more clever here. + self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e) return type_info = item.type elif isinstance(type_obj_type, AnyType): @@ -2111,10 +2114,11 @@ def check_super_arguments(self, e: SuperExpr) -> None: instance_type = instance_type.upper_bound if not isinstance(instance_type, (Instance, TupleType)): # Too tricky, give up. - self.chk.fail('Unsupported argument 2 for "super"', e) + # TODO: Do something more clever here. + self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e) return if isinstance(instance_type, TupleType): - # Needed for named tuples and classes derived from Tuple[...]. + # Needed for named tuples and other Tuple[...] subclasses. instance_type = instance_type.fallback if type_info not in instance_type.type.mro: self.chk.fail('Argument 2 for "super" not an instance of argument 1', e) @@ -2123,7 +2127,7 @@ def check_super_arguments(self, e: SuperExpr) -> None: # TODO: Check whether this is a valid type object here. pass elif not isinstance(instance_type, AnyType): - self.chk.fail('Unsupported argument 2 for "super"', e) + self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e) def analyze_super(self, e: SuperExpr, is_lvalue: bool) -> Type: """Type check a super expression.""" diff --git a/mypy/messages.py b/mypy/messages.py index 2cfb9e33fa429..fa1002f46f400 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -99,6 +99,7 @@ BARE_GENERIC = 'Missing type parameters for generic type' IMPLICIT_GENERIC_ANY_BUILTIN = 'Implicit generic "Any". Use \'{}\' and specify generic parameters' INCOMPATIBLE_TYPEVAR_VALUE = 'Value of type variable "{}" of {} cannot be {}' +UNSUPPORTED_ARGUMENT_2_FOR_SUPER = 'Unsupported argument 2 for "super"' ARG_CONSTRUCTOR_NAMES = { ARG_POS: "Arg", From 9c2a32448f3546e08338b0aced0e92fcbeaa2149 Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 6 Sep 2017 14:54:09 +0100 Subject: [PATCH 6/7] Update error message --- mypy/checkexpr.py | 2 +- mypy/messages.py | 9 +++++++++ test-data/unit/check-super.test | 9 ++++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index e020f30e8eed3..fd85c24e3ec94 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -2105,7 +2105,7 @@ def check_super_arguments(self, e: SuperExpr) -> None: elif isinstance(type_obj_type, AnyType): return else: - self.chk.fail('First argument for "super" must be a type object', e) + self.msg.first_argument_for_super_must_be_type(type_obj_type, e) return if isinstance(instance_type, (Instance, TupleType, TypeVarType)): diff --git a/mypy/messages.py b/mypy/messages.py index fa1002f46f400..51fa0c7d9955e 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -820,6 +820,15 @@ def invalid_keyword_var_arg(self, typ: Type, is_mapping: bool, context: Context) def undefined_in_superclass(self, member: str, context: Context) -> None: self.fail('"{}" undefined in superclass'.format(member), context) + def first_argument_for_super_must_be_type(self, actual: Type, context: Context) -> None: + if isinstance(actual, Instance): + # Don't include type of instance, because it can look confusingly like a type + # object. + type_str = 'a non-type instance' + else: + type_str = self.format(actual) + self.fail('Argument 1 for "super" must be a type object; got {}'.format(type_str), context) + def too_few_string_formatting_arguments(self, context: Context) -> None: self.fail('Not enough arguments for format string', context) diff --git a/test-data/unit/check-super.test b/test-data/unit/check-super.test index b96039afe1434..217de2b761821 100644 --- a/test-data/unit/check-super.test +++ b/test-data/unit/check-super.test @@ -260,7 +260,14 @@ class B: def f(self) -> None: pass class C(B): def h(self) -> None: - super(self, C).f # E: First argument for "super" must be a type object + super(self, C).f # E: Argument 1 for "super" must be a type object; got a non-type instance + +[case testInvalidFirstSuperArg] +class B: + def f(self) -> None: pass +class C(B): + def h(self) -> None: + super(None, C).f # E: Argument 1 for "super" must be a type object; got "None" [case testInvalidSecondArgumentToSuper] class B: From 66e440503a8f3928da79639f87636045859a523f Mon Sep 17 00:00:00 2001 From: Jukka Lehtosalo Date: Wed, 6 Sep 2017 16:51:52 +0100 Subject: [PATCH 7/7] Fix invalid error message --- mypy/checkexpr.py | 2 +- test-data/unit/check-super.test | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index fd85c24e3ec94..eb1f315eb6ba7 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -2099,7 +2099,7 @@ def check_super_arguments(self, e: SuperExpr) -> None: if not isinstance(item, Instance): # A complicated type object type. Too tricky, give up. # TODO: Do something more clever here. - self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e) + self.chk.fail('Unsupported argument 1 for "super"', e) return type_info = item.type elif isinstance(type_obj_type, AnyType): diff --git a/test-data/unit/check-super.test b/test-data/unit/check-super.test index 217de2b761821..73ac92a17d91b 100644 --- a/test-data/unit/check-super.test +++ b/test-data/unit/check-super.test @@ -287,3 +287,13 @@ class B(A): def h(self) -> None: def a() -> None: super().f() # E: super() requires one or more positional arguments in enclosing function + +[case testSuperWithUnsupportedTypeObject] +from typing import Type + +class A: + def f(self) -> int: pass + +class B(A): + def h(self, t: Type[None]) -> None: + super(t, self).f # E: Unsupported argument 1 for "super"