From 3d4c8eff1fd8a16529412e009de4430d349389db Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Thu, 23 Mar 2023 23:21:07 +0000 Subject: [PATCH 1/6] Stubtest: verify stub methods or properties are decorated with `@final` if they are decorated with `@final` at runtime --- mypy/stubtest.py | 8 +++ mypy/test/teststubtest.py | 141 +++++++++++++++++++++++++++++++++++++- 2 files changed, 148 insertions(+), 1 deletion(-) diff --git a/mypy/stubtest.py b/mypy/stubtest.py index b0ef94e62480..46b9f4663648 100644 --- a/mypy/stubtest.py +++ b/mypy/stubtest.py @@ -564,6 +564,8 @@ def _verify_static_class_methods( yield "runtime is a staticmethod but stub is not" if not isinstance(static_runtime, staticmethod) and stub.is_static: yield "stub is a staticmethod but runtime is not" + if isinstance(stub, nodes.FuncDef): + yield from _verify_final_method(stub, static_runtime) def _verify_arg_name( @@ -1115,6 +1117,7 @@ def verify_paramspecexpr( def _verify_readonly_property(stub: nodes.Decorator, runtime: Any) -> Iterator[str]: assert stub.func.is_property if isinstance(runtime, property): + yield from _verify_final_method(stub.func, runtime.fget) return if inspect.isdatadescriptor(runtime): # It's enough like a property... @@ -1143,6 +1146,11 @@ def _verify_abstract_status(stub: nodes.FuncDef, runtime: Any) -> Iterator[str]: yield f"is inconsistent, runtime {item_type} is abstract but stub is not" +def _verify_final_method(stub: nodes.FuncDef, runtime: Any) -> Iterator[str]: + if getattr(runtime, "__final__", False) and not stub.is_final: + yield "is decorated with @final at runtime, but not in the stub" + + def _resolve_funcitem_from_decorator(dec: nodes.OverloadPart) -> nodes.FuncItem | None: """Returns a FuncItem that corresponds to the output of the decorator. diff --git a/mypy/test/teststubtest.py b/mypy/test/teststubtest.py index d39812b5f9b6..29eed8e4755d 100644 --- a/mypy/test/teststubtest.py +++ b/mypy/test/teststubtest.py @@ -1143,7 +1143,10 @@ def test_not_subclassable(self) -> Iterator[Case]: def test_has_runtime_final_decorator(self) -> Iterator[Case]: yield Case( stub="from typing_extensions import final", - runtime="from typing_extensions import final", + runtime=""" + import functools + from typing_extensions import final + """, error=None, ) yield Case( @@ -1177,6 +1180,142 @@ class C: ... """, error="C", ) + yield Case( + stub=""" + class D: + @final + def foo(self) -> None: ... + @final + @staticmethod + def bar() -> None: ... + @final + @classmethod + def baz(cls) -> None: ... + @property + @final + def eggs(self) -> int: ... + @final + def ham(self, obj: int) -> int: ... + """, + runtime=""" + class D: + @final + def foo(self): pass + @final + @staticmethod + def bar(): pass + @final + @classmethod + def baz(cls): pass + @property + @final + def eggs(self): return 42 + @final + @functools.lru_cache() + def ham(self, obj): return obj * 2 + """, + error=None, + ) + # Stub methods are allowed to have @final even if the runtime doesn't... + yield Case( + stub=""" + class E: + @final + def foo(self) -> None: ... + @final + @staticmethod + def bar() -> None: ... + @final + @classmethod + def baz(cls) -> None: ... + @property + @final + def eggs(self) -> int: ... + @final + def ham(self, obj: int) -> int: ... + """, + runtime=""" + class E: + def foo(self): pass + @staticmethod + def bar(): pass + @classmethod + def baz(cls): pass + @property + def eggs(self): return 42 + @functools.lru_cache() + def ham(self, obj): return obj * 2 + """, + error=None, + ) + # ...But if the runtime has @final, the stub must have it as well + yield Case( + stub=""" + class F: + def foo(self) -> None: ... + """, + runtime=""" + class F: + @final + def foo(self): pass + """, + error="F.foo", + ) + yield Case( + stub=""" + class G: + @staticmethod + def foo() -> None: ... + """, + runtime=""" + class G: + @final + @staticmethod + def foo(): pass + """, + error="G.foo", + ) + yield Case( + stub=""" + class H: + @classmethod + def foo(cls) -> None: ... + """, + runtime=""" + class H: + @final + @classmethod + def foo(cls): pass + """, + error="H.foo", + ) + yield Case( + stub=""" + class I: + @property + def foo(self) -> int: ... + """, + runtime=""" + class I: + @property + @final + def foo(self): return 42 + """, + error="I.foo", + ) + yield Case( + stub=""" + class J: + def foo(self, obj: int) -> int: ... + """, + runtime=""" + class J: + @final + @functools.lru_cache() + def foo(self, obj): return obj * 2 + """, + error="J.foo", + ) @collect_cases def test_name_mangling(self) -> Iterator[Case]: From b749c908ec0db84560981e1d26e23bf9146b58fb Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 14:35:34 +0000 Subject: [PATCH 2/6] More tests --- mypy/test/teststubtest.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/mypy/test/teststubtest.py b/mypy/test/teststubtest.py index 29eed8e4755d..97445c03361d 100644 --- a/mypy/test/teststubtest.py +++ b/mypy/test/teststubtest.py @@ -1188,13 +1188,22 @@ def foo(self) -> None: ... @final @staticmethod def bar() -> None: ... + @staticmethod + @final + def bar2() -> None: ... @final @classmethod def baz(cls) -> None: ... + @classmethod + @final + def baz2(cls) -> None: ... @property @final def eggs(self) -> int: ... @final + @property + def eggs2(self) -> int: ... + @final def ham(self, obj: int) -> int: ... """, runtime=""" @@ -1204,13 +1213,22 @@ def foo(self): pass @final @staticmethod def bar(): pass + @staticmethod + @final + def bar2(): pass @final @classmethod def baz(cls): pass + @classmethod + @final + def baz2(cls): pass @property @final def eggs(self): return 42 @final + @property + def eggs2(self): pass + @final @functools.lru_cache() def ham(self, obj): return obj * 2 """, @@ -1225,13 +1243,22 @@ def foo(self) -> None: ... @final @staticmethod def bar() -> None: ... + @staticmethod + @final + def bar2() -> None: ... @final @classmethod def baz(cls) -> None: ... + @classmethod + @final + def baz2(cls) -> None: ... @property @final def eggs(self) -> int: ... @final + @property + def eggs2(self) -> int: ... + @final def ham(self, obj: int) -> int: ... """, runtime=""" @@ -1239,10 +1266,16 @@ class E: def foo(self): pass @staticmethod def bar(): pass + @staticmethod + def bar2(): pass @classmethod def baz(cls): pass + @classmethod + def baz2(cls): pass @property def eggs(self): return 42 + @property + def eggs2(self): return 42 @functools.lru_cache() def ham(self, obj): return obj * 2 """, From 28b43f38cae53128157f8223f572e7f7c98e9731 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 14:45:23 +0000 Subject: [PATCH 3/6] Refactor to be more principled --- mypy/stubtest.py | 40 ++++++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/mypy/stubtest.py b/mypy/stubtest.py index 46b9f4663648..fef3e33b1e26 100644 --- a/mypy/stubtest.py +++ b/mypy/stubtest.py @@ -529,8 +529,21 @@ def verify_typeinfo( yield from verify(stub_to_verify, runtime_attr, object_path + [entry]) +def _static_lookup_runtime(object_path: list[str]) -> MaybeMissing[Any]: + static_runtime = importlib.import_module(object_path[0]) + for entry in object_path[1:]: + try: + static_runtime = inspect.getattr_static(static_runtime, entry) + except AttributeError: + # This can happen with mangled names, ignore for now. + # TODO: pass more information about ancestors of nodes/objects to verify, so we don't + # have to do this hacky lookup. Would be useful in several places. + return MISSING + return static_runtime + + def _verify_static_class_methods( - stub: nodes.FuncBase, runtime: Any, object_path: list[str] + stub: nodes.FuncBase, runtime: Any, static_runtime: MaybeMissing[Any], object_path: list[str] ) -> Iterator[str]: if stub.name in ("__new__", "__init_subclass__", "__class_getitem__"): # Special cased by Python, so don't bother checking @@ -545,17 +558,6 @@ def _verify_static_class_methods( yield "stub is a classmethod but runtime is not" return - # Look the object up statically, to avoid binding by the descriptor protocol - static_runtime = importlib.import_module(object_path[0]) - for entry in object_path[1:]: - try: - static_runtime = inspect.getattr_static(static_runtime, entry) - except AttributeError: - # This can happen with mangled names, ignore for now. - # TODO: pass more information about ancestors of nodes/objects to verify, so we don't - # have to do this hacky lookup. Would be useful in a couple other places too. - return - if isinstance(static_runtime, classmethod) and not stub.is_class: yield "runtime is a classmethod but stub is not" if not isinstance(static_runtime, classmethod) and stub.is_class: @@ -564,8 +566,6 @@ def _verify_static_class_methods( yield "runtime is a staticmethod but stub is not" if not isinstance(static_runtime, staticmethod) and stub.is_static: yield "stub is a staticmethod but runtime is not" - if isinstance(stub, nodes.FuncDef): - yield from _verify_final_method(stub, static_runtime) def _verify_arg_name( @@ -947,11 +947,16 @@ def verify_funcitem( if not callable(runtime): return + # Look the object up statically, to avoid binding by the descriptor protocol + static_runtime = _static_lookup_runtime(object_path) + if isinstance(stub, nodes.FuncDef): for error_text in _verify_abstract_status(stub, runtime): yield Error(object_path, error_text, stub, runtime) + for error_text in _verify_final_method(stub, static_runtime): + yield Error(object_path, error_text, stub, runtime) - for message in _verify_static_class_methods(stub, runtime, object_path): + for message in _verify_static_class_methods(stub, runtime, static_runtime, object_path): yield Error(object_path, "is inconsistent, " + message, stub, runtime) signature = safe_inspect_signature(runtime) @@ -1054,7 +1059,10 @@ def verify_overloadedfuncdef( if not callable(runtime): return - for message in _verify_static_class_methods(stub, runtime, object_path): + # Look the object up statically, to avoid binding by the descriptor protocol + static_runtime = _static_lookup_runtime(object_path) + + for message in _verify_static_class_methods(stub, runtime, static_runtime, object_path): yield Error(object_path, "is inconsistent, " + message, stub, runtime) signature = safe_inspect_signature(runtime) From 329c1c8e8645cc83f04d08ed23c2cc901a67b44a Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 14:53:06 +0000 Subject: [PATCH 4/6] Also support the other order of stacking `@final` --- mypy/stubtest.py | 16 ++++++++++---- mypy/test/teststubtest.py | 44 ++++++++++++++++++++++++++++++++------- 2 files changed, 48 insertions(+), 12 deletions(-) diff --git a/mypy/stubtest.py b/mypy/stubtest.py index fef3e33b1e26..532d4a71439b 100644 --- a/mypy/stubtest.py +++ b/mypy/stubtest.py @@ -953,7 +953,7 @@ def verify_funcitem( if isinstance(stub, nodes.FuncDef): for error_text in _verify_abstract_status(stub, runtime): yield Error(object_path, error_text, stub, runtime) - for error_text in _verify_final_method(stub, static_runtime): + for error_text in _verify_final_method(stub, runtime, static_runtime): yield Error(object_path, error_text, stub, runtime) for message in _verify_static_class_methods(stub, runtime, static_runtime, object_path): @@ -1125,7 +1125,7 @@ def verify_paramspecexpr( def _verify_readonly_property(stub: nodes.Decorator, runtime: Any) -> Iterator[str]: assert stub.func.is_property if isinstance(runtime, property): - yield from _verify_final_method(stub.func, runtime.fget) + yield from _verify_final_method(stub.func, runtime.fget, MISSING) return if inspect.isdatadescriptor(runtime): # It's enough like a property... @@ -1154,8 +1154,16 @@ def _verify_abstract_status(stub: nodes.FuncDef, runtime: Any) -> Iterator[str]: yield f"is inconsistent, runtime {item_type} is abstract but stub is not" -def _verify_final_method(stub: nodes.FuncDef, runtime: Any) -> Iterator[str]: - if getattr(runtime, "__final__", False) and not stub.is_final: +def _verify_final_method(stub: nodes.FuncDef, runtime: Any, static_runtime: MaybeMissing[Any]) -> Iterator[str]: + if stub.is_final: + return + if ( + getattr(runtime, "__final__", False) + or ( + static_runtime is not MISSING + and getattr(static_runtime, "__final__", False) + ) + ): yield "is decorated with @final at runtime, but not in the stub" diff --git a/mypy/test/teststubtest.py b/mypy/test/teststubtest.py index 97445c03361d..5d7afdeb9455 100644 --- a/mypy/test/teststubtest.py +++ b/mypy/test/teststubtest.py @@ -1311,43 +1311,71 @@ def foo(): pass yield Case( stub=""" class H: + @staticmethod + def foo() -> None: ... + """, + runtime=""" + class H: + @staticmethod + @final + def foo(): pass + """, + error="H.foo", + ) + yield Case( + stub=""" + class I: @classmethod def foo(cls) -> None: ... """, runtime=""" - class H: + class I: @final @classmethod def foo(cls): pass """, - error="H.foo", + error="I.foo", ) yield Case( stub=""" - class I: + class J: + @classmethod + def foo(cls) -> None: ... + """, + runtime=""" + class J: + @classmethod + @final + def foo(cls): pass + """, + error="J.foo", + ) + yield Case( + stub=""" + class K: @property def foo(self) -> int: ... """, runtime=""" - class I: + class K: @property @final def foo(self): return 42 """, - error="I.foo", + error="K.foo", ) yield Case( stub=""" - class J: + class L: def foo(self, obj: int) -> int: ... """, runtime=""" - class J: + class L: @final @functools.lru_cache() def foo(self, obj): return obj * 2 """, - error="J.foo", + error="L.foo", ) @collect_cases From eb5d0e150aaba036b35fc0322806ca93e14ba3ea Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 15:02:44 +0000 Subject: [PATCH 5/6] Blacken, plus more comments --- mypy/stubtest.py | 14 +++++++------- mypy/test/teststubtest.py | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/mypy/stubtest.py b/mypy/stubtest.py index 532d4a71439b..28d79487c7fd 100644 --- a/mypy/stubtest.py +++ b/mypy/stubtest.py @@ -1065,6 +1065,8 @@ def verify_overloadedfuncdef( for message in _verify_static_class_methods(stub, runtime, static_runtime, object_path): yield Error(object_path, "is inconsistent, " + message, stub, runtime) + # TODO: Should call _verify_final_method here, but it crashes stubtest: see #14950 + signature = safe_inspect_signature(runtime) if not signature: return @@ -1154,15 +1156,13 @@ def _verify_abstract_status(stub: nodes.FuncDef, runtime: Any) -> Iterator[str]: yield f"is inconsistent, runtime {item_type} is abstract but stub is not" -def _verify_final_method(stub: nodes.FuncDef, runtime: Any, static_runtime: MaybeMissing[Any]) -> Iterator[str]: +def _verify_final_method( + stub: nodes.FuncDef, runtime: Any, static_runtime: MaybeMissing[Any] +) -> Iterator[str]: if stub.is_final: return - if ( - getattr(runtime, "__final__", False) - or ( - static_runtime is not MISSING - and getattr(static_runtime, "__final__", False) - ) + if getattr(runtime, "__final__", False) or ( + static_runtime is not MISSING and getattr(static_runtime, "__final__", False) ): yield "is decorated with @final at runtime, but not in the stub" diff --git a/mypy/test/teststubtest.py b/mypy/test/teststubtest.py index 5d7afdeb9455..25ee2c524bfb 100644 --- a/mypy/test/teststubtest.py +++ b/mypy/test/teststubtest.py @@ -1364,6 +1364,24 @@ def foo(self): return 42 """, error="K.foo", ) + # This test wouldn't pass, + # because the runtime can't set __final__ on instances of builtins.property, + # so stubtest has non way of knowing that the runtime was decorated with @final: + # + # yield Case( + # stub=""" + # class K2: + # @property + # def foo(self) -> int: ... + # """, + # runtime=""" + # class K2: + # @final + # @property + # def foo(self): return 42 + # """, + # error="K2.foo", + # ) yield Case( stub=""" class L: From f41630fb85daa1fb163b9f7dc3e0ce7558f0d120 Mon Sep 17 00:00:00 2001 From: AlexWaygood Date: Fri, 24 Mar 2023 15:13:59 +0000 Subject: [PATCH 6/6] Fix behaviour change in `_verify_static_class_method`; improve comment --- mypy/stubtest.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/mypy/stubtest.py b/mypy/stubtest.py index 28d79487c7fd..860247764de0 100644 --- a/mypy/stubtest.py +++ b/mypy/stubtest.py @@ -558,6 +558,9 @@ def _verify_static_class_methods( yield "stub is a classmethod but runtime is not" return + if static_runtime is MISSING: + return + if isinstance(static_runtime, classmethod) and not stub.is_class: yield "runtime is a classmethod but stub is not" if not isinstance(static_runtime, classmethod) and stub.is_class: @@ -1065,7 +1068,8 @@ def verify_overloadedfuncdef( for message in _verify_static_class_methods(stub, runtime, static_runtime, object_path): yield Error(object_path, "is inconsistent, " + message, stub, runtime) - # TODO: Should call _verify_final_method here, but it crashes stubtest: see #14950 + # TODO: Should call _verify_final_method here, + # but overloaded final methods in stubs cause a stubtest crash: see #14950 signature = safe_inspect_signature(runtime) if not signature: