From af4e8fd24d65c1cca82b4c647a4a367409ff6fe6 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Fri, 29 Jul 2022 13:56:03 +0200 Subject: [PATCH 01/11] Treat methods with empty bodies in Protocols as abstract Closes #8005 Closes #8926 Methods in Protocols are considered abstract if they have an empty function body, have a return type that is not compatible with `None`, and are not in a stub file. --- mypy/checker.py | 47 +-------- mypy/semanal.py | 83 +++++++++++++++- mypy/semanal_classprop.py | 6 +- test-data/unit/check-protocols.test | 149 ++++++++++++++++++++++++++++ 4 files changed, 237 insertions(+), 48 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 00e104d8bcf3..678231cf2517 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -115,7 +115,6 @@ ReturnStmt, StarExpr, Statement, - StrExpr, SymbolTable, SymbolTableNode, TempNode, @@ -134,7 +133,7 @@ from mypy.plugin import CheckerPluginInterface, Plugin from mypy.sametypes import is_same_type from mypy.scope import Scope -from mypy.semanal import refers_to_fullname, set_callable_name +from mypy.semanal import is_trivial_body, refers_to_fullname, set_callable_name from mypy.semanal_enum import ENUM_BASES, ENUM_SPECIAL_PROPS from mypy.sharedparse import BINARY_MAGIC_METHODS from mypy.state import state @@ -1171,7 +1170,7 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: Optional[str]) item.arguments[i].variable.type = arg_type # Type check initialization expressions. - body_is_trivial = self.is_trivial_body(defn.body) + body_is_trivial = is_trivial_body(defn.body) self.check_default_args(item, body_is_trivial) # Type check body in a new scope. @@ -1339,48 +1338,6 @@ def check___new___signature(self, fdef: FuncDef, typ: CallableType) -> None: "but must return a subtype of", ) - def is_trivial_body(self, block: Block) -> bool: - """Returns 'true' if the given body is "trivial" -- if it contains just a "pass", - "..." (ellipsis), or "raise NotImplementedError()". A trivial body may also - start with a statement containing just a string (e.g. a docstring). - - Note: functions that raise other kinds of exceptions do not count as - "trivial". We use this function to help us determine when it's ok to - relax certain checks on body, but functions that raise arbitrary exceptions - are more likely to do non-trivial work. For example: - - def halt(self, reason: str = ...) -> NoReturn: - raise MyCustomError("Fatal error: " + reason, self.line, self.context) - - A function that raises just NotImplementedError is much less likely to be - this complex. - """ - body = block.body - - # Skip a docstring - if body and isinstance(body[0], ExpressionStmt) and isinstance(body[0].expr, StrExpr): - body = block.body[1:] - - if len(body) == 0: - # There's only a docstring (or no body at all). - return True - elif len(body) > 1: - return False - - stmt = body[0] - - if isinstance(stmt, RaiseStmt): - expr = stmt.expr - if expr is None: - return False - if isinstance(expr, CallExpr): - expr = expr.callee - - return isinstance(expr, NameExpr) and expr.fullname == "builtins.NotImplementedError" - - return isinstance(stmt, PassStmt) or ( - isinstance(stmt, ExpressionStmt) and isinstance(stmt.expr, EllipsisExpr) - ) def check_reverse_op_method( self, defn: FuncItem, reverse_type: CallableType, reverse_name: str, context: Context diff --git a/mypy/semanal.py b/mypy/semanal.py index 928b084d981b..d55e8a2af7c0 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -145,6 +145,7 @@ OverloadedFuncDef, OverloadPart, ParamSpecExpr, + PassStmt, PlaceholderNode, PromoteExpr, RaiseStmt, @@ -837,6 +838,16 @@ def analyze_func_def(self, defn: FuncDef) -> None: self.analyze_arg_initializers(defn) self.analyze_function_body(defn) + + # Mark protocol methods with empty bodies and None-incompatible return types as abstract. + if self.is_class_scope() and defn.type is not None: + assert self.type is not None and isinstance(defn.type, CallableType) + if (not self.is_stub_file and self.type.is_protocol and + (not isinstance(self.scope.function, OverloadedFuncDef) + or defn.is_property) and + not can_be_none(defn.type.ret_type) and is_trivial_body(defn.body)): + defn.is_abstract = True + if ( defn.is_coroutine and isinstance(defn.type, CallableType) @@ -975,6 +986,21 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: # We know this is an overload def. Infer properties and perform some checks. self.process_final_in_overload(defn) self.process_static_or_class_method_in_overload(defn) + if defn.impl: + self.process_overload_impl(defn) + + def process_overload_impl(self, defn: OverloadedFuncDef) -> None: + """Set flags for an overload implementation. + + Currently, this checks for a trivial body in protocols classes, + where it makes the method implicitly abstract. + """ + assert defn.impl is not None + impl = defn.impl if isinstance(defn.impl, FuncDef) else defn.impl.func + if is_trivial_body(impl.body) and self.is_class_scope() and not self.is_stub_file: + assert self.type is not None + if self.type.is_protocol: + impl.is_abstract = True def analyze_overload_sigs_and_impl( self, defn: OverloadedFuncDef @@ -1052,7 +1078,8 @@ def handle_missing_overload_implementation(self, defn: OverloadedFuncDef) -> Non """Generate error about missing overload implementation (only if needed).""" if not self.is_stub_file: if self.type and self.type.is_protocol and not self.is_func_scope(): - # An overloaded protocol method doesn't need an implementation. + # An overloaded protocol method doesn't need an implementation, + # but if it doesn't have one, then it is considered implicitly abstract. for item in defn.items: if isinstance(item, Decorator): item.func.is_abstract = True @@ -6033,3 +6060,57 @@ def is_same_symbol(a: Optional[SymbolNode], b: Optional[SymbolNode]) -> bool: or (isinstance(a, PlaceholderNode) and isinstance(b, PlaceholderNode)) or is_same_var_from_getattr(a, b) ) + + +def is_trivial_body(block: Block) -> bool: + """Returns 'true' if the given body is "trivial" -- if it contains just a "pass", + "..." (ellipsis), or "raise NotImplementedError()". A trivial body may also + start with a statement containing just a string (e.g. a docstring). + + Note: functions that raise other kinds of exceptions do not count as + "trivial". We use this function to help us determine when it's ok to + relax certain checks on body, but functions that raise arbitrary exceptions + are more likely to do non-trivial work. For example: + + def halt(self, reason: str = ...) -> NoReturn: + raise MyCustomError("Fatal error: " + reason, self.line, self.context) + + A function that raises just NotImplementedError is much less likely to be + this complex. + """ + body = block.body + + # Skip a docstring + if body and isinstance(body[0], ExpressionStmt) and isinstance(body[0].expr, StrExpr): + body = block.body[1:] + + if len(body) == 0: + # There's only a docstring (or no body at all). + return True + elif len(body) > 1: + return False + + stmt = body[0] + + if isinstance(stmt, RaiseStmt): + expr = stmt.expr + if expr is None: + return False + if isinstance(expr, CallExpr): + expr = expr.callee + + return isinstance(expr, NameExpr) and expr.fullname == "builtins.NotImplementedError" + + return isinstance(stmt, PassStmt) or ( + isinstance(stmt, ExpressionStmt) and isinstance(stmt.expr, EllipsisExpr) + ) + + +def can_be_none(t: Type) -> bool: + """Can a variable of the given type be None?""" + t = get_proper_type(t) + return ( + isinstance(t, NoneType) or + isinstance(t, AnyType) or + (isinstance(t, UnionType) and any(can_be_none(ut) for ut in t.items)) + ) diff --git a/mypy/semanal_classprop.py b/mypy/semanal_classprop.py index 08ac6f6951c4..0bd0becf0813 100644 --- a/mypy/semanal_classprop.py +++ b/mypy/semanal_classprop.py @@ -11,6 +11,7 @@ from mypy.nodes import ( CallExpr, Decorator, + FuncDef, Node, OverloadedFuncDef, PromoteExpr, @@ -69,8 +70,9 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E else: func = node if isinstance(func, Decorator): - fdef = func.func - if fdef.is_abstract and name not in concrete: + func = func.func + if isinstance(func, FuncDef): + if func.is_abstract and name not in concrete: typ.is_abstract = True abstract.append(name) if base is typ: diff --git a/test-data/unit/check-protocols.test b/test-data/unit/check-protocols.test index 4c5a0b44d714..80c10e0196c8 100644 --- a/test-data/unit/check-protocols.test +++ b/test-data/unit/check-protocols.test @@ -2924,3 +2924,152 @@ class C: def round(number: SupportsRound[_T], ndigits: int) -> _T: ... round(C(), 1) + +[case testEmptyBodyImplicitlyAbstractProtocol] +from typing import Protocol, overload, Union + +class P1(Protocol): + def meth(self) -> int: ... +class B1(P1): ... +class C1(P1): + def meth(self) -> int: + return 0 +B1() # E: Cannot instantiate abstract class "B1" with abstract attribute "meth" +C1() + +class P2(Protocol): + @classmethod + def meth(cls) -> int: ... +class B2(P2): ... +class C2(P2): + @classmethod + def meth(cls) -> int: + return 0 +B2() # E: Cannot instantiate abstract class "B2" with abstract attribute "meth" +C2() + +class P3(Protocol): + @overload + def meth(self, x: int) -> int: ... + @overload + def meth(self, x: str) -> str: ... +class B3(P3): ... +class C3(P3): + @overload + def meth(self, x: int) -> int: ... + @overload + def meth(self, x: str) -> str: ... + def meth(self, x: Union[int, str]) -> Union[int, str]: + return 0 +B3() # E: Cannot instantiate abstract class "B3" with abstract attribute "meth" +C3() +[builtins fixtures/classmethod.pyi] + +[case testEmptyBodyImplicitlyAbstractProtocolProperty] +from typing import Protocol + +class P1(Protocol): + @property + def attr(self) -> int: ... +class B1(P1): ... +class C1(P1): + @property + def attr(self) -> int: + return 0 +B1() # E: Cannot instantiate abstract class "B1" with abstract attribute "attr" +C1() + +class P2(Protocol): + @property + def attr(self) -> int: ... + @attr.setter + def attr(self, value: int) -> None: ... +class B2(P2): ... +class C2(P2): + @property + def attr(self) -> int: return 0 + @attr.setter + def attr(self, value: int) -> None: pass +B2() # E: Cannot instantiate abstract class "B2" with abstract attribute "attr" +C2() +[builtins fixtures/property.pyi] + +[case testEmptyBodyImplicitlyAbstractProtocolStub] +from stub import P1, P2, P3, P4 + +class B1(P1): ... +class B2(P2): ... +class B3(P3): ... +class B4(P4): ... + +B1() +B2() +B3() +B4() # E: Cannot instantiate abstract class "B4" with abstract attribute "meth" + +[file stub.pyi] +from typing import Protocol, overload, Union +from abc import abstractmethod + +class P1(Protocol): + def meth(self) -> int: ... + +class P2(Protocol): + @classmethod + def meth(cls) -> int: ... + +class P3(Protocol): + @overload + def meth(self, x: int) -> int: ... + @overload + def meth(self, x: str) -> str: ... + +class P4(Protocol): + @abstractmethod + def meth(self) -> int: ... +[builtins fixtures/classmethod.pyi] + +[case testEmptyBodyVariationsImplicitlyAbstractProtocol] +from typing import Protocol + +class WithPass(Protocol): + def meth(self) -> int: + pass +class A(WithPass): ... +A() # E: Cannot instantiate abstract class "A" with abstract attribute "meth" + +class WithEllipses(Protocol): + def meth(self) -> int: ... +class B(WithEllipses): ... +B() # E: Cannot instantiate abstract class "B" with abstract attribute "meth" + +class WithDocstring(Protocol): + def meth(self) -> int: + """Docstring for meth. + + This is meth.""" +class C(WithDocstring): ... +C() # E: Cannot instantiate abstract class "C" with abstract attribute "meth" + +class WithRaise(Protocol): + def meth(self) -> int: + """Docstring for meth.""" + raise NotImplementedError +class D(WithRaise): ... +D() # E: Cannot instantiate abstract class "D" with abstract attribute "meth" +[builtins fixtures/exception.pyi] + +[case testEmptyBodyNonAbstractProtocol] +from typing import Any, Optional, Protocol, Union + +class NotAbstract(Protocol): + def f(self) -> None: ... + def g(self) -> Any: ... + def h(self, x: int): ... + def j(self): ... + def k(self, x): ... + def l(self) -> Optional[int]: ... + def m(self) -> Union[str, None]: ... + +class A(NotAbstract): ... +A() From aaaf4c45ed6c3750b0ef2bb5cf96c1319cf25af5 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Fri, 29 Jul 2022 13:56:14 +0200 Subject: [PATCH 02/11] Address code review feedback --- mypy/semanal.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index d55e8a2af7c0..d37e5cd3fbfc 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -986,8 +986,7 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: # We know this is an overload def. Infer properties and perform some checks. self.process_final_in_overload(defn) self.process_static_or_class_method_in_overload(defn) - if defn.impl: - self.process_overload_impl(defn) + self.process_overload_impl(defn) def process_overload_impl(self, defn: OverloadedFuncDef) -> None: """Set flags for an overload implementation. @@ -995,7 +994,8 @@ def process_overload_impl(self, defn: OverloadedFuncDef) -> None: Currently, this checks for a trivial body in protocols classes, where it makes the method implicitly abstract. """ - assert defn.impl is not None + if defn.impl is None: + return impl = defn.impl if isinstance(defn.impl, FuncDef) else defn.impl.func if is_trivial_body(impl.body) and self.is_class_scope() and not self.is_stub_file: assert self.type is not None From 614c9cc16bbf25c19d44ce1e58ba84a14c0b6cf2 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Fri, 29 Jul 2022 13:54:25 +0200 Subject: [PATCH 03/11] Format with black --- mypy/checker.py | 1 - mypy/semanal.py | 17 ++++++++++------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 678231cf2517..7ae4132418c1 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1338,7 +1338,6 @@ def check___new___signature(self, fdef: FuncDef, typ: CallableType) -> None: "but must return a subtype of", ) - def check_reverse_op_method( self, defn: FuncItem, reverse_type: CallableType, reverse_name: str, context: Context ) -> None: diff --git a/mypy/semanal.py b/mypy/semanal.py index d37e5cd3fbfc..10f0384cda89 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -842,10 +842,13 @@ def analyze_func_def(self, defn: FuncDef) -> None: # Mark protocol methods with empty bodies and None-incompatible return types as abstract. if self.is_class_scope() and defn.type is not None: assert self.type is not None and isinstance(defn.type, CallableType) - if (not self.is_stub_file and self.type.is_protocol and - (not isinstance(self.scope.function, OverloadedFuncDef) - or defn.is_property) and - not can_be_none(defn.type.ret_type) and is_trivial_body(defn.body)): + if ( + not self.is_stub_file + and self.type.is_protocol + and (not isinstance(self.scope.function, OverloadedFuncDef) or defn.is_property) + and not can_be_none(defn.type.ret_type) + and is_trivial_body(defn.body) + ): defn.is_abstract = True if ( @@ -6110,7 +6113,7 @@ def can_be_none(t: Type) -> bool: """Can a variable of the given type be None?""" t = get_proper_type(t) return ( - isinstance(t, NoneType) or - isinstance(t, AnyType) or - (isinstance(t, UnionType) and any(can_be_none(ut) for ut in t.items)) + isinstance(t, NoneType) + or isinstance(t, AnyType) + or (isinstance(t, UnionType) and any(can_be_none(ut) for ut in t.items)) ) From 5714b56cc2ef9a146632beb79c8e0d8561108ce3 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Fri, 29 Jul 2022 16:03:42 +0200 Subject: [PATCH 04/11] Don't special-case none-return --- mypy/semanal.py | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 10f0384cda89..8090c160820f 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -839,14 +839,14 @@ def analyze_func_def(self, defn: FuncDef) -> None: self.analyze_arg_initializers(defn) self.analyze_function_body(defn) - # Mark protocol methods with empty bodies and None-incompatible return types as abstract. if self.is_class_scope() and defn.type is not None: - assert self.type is not None and isinstance(defn.type, CallableType) + assert self.type is not None + # Mark protocol methods with empty bodies as implicitly abstract. + # This makes explicit protocol subclassing type-safe. if ( - not self.is_stub_file - and self.type.is_protocol + self.type.is_protocol + and not self.is_stub_file # Bodies in stub files are always empty. and (not isinstance(self.scope.function, OverloadedFuncDef) or defn.is_property) - and not can_be_none(defn.type.ret_type) and is_trivial_body(defn.body) ): defn.is_abstract = True @@ -6107,13 +6107,3 @@ def halt(self, reason: str = ...) -> NoReturn: return isinstance(stmt, PassStmt) or ( isinstance(stmt, ExpressionStmt) and isinstance(stmt.expr, EllipsisExpr) ) - - -def can_be_none(t: Type) -> bool: - """Can a variable of the given type be None?""" - t = get_proper_type(t) - return ( - isinstance(t, NoneType) - or isinstance(t, AnyType) - or (isinstance(t, UnionType) and any(can_be_none(ut) for ut in t.items)) - ) From 8ad2ee16a0cce19ccef1bd0bc335be51f5e81307 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Fri, 29 Jul 2022 17:08:42 +0200 Subject: [PATCH 05/11] Add a nice note to the error message --- mypy/checker.py | 2 +- mypy/checkexpr.py | 36 +++++++++++++++++++++++++++++++++++- mypy/messages.py | 11 ++++++++++- mypy/nodes.py | 17 ++++++++++++----- mypy/semanal.py | 18 ++++++++++-------- mypy/semanal_classprop.py | 15 ++++++++++----- mypy/strconv.py | 2 +- mypy/stubgen.py | 2 +- mypy/treetransform.py | 2 +- 9 files changed, 81 insertions(+), 24 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 7ae4132418c1..1b69d7a96163 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -617,7 +617,7 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: for fdef in defn.items: assert isinstance(fdef, Decorator) self.check_func_item(fdef.func, name=fdef.func.name) - if fdef.func.is_abstract: + if fdef.func.abstract_status: num_abstract += 1 if num_abstract not in (0, len(defn.items)): self.fail(message_registry.INCONSISTENT_ABSTRACT_OVERLOAD, defn) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index 2b947cdc8e32..f74eb0d02a6d 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -27,6 +27,7 @@ ARG_POS, ARG_STAR, ARG_STAR2, + IS_ABSTRACT, LITERAL_TYPE, REVEAL_TYPE, ArgKind, @@ -1242,8 +1243,16 @@ def check_callable_call( and not callee.type_object().fallback_to_any ): type = callee.type_object() + # Determine whether the abstract attributes are functions with + # None-compatible return types and whether they were defined in a protocol. + is_none_ret_and_prot: Dict[str, bool] = {} + for attr_name, abstract_status in type.abstract_attributes: + if abstract_status == IS_ABSTRACT: + is_none_ret_and_prot[attr_name] = False + continue + is_none_ret_and_prot[attr_name] = self._has_none_compat_return(type, attr_name) self.msg.cannot_instantiate_abstract_class( - callee.type_object().name, type.abstract_attributes, context + callee.type_object().name, is_none_ret_and_prot, context ) elif ( callee.is_type_obj() @@ -1335,6 +1344,31 @@ def check_callable_call( callee = callee.copy_modified(ret_type=new_ret_type) return callee.ret_type, callee + def _has_none_compat_return(self, type: TypeInfo, attr_name: str) -> bool: + """Determine whether the attribute is a function with None-compatible return type. + + Additionally, check whether the function was defined in a protocol. + """ + for base in type.mro: + symnode = base.names.get(attr_name) + if symnode is None or not base.is_protocol: + continue + node = symnode.node + if isinstance(node, OverloadedFuncDef): + if node.impl is not None: + assert isinstance(node.impl.type, CallableType) + return is_subtype(NoneType(), node.impl.type.ret_type) + for overload in node.items: + assert isinstance(overload.type, CallableType) + if is_subtype(NoneType(), overload.type.ret_type): + return True + else: + return False + if isinstance(node, FuncDef) and node.type is not None: + assert isinstance(node.type, CallableType) + return is_subtype(NoneType(), node.type.ret_type) + return False + def analyze_type_type_callee(self, item: ProperType, context: Context) -> Type: """Analyze the callee X in X(...) where X is Type[item]. diff --git a/mypy/messages.py b/mypy/messages.py index 3a38f91253a4..19a9257d6c7d 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1321,7 +1321,7 @@ def incompatible_conditional_function_def(self, defn: FuncDef) -> None: self.fail("All conditional function variants must have identical " "signatures", defn) def cannot_instantiate_abstract_class( - self, class_name: str, abstract_attributes: List[str], context: Context + self, class_name: str, abstract_attributes: Dict[str, bool], context: Context ) -> None: attrs = format_string_list([f'"{a}"' for a in abstract_attributes]) self.fail( @@ -1330,6 +1330,15 @@ def cannot_instantiate_abstract_class( context, code=codes.ABSTRACT, ) + for a, is_none_ret_and_prot in abstract_attributes.items(): + if not is_none_ret_and_prot: + continue + self.note( + '"{}" was implicitly marked abstract because it has an empty function body. ' + "If it is not meant to be abstract, explicitly return None.".format(a), + context, + code=codes.OVERRIDE, + ) def base_class_definitions_incompatible( self, name: str, base1: TypeInfo, base2: TypeInfo, context: Context diff --git a/mypy/nodes.py b/mypy/nodes.py index 690dca595452..f74a5f5da227 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -758,7 +758,12 @@ def is_dynamic(self) -> bool: return self.type is None -FUNCDEF_FLAGS: Final = FUNCITEM_FLAGS + ["is_decorated", "is_conditional", "is_abstract"] +FUNCDEF_FLAGS: Final = FUNCITEM_FLAGS + ["is_decorated", "is_conditional"] + +# Abstract status of a function +NOT_ABSTRACT: Final = 0 +IS_ABSTRACT: Final = 1 +IMPLICITLY_ABSTRACT: Final = 2 class FuncDef(FuncItem, SymbolNode, Statement): @@ -771,7 +776,7 @@ class FuncDef(FuncItem, SymbolNode, Statement): "_name", "is_decorated", "is_conditional", - "is_abstract", + "abstract_status", "original_def", "deco_line", ) @@ -788,7 +793,7 @@ def __init__( self._name = name self.is_decorated = False self.is_conditional = False # Defined conditionally (within block)? - self.is_abstract = False + self.abstract_status = NOT_ABSTRACT self.is_final = False # Original conditional definition self.original_def: Union[None, FuncDef, Var, Decorator] = None @@ -817,6 +822,7 @@ def serialize(self) -> JsonDict: "arg_kinds": [int(x.value) for x in self.arg_kinds], "type": None if self.type is None else self.type.serialize(), "flags": get_flags(self, FUNCDEF_FLAGS), + "abstract_status": self.abstract_status, # TODO: Do we need expanded, original_def? } @@ -839,6 +845,7 @@ def deserialize(cls, data: JsonDict) -> "FuncDef": # NOTE: ret.info is set in the fixup phase. ret.arg_names = data["arg_names"] ret.arg_kinds = [ArgKind(x) for x in data["arg_kinds"]] + ret.abstract_status = data["abstract_status"] # Leave these uninitialized so that future uses will trigger an error del ret.arguments del ret.max_pos @@ -2674,7 +2681,7 @@ class is generic then it will be a type constructor of higher kind. is_abstract: bool # Does the class have any abstract attributes? is_protocol: bool # Is this a protocol class? runtime_protocol: bool # Does this protocol support isinstance checks? - abstract_attributes: List[str] + abstract_attributes: List[Tuple[str, int]] deletable_attributes: List[str] # Used by mypyc only # Does this type have concrete `__slots__` defined? # If class does not have `__slots__` defined then it is `None`, @@ -3034,7 +3041,7 @@ def deserialize(cls, data: JsonDict) -> "TypeInfo": ti = TypeInfo(names, defn, module_name) ti._fullname = data["fullname"] # TODO: Is there a reason to reconstruct ti.subtypes? - ti.abstract_attributes = data["abstract_attributes"] + ti.abstract_attributes = [tuple(attr) for attr in data["abstract_attributes"]] ti.type_vars = data["type_vars"] ti.has_param_spec_type = data["has_param_spec_type"] ti.bases = [mypy.types.Instance.deserialize(b) for b in data["bases"]] diff --git a/mypy/semanal.py b/mypy/semanal.py index 8090c160820f..963f74d9f0a0 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -84,7 +84,9 @@ CONTRAVARIANT, COVARIANT, GDEF, + IMPLICITLY_ABSTRACT, INVARIANT, + IS_ABSTRACT, LDEF, MDEF, REVEAL_LOCALS, @@ -849,7 +851,7 @@ def analyze_func_def(self, defn: FuncDef) -> None: and (not isinstance(self.scope.function, OverloadedFuncDef) or defn.is_property) and is_trivial_body(defn.body) ): - defn.is_abstract = True + defn.abstract_status = IMPLICITLY_ABSTRACT if ( defn.is_coroutine @@ -1003,7 +1005,7 @@ def process_overload_impl(self, defn: OverloadedFuncDef) -> None: if is_trivial_body(impl.body) and self.is_class_scope() and not self.is_stub_file: assert self.type is not None if self.type.is_protocol: - impl.is_abstract = True + impl.abstract_status = IMPLICITLY_ABSTRACT def analyze_overload_sigs_and_impl( self, defn: OverloadedFuncDef @@ -1085,9 +1087,9 @@ def handle_missing_overload_implementation(self, defn: OverloadedFuncDef) -> Non # but if it doesn't have one, then it is considered implicitly abstract. for item in defn.items: if isinstance(item, Decorator): - item.func.is_abstract = True + item.func.abstract_status = IMPLICITLY_ABSTRACT else: - item.is_abstract = True + item.abstract_status = IMPLICITLY_ABSTRACT else: self.fail( "An overloaded function outside a stub file must have an implementation", @@ -1163,7 +1165,7 @@ def analyze_property_with_multi_part_definition(self, defn: OverloadedFuncDef) - # The first item represents the entire property. first_item.var.is_settable_property = True # Get abstractness from the original definition. - item.func.is_abstract = first_item.func.is_abstract + item.func.abstract_status = first_item.func.abstract_status else: self.fail("Decorated property not supported", item) item.func.accept(self) @@ -1250,7 +1252,7 @@ def visit_decorator(self, dec: Decorator) -> None: # A bunch of decorators are special cased here. if refers_to_fullname(d, "abc.abstractmethod"): removed.append(i) - dec.func.is_abstract = True + dec.func.abstract_status = IS_ABSTRACT self.check_decorated_function_is_method("abstractmethod", dec) elif refers_to_fullname(d, ("asyncio.coroutines.coroutine", "types.coroutine")): removed.append(i) @@ -1272,7 +1274,7 @@ def visit_decorator(self, dec: Decorator) -> None: dec.func.is_property = True dec.var.is_property = True if refers_to_fullname(d, "abc.abstractproperty"): - dec.func.is_abstract = True + dec.func.abstract_status = IS_ABSTRACT elif refers_to_fullname(d, "functools.cached_property"): dec.var.is_settable_property = True self.check_decorated_function_is_method("property", dec) @@ -1301,7 +1303,7 @@ def visit_decorator(self, dec: Decorator) -> None: dec.func.accept(self) if dec.decorators and dec.var.is_property: self.fail("Decorated property not supported", dec) - if dec.func.is_abstract and dec.func.is_final: + if dec.func.abstract_status and dec.func.is_final: self.fail(f"Method {dec.func.name} is both abstract and final", dec) def check_decorated_function_is_method(self, decorator: str, context: Context) -> None: diff --git a/mypy/semanal_classprop.py b/mypy/semanal_classprop.py index 0bd0becf0813..6e48e7410b92 100644 --- a/mypy/semanal_classprop.py +++ b/mypy/semanal_classprop.py @@ -3,12 +3,14 @@ These happen after semantic analysis and before type checking. """ -from typing import List, Optional, Set +from typing import List, Optional, Set, Tuple from typing_extensions import Final from mypy.errors import Errors from mypy.nodes import ( + IMPLICITLY_ABSTRACT, + IS_ABSTRACT, CallExpr, Decorator, FuncDef, @@ -47,7 +49,7 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E if typ.typeddict_type: return # TypedDict can't be abstract concrete: Set[str] = set() - abstract: List[str] = [] + abstract: List[Tuple[str, int]] = [] abstract_in_this_class: List[str] = [] if typ.is_newtype: # Special case: NewTypes are considered as always non-abstract, so they can be used as: @@ -72,15 +74,18 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E if isinstance(func, Decorator): func = func.func if isinstance(func, FuncDef): - if func.is_abstract and name not in concrete: + if ( + func.abstract_status in (IS_ABSTRACT, IMPLICITLY_ABSTRACT) + and name not in concrete + ): typ.is_abstract = True - abstract.append(name) + abstract.append((name, func.abstract_status)) if base is typ: abstract_in_this_class.append(name) elif isinstance(node, Var): if node.is_abstract_var and name not in concrete: typ.is_abstract = True - abstract.append(name) + abstract.append((name, IS_ABSTRACT)) if base is typ: abstract_in_this_class.append(name) concrete.add(name) diff --git a/mypy/strconv.py b/mypy/strconv.py index dec8f074da2c..30728ed3c6f2 100644 --- a/mypy/strconv.py +++ b/mypy/strconv.py @@ -133,7 +133,7 @@ def visit_func_def(self, o: "mypy.nodes.FuncDef") -> str: arg_kinds = {arg.kind for arg in o.arguments} if len(arg_kinds & {mypy.nodes.ARG_NAMED, mypy.nodes.ARG_NAMED_OPT}) > 0: a.insert(1, f"MaxPos({o.max_pos})") - if o.is_abstract: + if o.abstract_status: a.insert(-1, "Abstract") if o.is_static: a.insert(-1, "Static") diff --git a/mypy/stubgen.py b/mypy/stubgen.py index ee51efa00fb7..4a0717024e33 100755 --- a/mypy/stubgen.py +++ b/mypy/stubgen.py @@ -723,7 +723,7 @@ def visit_func_def( retname = None # implicit Any else: retname = self.print_annotation(o.unanalyzed_type.ret_type) - elif isinstance(o, FuncDef) and (o.is_abstract or o.name in METHODS_WITH_RETURN_VALUE): + elif isinstance(o, FuncDef) and (o.abstract_status or o.name in METHODS_WITH_RETURN_VALUE): # Always assume abstract methods return Any unless explicitly annotated. Also # some dunder methods should not have a None return type. retname = None # implicit Any diff --git a/mypy/treetransform.py b/mypy/treetransform.py index 0bf2213bf406..c9270223f6de 100644 --- a/mypy/treetransform.py +++ b/mypy/treetransform.py @@ -186,7 +186,7 @@ def visit_func_def(self, node: FuncDef) -> FuncDef: new._fullname = node._fullname new.is_decorated = node.is_decorated new.is_conditional = node.is_conditional - new.is_abstract = node.is_abstract + new.abstract_status = node.abstract_status new.is_static = node.is_static new.is_class = node.is_class new.is_property = node.is_property From 84a2f88dd5c76d0de489b1a9866eff2d10860d16 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Sat, 30 Jul 2022 13:18:43 +0100 Subject: [PATCH 06/11] Fix most errors --- mypy/checkexpr.py | 62 ++++++++++++++++---------- mypy/nodes.py | 2 +- mypy/semanal.py | 2 +- mypy/semanal_classprop.py | 4 +- test-data/unit/check-protocols.test | 69 +++++++++++++++++++++++------ 5 files changed, 98 insertions(+), 41 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index f74eb0d02a6d..800b782f4d3e 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -97,6 +97,7 @@ ) from mypy.sametypes import is_same_type from mypy.semanal_enum import ENUM_BASES +from mypy.state import state from mypy.subtypes import is_equivalent, is_subtype, non_method_protocol_members from mypy.traverser import has_await_expression from mypy.typeanal import ( @@ -1236,6 +1237,16 @@ def check_callable_call( return callee.ret_type, callee if ( + callee.is_type_obj() + and callee.type_object().is_protocol + # Exception for Type[...] + and not callee.from_type_type + ): + self.chk.fail( + message_registry.CANNOT_INSTANTIATE_PROTOCOL.format(callee.type_object().name), + context, + ) + elif ( callee.is_type_obj() and callee.type_object().is_abstract # Exception for Type[...] @@ -1245,24 +1256,14 @@ def check_callable_call( type = callee.type_object() # Determine whether the abstract attributes are functions with # None-compatible return types and whether they were defined in a protocol. - is_none_ret_and_prot: Dict[str, bool] = {} + is_none_ret_from_prot: Dict[str, bool] = {} for attr_name, abstract_status in type.abstract_attributes: if abstract_status == IS_ABSTRACT: - is_none_ret_and_prot[attr_name] = False + is_none_ret_from_prot[attr_name] = False continue - is_none_ret_and_prot[attr_name] = self._has_none_compat_return(type, attr_name) + is_none_ret_from_prot[attr_name] = self._check_non_ret_from_prot(type, attr_name) self.msg.cannot_instantiate_abstract_class( - callee.type_object().name, is_none_ret_and_prot, context - ) - elif ( - callee.is_type_obj() - and callee.type_object().is_protocol - # Exception for Type[...] - and not callee.from_type_type - ): - self.chk.fail( - message_registry.CANNOT_INSTANTIATE_PROTOCOL.format(callee.type_object().name), - context, + callee.type_object().name, is_none_ret_from_prot, context ) formal_to_actual = map_actuals_to_formals( @@ -1344,11 +1345,15 @@ def check_callable_call( callee = callee.copy_modified(ret_type=new_ret_type) return callee.ret_type, callee - def _has_none_compat_return(self, type: TypeInfo, attr_name: str) -> bool: + def _check_non_ret_from_prot(self, type: TypeInfo, attr_name: str) -> bool: """Determine whether the attribute is a function with None-compatible return type. Additionally, check whether the function was defined in a protocol. """ + if not state.strict_optional: + # If strict-optional is not set, is_subtype(NoneType(), T) is always True. + # So, we cannot do anything useful here in that case. + return False for base in type.mro: symnode = base.names.get(attr_name) if symnode is None or not base.is_protocol: @@ -1356,17 +1361,28 @@ def _has_none_compat_return(self, type: TypeInfo, attr_name: str) -> bool: node = symnode.node if isinstance(node, OverloadedFuncDef): if node.impl is not None: - assert isinstance(node.impl.type, CallableType) - return is_subtype(NoneType(), node.impl.type.ret_type) - for overload in node.items: - assert isinstance(overload.type, CallableType) - if is_subtype(NoneType(), overload.type.ret_type): + typ = get_proper_type(node.impl.type) + assert isinstance(typ, CallableType) + return is_subtype(NoneType(), typ.ret_type) + for func in node.items: + if isinstance(func, Decorator): + func = func.func + assert isinstance(func.type, CallableType) + if is_subtype(NoneType(), func.type.ret_type): return True + if func.is_property: + return False # Only check the first overload of properties. else: return False - if isinstance(node, FuncDef) and node.type is not None: - assert isinstance(node.type, CallableType) - return is_subtype(NoneType(), node.type.ret_type) + if isinstance(node, Decorator): + node = node.func + if isinstance(node, FuncDef): + if node.type is not None: + assert isinstance(node.type, CallableType) + return is_subtype(NoneType(), node.type.ret_type) + else: + # No type annotations are available + return True return False def analyze_type_type_callee(self, item: ProperType, context: Context) -> Type: diff --git a/mypy/nodes.py b/mypy/nodes.py index f74a5f5da227..4094b2b0ac0a 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -3041,7 +3041,7 @@ def deserialize(cls, data: JsonDict) -> "TypeInfo": ti = TypeInfo(names, defn, module_name) ti._fullname = data["fullname"] # TODO: Is there a reason to reconstruct ti.subtypes? - ti.abstract_attributes = [tuple(attr) for attr in data["abstract_attributes"]] + ti.abstract_attributes = [(attr[0], attr[1]) for attr in data["abstract_attributes"]] ti.type_vars = data["type_vars"] ti.has_param_spec_type = data["has_param_spec_type"] ti.bases = [mypy.types.Instance.deserialize(b) for b in data["bases"]] diff --git a/mypy/semanal.py b/mypy/semanal.py index 963f74d9f0a0..c2a48a6dadb5 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -841,7 +841,7 @@ def analyze_func_def(self, defn: FuncDef) -> None: self.analyze_arg_initializers(defn) self.analyze_function_body(defn) - if self.is_class_scope() and defn.type is not None: + if self.is_class_scope(): assert self.type is not None # Mark protocol methods with empty bodies as implicitly abstract. # This makes explicit protocol subclassing type-safe. diff --git a/mypy/semanal_classprop.py b/mypy/semanal_classprop.py index 6e48e7410b92..2fed79e87a89 100644 --- a/mypy/semanal_classprop.py +++ b/mypy/semanal_classprop.py @@ -103,13 +103,13 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E def report(message: str, severity: str) -> None: errors.report(typ.line, typ.column, message, severity=severity) - attrs = ", ".join(f'"{attr}"' for attr in sorted(abstract)) + attrs = ", ".join(f'"{attr}"' for attr, _ in sorted(abstract)) report(f"Class {typ.fullname} has abstract attributes {attrs}", "error") report( "If it is meant to be abstract, add 'abc.ABCMeta' as an explicit metaclass", "note" ) if typ.is_final and abstract: - attrs = ", ".join(f'"{attr}"' for attr in sorted(abstract)) + attrs = ", ".join(f'"{attr}"' for attr, _ in sorted(abstract)) errors.report( typ.line, typ.column, f"Final class {typ.fullname} has abstract attributes {attrs}" ) diff --git a/test-data/unit/check-protocols.test b/test-data/unit/check-protocols.test index 80c10e0196c8..17ac6b1e0111 100644 --- a/test-data/unit/check-protocols.test +++ b/test-data/unit/check-protocols.test @@ -444,7 +444,7 @@ class P(C, Protocol): # E: All bases of a protocol must be protocols class P2(P, D, Protocol): # E: All bases of a protocol must be protocols pass -P2() # E: Cannot instantiate abstract class "P2" with abstract attribute "attr" +P2() # E: Cannot instantiate protocol class "P2" p: P2 reveal_type(p.attr) # N: Revealed type is "builtins.int" @@ -1553,7 +1553,7 @@ f2(z) # E: Argument 1 to "f2" has incompatible type "Union[C, D1]"; expected "P2 from typing import Type, Protocol class P(Protocol): - def m(self) -> None: pass + def m(self) -> None: return None class P1(Protocol): def m(self) -> None: pass class Pbad(Protocol): @@ -1599,7 +1599,7 @@ f(GoodAlias) from typing import Type, Protocol class P(Protocol): - def m(self) -> None: pass + def m(self) -> None: return None class B(P): pass class C: def m(self) -> None: @@ -2926,6 +2926,7 @@ def round(number: SupportsRound[_T], ndigits: int) -> _T: ... round(C(), 1) [case testEmptyBodyImplicitlyAbstractProtocol] +# flags: --strict-optional from typing import Protocol, overload, Union class P1(Protocol): @@ -2953,6 +2954,12 @@ class P3(Protocol): def meth(self, x: int) -> int: ... @overload def meth(self, x: str) -> str: ... + @overload + def not_abstract(self, x: int) -> int: ... + @overload + def not_abstract(self, x: str) -> str: ... + def not_abstract(self, x: Union[int, str]) -> Union[int, str]: + return 0 class B3(P3): ... class C3(P3): @overload @@ -2966,6 +2973,7 @@ C3() [builtins fixtures/classmethod.pyi] [case testEmptyBodyImplicitlyAbstractProtocolProperty] +# flags: --strict-optional from typing import Protocol class P1(Protocol): @@ -3059,17 +3067,50 @@ class D(WithRaise): ... D() # E: Cannot instantiate abstract class "D" with abstract attribute "meth" [builtins fixtures/exception.pyi] -[case testEmptyBodyNonAbstractProtocol] -from typing import Any, Optional, Protocol, Union +[case testEmptyBodyNoneCompatibleProtocol] +# flags: --strict-optional +from typing import Any, Optional, Protocol, Union, overload +from typing_extensions import TypeAlias + +NoneAlias: TypeAlias = None -class NotAbstract(Protocol): +class NoneCompatible(Protocol): def f(self) -> None: ... def g(self) -> Any: ... - def h(self, x: int): ... - def j(self): ... - def k(self, x): ... - def l(self) -> Optional[int]: ... - def m(self) -> Union[str, None]: ... - -class A(NotAbstract): ... -A() + def h(self): ... + def j(self) -> Optional[int]: ... + def k(self) -> Union[str, None]: ... + def l(self) -> NoneAlias: ... + @overload + def m(self, x: int) -> int: ... + @overload + def m(self, x: str) -> None: ... + @classmethod + def n(cls) -> None: ... + def o(self, x): ... + def p(self, x: int): ... + +class A(NoneCompatible): ... +A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g", ... and "p" (7 methods suppressed) \ + # N: "f" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "g" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "h" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "j" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "k" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "l" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "m" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "n" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "o" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + # N: "p" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. +[builtins fixtures/tuple.pyi] +[builtins fixtures/classmethod.pyi] + +[case testEmptyBodyWithFinal] +from typing import Protocol, final + +class P(Protocol): + @final # E: Protocol member cannot be final + def f(self, x: int) -> str: ... + +class A(P): ... +A() # E: Cannot instantiate abstract class "A" with abstract attribute "f" From decee85074f191bd2153f129799e7f15ca5eebb2 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Sat, 30 Jul 2022 14:53:18 +0100 Subject: [PATCH 07/11] Prevent protocols from becoming abstract Not sure why this is needed. --- mypy/semanal_classprop.py | 6 ++++-- mypy/stubgen.py | 5 ++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/mypy/semanal_classprop.py b/mypy/semanal_classprop.py index 2fed79e87a89..39b4945417b2 100644 --- a/mypy/semanal_classprop.py +++ b/mypy/semanal_classprop.py @@ -78,13 +78,15 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E func.abstract_status in (IS_ABSTRACT, IMPLICITLY_ABSTRACT) and name not in concrete ): - typ.is_abstract = True + if not typ.is_protocol: + typ.is_abstract = True abstract.append((name, func.abstract_status)) if base is typ: abstract_in_this_class.append(name) elif isinstance(node, Var): if node.is_abstract_var and name not in concrete: - typ.is_abstract = True + if not typ.is_protocol: + typ.is_abstract = True abstract.append((name, IS_ABSTRACT)) if base is typ: abstract_in_this_class.append(name) diff --git a/mypy/stubgen.py b/mypy/stubgen.py index 4a0717024e33..7e46956b8a5d 100755 --- a/mypy/stubgen.py +++ b/mypy/stubgen.py @@ -71,6 +71,7 @@ ARG_POS, ARG_STAR, ARG_STAR2, + IS_ABSTRACT, AssignmentStmt, Block, BytesExpr, @@ -723,7 +724,9 @@ def visit_func_def( retname = None # implicit Any else: retname = self.print_annotation(o.unanalyzed_type.ret_type) - elif isinstance(o, FuncDef) and (o.abstract_status or o.name in METHODS_WITH_RETURN_VALUE): + elif isinstance(o, FuncDef) and ( + o.abstract_status == IS_ABSTRACT or o.name in METHODS_WITH_RETURN_VALUE + ): # Always assume abstract methods return Any unless explicitly annotated. Also # some dunder methods should not have a None return type. retname = None # implicit Any From 2e99ddd321adebcebf4081beec88e98b10c9c4c3 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Tue, 2 Aug 2022 13:57:08 +0200 Subject: [PATCH 08/11] Address review feedback - rename some variables and functions - treat overloads without implementation as explicitly abstract - don't show the note for methods without type annotations - add more comments to explain things - don't rely on specific constants being falsey - fix stubgen --- mypy/checker.py | 4 ++- mypy/checkexpr.py | 44 +++++++++-------------------- mypy/messages.py | 4 +-- mypy/nodes.py | 4 +++ mypy/semanal.py | 8 +++--- mypy/semanal_classprop.py | 7 ++--- mypy/strconv.py | 2 +- mypy/stubgen.py | 8 +++--- test-data/unit/check-protocols.test | 21 ++++---------- test-data/unit/stubgen.test | 17 +++++++++++ 10 files changed, 58 insertions(+), 61 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 1b69d7a96163..f9c7d0402975 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -67,7 +67,9 @@ CONTRAVARIANT, COVARIANT, GDEF, + IMPLICITLY_ABSTRACT, INVARIANT, + IS_ABSTRACT, LDEF, LITERAL_TYPE, MDEF, @@ -617,7 +619,7 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: for fdef in defn.items: assert isinstance(fdef, Decorator) self.check_func_item(fdef.func, name=fdef.func.name) - if fdef.func.abstract_status: + if fdef.func.abstract_status in (IS_ABSTRACT, IMPLICITLY_ABSTRACT): num_abstract += 1 if num_abstract not in (0, len(defn.items)): self.fail(message_registry.INCONSISTENT_ABSTRACT_OVERLOAD, defn) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index 800b782f4d3e..d580a671a6cc 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -27,7 +27,7 @@ ARG_POS, ARG_STAR, ARG_STAR2, - IS_ABSTRACT, + IMPLICITLY_ABSTRACT, LITERAL_TYPE, REVEAL_TYPE, ArgKind, @@ -1254,16 +1254,16 @@ def check_callable_call( and not callee.type_object().fallback_to_any ): type = callee.type_object() - # Determine whether the abstract attributes are functions with - # None-compatible return types and whether they were defined in a protocol. - is_none_ret_from_prot: Dict[str, bool] = {} + # Determine whether the implicitly abstract attributes are functions with + # None-compatible return types. + abstract_attributes: Dict[str, bool] = {} for attr_name, abstract_status in type.abstract_attributes: - if abstract_status == IS_ABSTRACT: - is_none_ret_from_prot[attr_name] = False - continue - is_none_ret_from_prot[attr_name] = self._check_non_ret_from_prot(type, attr_name) + if abstract_status == IMPLICITLY_ABSTRACT: + abstract_attributes[attr_name] = self.can_return_none(type, attr_name) + else: + abstract_attributes[attr_name] = False self.msg.cannot_instantiate_abstract_class( - callee.type_object().name, is_none_ret_from_prot, context + callee.type_object().name, abstract_attributes, context ) formal_to_actual = map_actuals_to_formals( @@ -1345,10 +1345,10 @@ def check_callable_call( callee = callee.copy_modified(ret_type=new_ret_type) return callee.ret_type, callee - def _check_non_ret_from_prot(self, type: TypeInfo, attr_name: str) -> bool: - """Determine whether the attribute is a function with None-compatible return type. + def can_return_none(self, type: TypeInfo, attr_name: str) -> bool: + """Is the given attribute a method with a None-compatible return type? - Additionally, check whether the function was defined in a protocol. + Overloads are only checked if there is an implementation. """ if not state.strict_optional: # If strict-optional is not set, is_subtype(NoneType(), T) is always True. @@ -1356,33 +1356,17 @@ def _check_non_ret_from_prot(self, type: TypeInfo, attr_name: str) -> bool: return False for base in type.mro: symnode = base.names.get(attr_name) - if symnode is None or not base.is_protocol: + if symnode is None: continue node = symnode.node if isinstance(node, OverloadedFuncDef): - if node.impl is not None: - typ = get_proper_type(node.impl.type) - assert isinstance(typ, CallableType) - return is_subtype(NoneType(), typ.ret_type) - for func in node.items: - if isinstance(func, Decorator): - func = func.func - assert isinstance(func.type, CallableType) - if is_subtype(NoneType(), func.type.ret_type): - return True - if func.is_property: - return False # Only check the first overload of properties. - else: - return False + node = node.impl if isinstance(node, Decorator): node = node.func if isinstance(node, FuncDef): if node.type is not None: assert isinstance(node.type, CallableType) return is_subtype(NoneType(), node.type.ret_type) - else: - # No type annotations are available - return True return False def analyze_type_type_callee(self, item: ProperType, context: Context) -> Type: diff --git a/mypy/messages.py b/mypy/messages.py index 19a9257d6c7d..bbf65e43e58e 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1330,8 +1330,8 @@ def cannot_instantiate_abstract_class( context, code=codes.ABSTRACT, ) - for a, is_none_ret_and_prot in abstract_attributes.items(): - if not is_none_ret_and_prot: + for a, can_return_none_and_implicit in abstract_attributes.items(): + if not can_return_none_and_implicit: continue self.note( '"{}" was implicitly marked abstract because it has an empty function body. ' diff --git a/mypy/nodes.py b/mypy/nodes.py index 4094b2b0ac0a..e1230f802cf6 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -762,7 +762,9 @@ def is_dynamic(self) -> bool: # Abstract status of a function NOT_ABSTRACT: Final = 0 +# Explicitly abstract (with @abstractmethod or overload without implementation) IS_ABSTRACT: Final = 1 +# Implicitly abstract: used for functions with trivial bodies defined in Protocols IMPLICITLY_ABSTRACT: Final = 2 @@ -2681,6 +2683,8 @@ class is generic then it will be a type constructor of higher kind. is_abstract: bool # Does the class have any abstract attributes? is_protocol: bool # Is this a protocol class? runtime_protocol: bool # Does this protocol support isinstance checks? + # List of names of abstract attributes together with their abstract status. + # The abstract status must be one of `NOT_ABSTRACT`, `IS_ABSTRACT`, `IMPLICITLY_ABSTRACT`. abstract_attributes: List[Tuple[str, int]] deletable_attributes: List[str] # Used by mypyc only # Does this type have concrete `__slots__` defined? diff --git a/mypy/semanal.py b/mypy/semanal.py index c2a48a6dadb5..e286bc625a35 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -1084,12 +1084,12 @@ def handle_missing_overload_implementation(self, defn: OverloadedFuncDef) -> Non if not self.is_stub_file: if self.type and self.type.is_protocol and not self.is_func_scope(): # An overloaded protocol method doesn't need an implementation, - # but if it doesn't have one, then it is considered implicitly abstract. + # but if it doesn't have one, then it is considered abstract. for item in defn.items: if isinstance(item, Decorator): - item.func.abstract_status = IMPLICITLY_ABSTRACT + item.func.abstract_status = IS_ABSTRACT else: - item.abstract_status = IMPLICITLY_ABSTRACT + item.abstract_status = IS_ABSTRACT else: self.fail( "An overloaded function outside a stub file must have an implementation", @@ -1303,7 +1303,7 @@ def visit_decorator(self, dec: Decorator) -> None: dec.func.accept(self) if dec.decorators and dec.var.is_property: self.fail("Decorated property not supported", dec) - if dec.func.abstract_status and dec.func.is_final: + if dec.func.abstract_status == IS_ABSTRACT and dec.func.is_final: self.fail(f"Method {dec.func.name} is both abstract and final", dec) def check_decorated_function_is_method(self, decorator: str, context: Context) -> None: diff --git a/mypy/semanal_classprop.py b/mypy/semanal_classprop.py index 39b4945417b2..ff60d424002b 100644 --- a/mypy/semanal_classprop.py +++ b/mypy/semanal_classprop.py @@ -49,6 +49,7 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E if typ.typeddict_type: return # TypedDict can't be abstract concrete: Set[str] = set() + # List of abstract attributes together with their abstract status abstract: List[Tuple[str, int]] = [] abstract_in_this_class: List[str] = [] if typ.is_newtype: @@ -78,15 +79,13 @@ def calculate_class_abstract_status(typ: TypeInfo, is_stub_file: bool, errors: E func.abstract_status in (IS_ABSTRACT, IMPLICITLY_ABSTRACT) and name not in concrete ): - if not typ.is_protocol: - typ.is_abstract = True + typ.is_abstract = True abstract.append((name, func.abstract_status)) if base is typ: abstract_in_this_class.append(name) elif isinstance(node, Var): if node.is_abstract_var and name not in concrete: - if not typ.is_protocol: - typ.is_abstract = True + typ.is_abstract = True abstract.append((name, IS_ABSTRACT)) if base is typ: abstract_in_this_class.append(name) diff --git a/mypy/strconv.py b/mypy/strconv.py index 30728ed3c6f2..2d6d6a01066b 100644 --- a/mypy/strconv.py +++ b/mypy/strconv.py @@ -133,7 +133,7 @@ def visit_func_def(self, o: "mypy.nodes.FuncDef") -> str: arg_kinds = {arg.kind for arg in o.arguments} if len(arg_kinds & {mypy.nodes.ARG_NAMED, mypy.nodes.ARG_NAMED_OPT}) > 0: a.insert(1, f"MaxPos({o.max_pos})") - if o.abstract_status: + if o.abstract_status in (mypy.nodes.IS_ABSTRACT, mypy.nodes.IMPLICITLY_ABSTRACT): a.insert(-1, "Abstract") if o.is_static: a.insert(-1, "Static") diff --git a/mypy/stubgen.py b/mypy/stubgen.py index 7e46956b8a5d..604296d8759b 100755 --- a/mypy/stubgen.py +++ b/mypy/stubgen.py @@ -913,16 +913,16 @@ def visit_class_def(self, o: ClassDef) -> None: if isinstance(o.metaclass, (NameExpr, MemberExpr)): meta = o.metaclass.accept(AliasPrinter(self)) base_types.append("metaclass=" + meta) - elif self.analyzed and o.info.is_abstract: - base_types.append("metaclass=abc.ABCMeta") - self.import_tracker.add_import("abc") - self.import_tracker.require_name("abc") elif self.analyzed and o.info.is_protocol: type_str = "Protocol" if o.info.type_vars: type_str += f'[{", ".join(o.info.type_vars)}]' base_types.append(type_str) self.add_typing_import("Protocol") + elif self.analyzed and o.info.is_abstract: + base_types.append("metaclass=abc.ABCMeta") + self.import_tracker.add_import("abc") + self.import_tracker.require_name("abc") if base_types: self.add(f"({', '.join(base_types)})") self.add(":\n") diff --git a/test-data/unit/check-protocols.test b/test-data/unit/check-protocols.test index 17ac6b1e0111..8e543f6d1b25 100644 --- a/test-data/unit/check-protocols.test +++ b/test-data/unit/check-protocols.test @@ -3077,21 +3077,15 @@ NoneAlias: TypeAlias = None class NoneCompatible(Protocol): def f(self) -> None: ... def g(self) -> Any: ... - def h(self): ... - def j(self) -> Optional[int]: ... - def k(self) -> Union[str, None]: ... - def l(self) -> NoneAlias: ... - @overload - def m(self, x: int) -> int: ... - @overload - def m(self, x: str) -> None: ... + def h(self) -> Optional[int]: ... + def j(self) -> Union[str, None]: ... + def k(self) -> NoneAlias: ... @classmethod - def n(cls) -> None: ... - def o(self, x): ... - def p(self, x: int): ... + def l(cls) -> None: ... + def m(self, x: int): ... class A(NoneCompatible): ... -A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g", ... and "p" (7 methods suppressed) \ +A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g", ... and "m" (4 methods suppressed) \ # N: "f" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ # N: "g" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ # N: "h" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ @@ -3099,9 +3093,6 @@ A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g" # N: "k" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ # N: "l" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ # N: "m" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "n" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "o" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "p" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. [builtins fixtures/tuple.pyi] [builtins fixtures/classmethod.pyi] diff --git a/test-data/unit/stubgen.test b/test-data/unit/stubgen.test index a7c2ae6d21fd..408f116443d2 100644 --- a/test-data/unit/stubgen.test +++ b/test-data/unit/stubgen.test @@ -2675,6 +2675,23 @@ T2 = TypeVar('T2') class PT(Protocol[T, T2]): def f(self, x: T) -> T2: ... +[case testProtocolAbstractMethod_semanal] +from abc import abstractmethod +from typing import Protocol + +class P(Protocol): + @abstractmethod + def f(self, x: int, y: int) -> str: + ... + +[out] +from abc import abstractmethod +from typing import Protocol + +class P(Protocol): + @abstractmethod + def f(self, x: int, y: int) -> str: ... + [case testNonDefaultKeywordOnlyArgAfterAsterisk] def func(*, non_default_kwarg: bool, default_kwarg: bool = True): ... [out] From b2f3fbd793ee411f6c750cd91b9a3cfe679c99ec Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Tue, 2 Aug 2022 14:54:35 +0100 Subject: [PATCH 09/11] Don't mark as implicitly abstract if it's already abstract --- mypy/semanal.py | 1 + test-data/unit/check-protocols.test | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/mypy/semanal.py b/mypy/semanal.py index e286bc625a35..7fb8bae48ade 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -849,6 +849,7 @@ def analyze_func_def(self, defn: FuncDef) -> None: self.type.is_protocol and not self.is_stub_file # Bodies in stub files are always empty. and (not isinstance(self.scope.function, OverloadedFuncDef) or defn.is_property) + and defn.abstract_status != IS_ABSTRACT and is_trivial_body(defn.body) ): defn.abstract_status = IMPLICITLY_ABSTRACT diff --git a/test-data/unit/check-protocols.test b/test-data/unit/check-protocols.test index 8e543f6d1b25..b77447da166c 100644 --- a/test-data/unit/check-protocols.test +++ b/test-data/unit/check-protocols.test @@ -3069,6 +3069,7 @@ D() # E: Cannot instantiate abstract class "D" with abstract attribute "meth" [case testEmptyBodyNoneCompatibleProtocol] # flags: --strict-optional +from abc import abstractmethod from typing import Any, Optional, Protocol, Union, overload from typing_extensions import TypeAlias @@ -3093,6 +3094,18 @@ A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g" # N: "k" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ # N: "l" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ # N: "m" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ + +class NoneCompatible2(Protocol): + @abstractmethod + def f(self) -> None: ... + @overload + def g(self, x: int) -> int: ... + @overload + def g(self, x: str) -> None: ... + def h(self, x): ... + +class B(NoneCompatible2): ... +B() # E: Cannot instantiate abstract class "B" with abstract attributes "f", "g" and "h" [builtins fixtures/tuple.pyi] [builtins fixtures/classmethod.pyi] From b96e4ecd1efc0f707f6ff0f593912ba3adb14683 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Tue, 2 Aug 2022 15:17:49 +0100 Subject: [PATCH 10/11] Fix context of new message --- mypy/messages.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypy/messages.py b/mypy/messages.py index bbf65e43e58e..8b818c09daef 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1337,7 +1337,7 @@ def cannot_instantiate_abstract_class( '"{}" was implicitly marked abstract because it has an empty function body. ' "If it is not meant to be abstract, explicitly return None.".format(a), context, - code=codes.OVERRIDE, + code=codes.ABSTRACT, ) def base_class_definitions_incompatible( From 7dc8d868e90273e2310831bf7cfb5d55f8748b93 Mon Sep 17 00:00:00 2001 From: Thomas MK Date: Tue, 2 Aug 2022 18:43:23 +0100 Subject: [PATCH 11/11] Tweak the note --- mypy/messages.py | 25 +++++++++++++++++-------- test-data/unit/check-protocols.test | 27 +++++++++++++-------------- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/mypy/messages.py b/mypy/messages.py index 8b818c09daef..910e193f9124 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -1330,15 +1330,24 @@ def cannot_instantiate_abstract_class( context, code=codes.ABSTRACT, ) - for a, can_return_none_and_implicit in abstract_attributes.items(): - if not can_return_none_and_implicit: - continue - self.note( - '"{}" was implicitly marked abstract because it has an empty function body. ' - "If it is not meant to be abstract, explicitly return None.".format(a), - context, - code=codes.ABSTRACT, + attrs_with_none = [ + f'"{a}"' + for a, implicit_and_can_return_none in abstract_attributes.items() + if implicit_and_can_return_none + ] + if not attrs_with_none: + return + if len(attrs_with_none) == 1: + note = ( + "The following method was marked implicitly abstract because it has an empty " + "function body: {}. If it is not meant to be abstract, explicitly return None." + ) + else: + note = ( + "The following methods were marked implicitly abstract because they have empty " + "function bodies: {}. If they are not meant to be abstract, explicitly return None." ) + self.note(note.format(format_string_list(attrs_with_none)), context, code=codes.ABSTRACT) def base_class_definitions_incompatible( self, name: str, base1: TypeInfo, base2: TypeInfo, context: Context diff --git a/test-data/unit/check-protocols.test b/test-data/unit/check-protocols.test index b77447da166c..9943a8fb4388 100644 --- a/test-data/unit/check-protocols.test +++ b/test-data/unit/check-protocols.test @@ -3079,23 +3079,22 @@ class NoneCompatible(Protocol): def f(self) -> None: ... def g(self) -> Any: ... def h(self) -> Optional[int]: ... - def j(self) -> Union[str, None]: ... - def k(self) -> NoneAlias: ... + def i(self) -> NoneAlias: ... @classmethod - def l(cls) -> None: ... - def m(self, x: int): ... + def j(cls) -> None: ... class A(NoneCompatible): ... -A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g", ... and "m" (4 methods suppressed) \ - # N: "f" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "g" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "h" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "j" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "k" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "l" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ - # N: "m" was implicitly marked abstract because it has an empty function body. If it is not meant to be abstract, explicitly return None. \ +A() # E: Cannot instantiate abstract class "A" with abstract attributes "f", "g", "h", "i" and "j" \ + # N: The following methods were marked implicitly abstract because they have empty function bodies: "f", "g", "h", "i" and "j". If they are not meant to be abstract, explicitly return None. class NoneCompatible2(Protocol): + def f(self, x: int): ... + +class B(NoneCompatible2): ... +B() # E: Cannot instantiate abstract class "B" with abstract attribute "f" \ + # N: The following method was marked implicitly abstract because it has an empty function body: "f". If it is not meant to be abstract, explicitly return None. + +class NoneCompatible3(Protocol): @abstractmethod def f(self) -> None: ... @overload @@ -3104,8 +3103,8 @@ class NoneCompatible2(Protocol): def g(self, x: str) -> None: ... def h(self, x): ... -class B(NoneCompatible2): ... -B() # E: Cannot instantiate abstract class "B" with abstract attributes "f", "g" and "h" +class C(NoneCompatible3): ... +C() # E: Cannot instantiate abstract class "C" with abstract attributes "f", "g" and "h" [builtins fixtures/tuple.pyi] [builtins fixtures/classmethod.pyi]