diff --git a/HISTORY.rst b/HISTORY.rst index f94da6bc..65ac8b5a 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -1,6 +1,11 @@ Changelog --------- +Unreleased +~~~~~~~~~~ + +* Fix ``typing.Protocol`` classes being mutated. The generated mutant methods became members of the protocol, so classes implementing it no longer conformed, and ``type_check_command`` either crashed with "Could not find mutant for type error" or discarded unrelated mutants as caught by the type checker. Methods of protocols, including default implementations, are no longer mutated (`#590`) + 3.8.0 ~~~~~~~~~~ diff --git a/src/mutmut/mutation/file_mutation.py b/src/mutmut/mutation/file_mutation.py index 3613565a..9f91e512 100644 --- a/src/mutmut/mutation/file_mutation.py +++ b/src/mutmut/mutation/file_mutation.py @@ -16,6 +16,7 @@ import libcst as cst import libcst.matchers as m +from libcst.helpers import get_full_name_for_node from libcst.metadata import MetadataWrapper from libcst.metadata import PositionProvider @@ -217,8 +218,13 @@ def __init__( ) self.ignored_classes: set[str] = set() self.ignored_functions: set[str] = set() + # names that `typing.Protocol` is imported as (`Protocol`, or an alias), and that its modules are imported as + self._protocol_names: set[str] = set() + self._protocol_module_names: set[str] = set() def on_visit(self, node: cst.CSTNode) -> bool: + self._record_protocol_import(node) + if self._skip_node_and_children(node): return False @@ -334,6 +340,17 @@ def _skip_node_and_children(self, node: cst.CSTNode) -> bool: return False return True + # ignore Protocol classes, because + # 1) the trampoline setup adds the mutant methods (`xǁFooǁbar__mutmut_1`, ...) as members of the protocol, + # so classes that implement it structurally no longer conform and type checkers report errors at every + # place where they are used as the protocol, which either crashes the type check filter or gets + # attributed to unrelated mutants + # 2) method stubs are never executed, so mutants of their default arguments cannot be killed + # This also leaves method bodies of protocols unmutated, including default implementations + # that explicit subclasses inherit, because mutating them changes the members in the same way. + if isinstance(node, cst.ClassDef) and self._is_protocol_class(node): + return True + # decorators are executed at definition time, so mutating them can raise # exceptions on import. A decorated class is still recursed into, because # its decorator is kept on the original class and only its methods get @@ -343,6 +360,49 @@ def _skip_node_and_children(self, node: cst.CSTNode) -> bool: return False + def _is_protocol_class(self, node: cst.ClassDef) -> bool: + """Does this class list `typing.Protocol` (or `Protocol[T]`, or the `typing_extensions` one) among its bases? + + A class is only a protocol if it lists `Protocol` explicitly, so subclasses of a protocol + that do not are regular classes and still get mutated. The base is resolved through the + imports, so other classes named `Protocol` (`asyncio.Protocol`, ...) are not matched.""" + for base in node.bases: + value = base.value + if isinstance(value, cst.Subscript): + value = value.value + if isinstance(value, cst.Name) and value.value in self._protocol_names: + return True + if ( + isinstance(value, cst.Attribute) + and value.attr.value == "Protocol" + and isinstance(value.value, cst.Name) + and value.value.value in self._protocol_module_names + ): + return True + return False + + def _record_protocol_import(self, node: cst.CSTNode) -> None: + """Record the names under which `typing.Protocol` and its modules are imported.""" + if isinstance(node, cst.Import): + for alias in node.names: + if alias.evaluated_name in PROTOCOL_MODULES: + self._protocol_module_names.add(alias.evaluated_alias or alias.evaluated_name) + elif ( + isinstance(node, cst.ImportFrom) + and not node.relative + and node.module is not None + and get_full_name_for_node(node.module) in PROTOCOL_MODULES + ): + if isinstance(node.names, cst.ImportStar): + self._protocol_names.add("Protocol") + return + for alias in node.names: + if alias.evaluated_name == "Protocol": + self._protocol_names.add(alias.evaluated_alias or "Protocol") + + +PROTOCOL_MODULES = {"typing", "typing_extensions"} + def _walk(node: cst.CSTNode) -> Iterator[cst.CSTNode]: """Yield `node` and every node below it.""" diff --git a/tests/mutation/test_mutation.py b/tests/mutation/test_mutation.py index 0ff0d198..e69fc291 100644 --- a/tests/mutation/test_mutation.py +++ b/tests/mutation/test_mutation.py @@ -2,6 +2,7 @@ import pathlib import shutil import subprocess +import sys import tempfile from collections import defaultdict from collections.abc import Sequence @@ -1241,6 +1242,105 @@ def sum(self): assert mutants == [source.replace("self.x + self.y", "self.x - self.y")] +@pytest.mark.parametrize( + "imports, bases", + [ + ("from typing import Protocol", "Protocol"), + ("import typing", "typing.Protocol"), + ("import typing as t", "t.Protocol"), + ("from typing import Protocol as P", "P"), + ("import typing_extensions", "typing_extensions.Protocol"), + ("from typing_extensions import Protocol", "Protocol"), + ("from typing import *", "Protocol"), + ("from typing import Protocol", "Protocol[T]"), + ("import typing", "Generic[T], typing.Protocol[T]"), + ("from typing import Protocol", "Protocol, metaclass=Meta"), + ], +) +def test_do_not_mutate_protocol_classes(imports, bases): + # Mutating a method of a Protocol adds the generated mutant methods as members + # of the protocol, so implementations of it no longer conform (#590) + source = f""" +{imports} + +class Source({bases}): + def read(self, size: int = 10) -> str: ... + + def read_twice(self) -> str: + return self.read() + self.read() +""".strip() + + assert mutants_for_source(source) == [] + assert "xǁSourceǁ" not in mutated_module(source) + + +@pytest.mark.parametrize( + "source", + [ + pytest.param( + """ +from typing import Protocol, runtime_checkable + +@runtime_checkable +class Source(Protocol): + def read(self, size: int = 10) -> str: ... +""", + id="runtime_checkable", + ), + pytest.param( + """ +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from typing import Protocol +else: + Protocol = object + +class Source(Protocol): + def read(self, size: int = 10) -> str: ... +""", + id="type_checking_import", + ), + pytest.param( + """ +from typing import Protocol + +class Source[T](Protocol): + def read(self, size: int = 10) -> T: ... +""", + id="pep695_type_parameters", + marks=pytest.mark.skipif(sys.version_info < (3, 12), reason="PEP 695 syntax needs Python 3.12"), + ), + ], +) +def test_do_not_mutate_protocol_class_forms(source): + assert "xǁSourceǁ" not in mutated_module(source.strip()) + + +@pytest.mark.parametrize( + "imports, bases", + [ + # a subclass of a protocol that does not list Protocol itself is a regular class + ("from typing import Protocol\nclass Source(Protocol): ...", "Source"), + # other classes named Protocol are regular base classes + ("import asyncio", "asyncio.Protocol"), + ("from twisted.internet.protocol import Protocol", "Protocol"), + ("from .typing import Protocol", "Protocol"), + ], +) +def test_mutate_classes_that_are_not_protocols(imports, bases): + source = f""" +{imports} + +class File({bases}): + def read(self, size: int = 10) -> str: + return "x" * size +""".strip() + + assert mutants_for_source(source) + assert "xǁFileǁread__mutmut_orig" in mutated_module(source) + + def test_do_not_mutate_pattern_single_line(patch_config): source = 'logger.info("hello")' patch_config("do_not_mutate_patterns", [r"logger\.\w+\("])