From 854bf5a8a929b428128e99eec7004b75ae49e175 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Mon, 26 May 2025 08:01:32 +0200 Subject: [PATCH 1/7] Add return type mismatch check for overridden methods --- pylint/checkers/classes/class_checker.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index b493a1ba73..13ebec287b 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -1510,6 +1510,27 @@ def _check_invalid_overridden_method( node=function_node, ) + # Check for return type mismatches + parent_returns = parent_function_node.returns + current_returns = function_node.returns + + # Only check if both methods have return type annotations + if parent_returns is not None and current_returns is not None: + parent_return_str = parent_returns.as_string() + current_return_str = current_returns.as_string() + + # Compare return type annotations as strings + if parent_return_str != current_return_str: + self.add_message( + "invalid-overridden-method", + args=( + function_node.name, + f"return type '{parent_return_str}'", + f"return type '{current_return_str}'", + ), + node=function_node, + ) + def _check_functools_or_not(self, decorator: nodes.Attribute) -> bool: if decorator.attrname != "cached_property": return False From 72bb0a8ddfb9ccfad250b6075dda29faf5ea2015 Mon Sep 17 00:00:00 2001 From: Julfried <51880314+Julfried@users.noreply.github.com> Date: Mon, 26 May 2025 08:02:15 +0200 Subject: [PATCH 2/7] Add a test case for this --- .../i/invalid/invalid_overridden_method.py | 14 ++++++++++++++ .../i/invalid/invalid_overridden_method.txt | 13 +++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/tests/functional/i/invalid/invalid_overridden_method.py b/tests/functional/i/invalid/invalid_overridden_method.py index d81a7882bd..6f006383a9 100644 --- a/tests/functional/i/invalid/invalid_overridden_method.py +++ b/tests/functional/i/invalid/invalid_overridden_method.py @@ -1,5 +1,6 @@ # pylint: disable=missing-docstring,too-few-public-methods,disallowed-name,invalid-name,unused-argument import abc +from io import TextIOWrapper, BytesIO class SuperClass(metaclass=abc.ABCMeta): @@ -125,3 +126,16 @@ def bar(self): # [invalid-overridden-method] @multiple_returns def bar2(self): # [invalid-overridden-method] return False + + +# Test case for return type mismatch from the issue +class BaseClass(abc.ABC): + @abc.abstractmethod + def read_file(self, path: str) -> TextIOWrapper: + """Abstract method that should return a TextIOWrapper.""" + raise NotImplementedError("Method must be implemented by subclass") + +class ChildClass(BaseClass): + def read_file(self, path: str) -> BytesIO: # [invalid-overridden-method] + """Implementation returns BytesIO instead of TextIOWrapper.""" + return BytesIO(b"content") diff --git a/tests/functional/i/invalid/invalid_overridden_method.txt b/tests/functional/i/invalid/invalid_overridden_method.txt index 5506109df0..4fe3b11b67 100644 --- a/tests/functional/i/invalid/invalid_overridden_method.txt +++ b/tests/functional/i/invalid/invalid_overridden_method.txt @@ -1,6 +1,7 @@ -invalid-overridden-method:38:4:38:12:InvalidDerived.prop:Method 'prop' was expected to be 'property', found it instead as 'method':UNDEFINED -invalid-overridden-method:41:4:41:20:InvalidDerived.async_method:Method 'async_method' was expected to be 'async', found it instead as 'non-async':UNDEFINED -invalid-overridden-method:45:4:45:16:InvalidDerived.method_a:Method 'method_a' was expected to be 'method', found it instead as 'property':UNDEFINED -invalid-overridden-method:48:4:48:22:InvalidDerived.method_b:Method 'method_b' was expected to be 'non-async', found it instead as 'async':UNDEFINED -invalid-overridden-method:122:4:122:11:B.bar:Method 'bar' was expected to be 'property', found it instead as 'method':UNDEFINED -invalid-overridden-method:126:4:126:12:B.bar2:Method 'bar2' was expected to be 'property', found it instead as 'method':UNDEFINED +invalid-overridden-method:39:4:39:12:InvalidDerived.prop:Method 'prop' was expected to be 'property', found it instead as 'method':UNDEFINED +invalid-overridden-method:42:4:42:20:InvalidDerived.async_method:Method 'async_method' was expected to be 'async', found it instead as 'non-async':UNDEFINED +invalid-overridden-method:46:4:46:16:InvalidDerived.method_a:Method 'method_a' was expected to be 'method', found it instead as 'property':UNDEFINED +invalid-overridden-method:49:4:49:22:InvalidDerived.method_b:Method 'method_b' was expected to be 'non-async', found it instead as 'async':UNDEFINED +invalid-overridden-method:123:4:123:11:B.bar:Method 'bar' was expected to be 'property', found it instead as 'method':UNDEFINED +invalid-overridden-method:127:4:127:12:B.bar2:Method 'bar2' was expected to be 'property', found it instead as 'method':UNDEFINED +invalid-overridden-method:139:4:139:17:ChildClass.read_file:Method 'read_file' was expected to be "return type 'TextIOWrapper'", found it instead as "return type 'BytesIO'":UNDEFINED From e95a16c42bb012e83c683a1e6b6adcd10cd2b9af Mon Sep 17 00:00:00 2001 From: Julfried Date: Sun, 29 Jun 2025 23:43:58 +0200 Subject: [PATCH 3/7] Improve return type check --- pylint/checkers/classes/class_checker.py | 46 +++++++++++++++++------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index 13ebec287b..def92d9776 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -33,6 +33,7 @@ is_iterable, is_property_setter, is_property_setter_or_deleter, + is_subclass_of, node_frame_class, only_required_for_messages, safe_infer, @@ -1516,20 +1517,39 @@ def _check_invalid_overridden_method( # Only check if both methods have return type annotations if parent_returns is not None and current_returns is not None: - parent_return_str = parent_returns.as_string() - current_return_str = current_returns.as_string() + parent_return_type = safe_infer(parent_returns) + current_return_type = safe_infer(current_returns) - # Compare return type annotations as strings - if parent_return_str != current_return_str: - self.add_message( - "invalid-overridden-method", - args=( - function_node.name, - f"return type '{parent_return_str}'", - f"return type '{current_return_str}'", - ), - node=function_node, - ) + compatible = False + + if ( + parent_return_type + and current_return_type + and isinstance(parent_return_type, nodes.ClassDef) + and isinstance(current_return_type, nodes.ClassDef) + ): + # Same type check + if parent_return_type.qname() == current_return_type.qname(): + compatible = True + # Covariant check - current is subclass of parent + elif ( + isinstance(current_return_type, nodes.ClassDef) + and isinstance(parent_return_type, nodes.ClassDef) + and is_subclass_of(current_return_type, parent_return_type) + ): + compatible = True + + # Emit error if not compatible + if not compatible: + self.add_message( + "invalid-overridden-method", + args=( + function_node.name, + f"return type '{parent_return_type.name}'", + f"return type '{current_return_type.name}'", + ), + node=function_node, + ) def _check_functools_or_not(self, decorator: nodes.Attribute) -> bool: if decorator.attrname != "cached_property": From e75805c926fc1d03ca1ff8e92904efc6695512dd Mon Sep 17 00:00:00 2001 From: Julfried Date: Wed, 2 Jul 2025 23:09:06 +0200 Subject: [PATCH 4/7] Refactor return type compatibility check in class methods to now also correctly handle Any as a Return type --- pylint/checkers/classes/class_checker.py | 45 +++++++++++++++++------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index def92d9776..c988e78150 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -1520,24 +1520,16 @@ def _check_invalid_overridden_method( parent_return_type = safe_infer(parent_returns) current_return_type = safe_infer(current_returns) - compatible = False - if ( parent_return_type and current_return_type and isinstance(parent_return_type, nodes.ClassDef) and isinstance(current_return_type, nodes.ClassDef) ): - # Same type check - if parent_return_type.qname() == current_return_type.qname(): - compatible = True - # Covariant check - current is subclass of parent - elif ( - isinstance(current_return_type, nodes.ClassDef) - and isinstance(parent_return_type, nodes.ClassDef) - and is_subclass_of(current_return_type, parent_return_type) - ): - compatible = True + # Check for compatibility + compatible = self._are_return_types_compatible( + current_return_type, parent_return_type + ) # Emit error if not compatible if not compatible: @@ -2097,6 +2089,7 @@ def _check_accessed_members( node.local_attr(attr) # yes, stop here continue + except astroid.NotFoundError: pass # is it an instance attribute of a parent class ? @@ -2434,6 +2427,34 @@ def _check_signature( "signature-differs", args=(class_type, method1.name), node=method1 ) + def _are_return_types_compatible( + self, current_type: nodes.ClassDef, parent_type: nodes.ClassDef + ) -> bool: + """Check if current_type is compatible with parent_type for return type + checking. + + Compatible means: + 1. Same type (qname match) + 2. current_type is a subclass of parent_type (covariance) + 3. parent_type is typing.Any (any type is compatible with Any) + """ + # Same type check + if current_type.qname() == parent_type.qname(): + return True + + # Special case: Any type accepts anything + if parent_type.qname() == "typing.Any": + return True + + # Covariant check - current is subclass of parent + if is_subclass_of(current_type, parent_type): + return True + + # For built-in types, also check by qualified name in ancestors + parent_qname = parent_type.qname() + current_ancestors = {ancestor.qname() for ancestor in current_type.ancestors()} + return parent_qname in current_ancestors + def _uses_mandatory_method_param( self, node: nodes.Attribute | nodes.Assign | nodes.AssignAttr ) -> bool: From 63ea2fd547ddd4521d6dcee7d9ba9dcdd3aa930d Mon Sep 17 00:00:00 2001 From: Julfried Date: Thu, 3 Jul 2025 00:18:23 +0200 Subject: [PATCH 5/7] Add newsfragment --- doc/whatsnew/fragments/10351.false_negative | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 doc/whatsnew/fragments/10351.false_negative diff --git a/doc/whatsnew/fragments/10351.false_negative b/doc/whatsnew/fragments/10351.false_negative new file mode 100644 index 0000000000..d6c2e22413 --- /dev/null +++ b/doc/whatsnew/fragments/10351.false_negative @@ -0,0 +1,5 @@ +Enhanced `invalid-overridden-method` checker to detect return type mismatches when overriding methods. +The checker now properly identifies when subclass methods have incompatible return types compared to their parent methods, +including support for covariant return types and proper `typing.Any` handling. + +Closes #10351 From 1ca1d5b1a9e370ac4644ae6ad8486e55882a6e03 Mon Sep 17 00:00:00 2001 From: Julfried Date: Thu, 3 Jul 2025 00:43:17 +0200 Subject: [PATCH 6/7] Update comments in test case --- tests/functional/i/invalid/invalid_overridden_method.py | 3 ++- tests/functional/i/invalid/invalid_overridden_method.txt | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/functional/i/invalid/invalid_overridden_method.py b/tests/functional/i/invalid/invalid_overridden_method.py index 6f006383a9..259c91551a 100644 --- a/tests/functional/i/invalid/invalid_overridden_method.py +++ b/tests/functional/i/invalid/invalid_overridden_method.py @@ -128,7 +128,8 @@ def bar2(self): # [invalid-overridden-method] return False -# Test case for return type mismatch from the issue +# https://github.com/pylint-dev/pylint/issues/10351 +# Test case for return type mismatch class BaseClass(abc.ABC): @abc.abstractmethod def read_file(self, path: str) -> TextIOWrapper: diff --git a/tests/functional/i/invalid/invalid_overridden_method.txt b/tests/functional/i/invalid/invalid_overridden_method.txt index 4fe3b11b67..06de3e8ab7 100644 --- a/tests/functional/i/invalid/invalid_overridden_method.txt +++ b/tests/functional/i/invalid/invalid_overridden_method.txt @@ -4,4 +4,4 @@ invalid-overridden-method:46:4:46:16:InvalidDerived.method_a:Method 'method_a' w invalid-overridden-method:49:4:49:22:InvalidDerived.method_b:Method 'method_b' was expected to be 'non-async', found it instead as 'async':UNDEFINED invalid-overridden-method:123:4:123:11:B.bar:Method 'bar' was expected to be 'property', found it instead as 'method':UNDEFINED invalid-overridden-method:127:4:127:12:B.bar2:Method 'bar2' was expected to be 'property', found it instead as 'method':UNDEFINED -invalid-overridden-method:139:4:139:17:ChildClass.read_file:Method 'read_file' was expected to be "return type 'TextIOWrapper'", found it instead as "return type 'BytesIO'":UNDEFINED +invalid-overridden-method:140:4:140:17:ChildClass.read_file:Method 'read_file' was expected to be "return type 'TextIOWrapper'", found it instead as "return type 'BytesIO'":UNDEFINED From a76ccc57536245b2af61bc78ea8c7b24a7003e4a Mon Sep 17 00:00:00 2001 From: Julfried Date: Thu, 3 Jul 2025 00:50:25 +0200 Subject: [PATCH 7/7] Try to increase test coverage --- .../functional/i/invalid/invalid_overridden_method.py | 10 +++++++++- .../functional/i/invalid/invalid_overridden_method.txt | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/tests/functional/i/invalid/invalid_overridden_method.py b/tests/functional/i/invalid/invalid_overridden_method.py index 259c91551a..9922849d6b 100644 --- a/tests/functional/i/invalid/invalid_overridden_method.py +++ b/tests/functional/i/invalid/invalid_overridden_method.py @@ -130,13 +130,21 @@ def bar2(self): # [invalid-overridden-method] # https://github.com/pylint-dev/pylint/issues/10351 # Test case for return type mismatch +class ReturnType(TextIOWrapper): + pass + class BaseClass(abc.ABC): @abc.abstractmethod def read_file(self, path: str) -> TextIOWrapper: """Abstract method that should return a TextIOWrapper.""" raise NotImplementedError("Method must be implemented by subclass") -class ChildClass(BaseClass): +class ChildClass1(BaseClass): def read_file(self, path: str) -> BytesIO: # [invalid-overridden-method] """Implementation returns BytesIO instead of TextIOWrapper.""" return BytesIO(b"content") + +class ChildClass2(BaseClass): + def read_file(self, path: str) -> ReturnType: + """Implementation returns BytesIO instead of TextIOWrapper.""" + return ReturnType(BytesIO(b"content")) diff --git a/tests/functional/i/invalid/invalid_overridden_method.txt b/tests/functional/i/invalid/invalid_overridden_method.txt index 06de3e8ab7..edc5bb5af2 100644 --- a/tests/functional/i/invalid/invalid_overridden_method.txt +++ b/tests/functional/i/invalid/invalid_overridden_method.txt @@ -4,4 +4,4 @@ invalid-overridden-method:46:4:46:16:InvalidDerived.method_a:Method 'method_a' w invalid-overridden-method:49:4:49:22:InvalidDerived.method_b:Method 'method_b' was expected to be 'non-async', found it instead as 'async':UNDEFINED invalid-overridden-method:123:4:123:11:B.bar:Method 'bar' was expected to be 'property', found it instead as 'method':UNDEFINED invalid-overridden-method:127:4:127:12:B.bar2:Method 'bar2' was expected to be 'property', found it instead as 'method':UNDEFINED -invalid-overridden-method:140:4:140:17:ChildClass.read_file:Method 'read_file' was expected to be "return type 'TextIOWrapper'", found it instead as "return type 'BytesIO'":UNDEFINED +invalid-overridden-method:143:4:143:17:ChildClass1.read_file:Method 'read_file' was expected to be "return type 'TextIOWrapper'", found it instead as "return type 'BytesIO'":UNDEFINED