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 01/12] 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 02/12] 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 03/12] 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 04/12] 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 05/12] 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 06/12] 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 07/12] 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 From 4c2cd7dcf63db8923162310dc069316fbd9920e8 Mon Sep 17 00:00:00 2001 From: Julfried Date: Fri, 18 Jul 2025 23:58:47 +0200 Subject: [PATCH 08/12] Check for subtype --- pylint/checkers/classes/class_checker.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index c988e78150..5ef796ebb5 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -2450,6 +2450,10 @@ def _are_return_types_compatible( if is_subclass_of(current_type, parent_type): return True + # Covariant check - current is subtype of parent + if current_type.is_subtype_of(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()} From bd2bd28d1736cc8d4dc3c39f4971f0f0bb0b8e21 Mon Sep 17 00:00:00 2001 From: Julfried Date: Sat, 19 Jul 2025 09:09:03 +0200 Subject: [PATCH 09/12] Add better handling for Literals --- pylint/checkers/classes/class_checker.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index 5ef796ebb5..6104062619 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -2437,6 +2437,7 @@ def _are_return_types_compatible( 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) + 4. current_type is a Literal containing values compatible with parent_type """ # Same type check if current_type.qname() == parent_type.qname(): @@ -2446,6 +2447,16 @@ def _are_return_types_compatible( if parent_type.qname() == "typing.Any": return True + # Special case: Hanndle constant Literal types + if current_type.qname() == ".Literal": + inferred = current_type.values() + for i in inferred: + # print("Inferred value:", i) + if isinstance(i, nodes.Const): + # If we have a Literal with a class, we can check its qname + if i.qname() == parent_type.qname(): + return True + # Covariant check - current is subclass of parent if is_subclass_of(current_type, parent_type): return True From 6ba53248727840eed4b55a6e2455ac714d487203 Mon Sep 17 00:00:00 2001 From: Julfried Date: Mon, 21 Jul 2025 22:28:16 +0200 Subject: [PATCH 10/12] Correctly call is_subtype_of --- pylint/checkers/classes/class_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index 6104062619..5797f2a398 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -2462,7 +2462,7 @@ def _are_return_types_compatible( return True # Covariant check - current is subtype of parent - if current_type.is_subtype_of(parent_type): + if current_type.is_subtype_of(parent_type.qname()): return True # For built-in types, also check by qualified name in ancestors From f09be72cbc24db2fb1c8adc35142fe6264105c99 Mon Sep 17 00:00:00 2001 From: Julfried Date: Mon, 21 Jul 2025 22:38:03 +0200 Subject: [PATCH 11/12] Fix typo --- pylint/checkers/classes/class_checker.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index 5797f2a398..c8cd2f6a0d 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -2447,7 +2447,7 @@ def _are_return_types_compatible( if parent_type.qname() == "typing.Any": return True - # Special case: Hanndle constant Literal types + # Special case: Handle constant Literal types if current_type.qname() == ".Literal": inferred = current_type.values() for i in inferred: From 7ed0659caacc016ad81dee6ee9eb1915a927af26 Mon Sep 17 00:00:00 2001 From: Julfried Date: Mon, 21 Jul 2025 22:49:16 +0200 Subject: [PATCH 12/12] Remove comment --- pylint/checkers/classes/class_checker.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pylint/checkers/classes/class_checker.py b/pylint/checkers/classes/class_checker.py index c8cd2f6a0d..1877dd7c2f 100644 --- a/pylint/checkers/classes/class_checker.py +++ b/pylint/checkers/classes/class_checker.py @@ -2451,7 +2451,6 @@ def _are_return_types_compatible( if current_type.qname() == ".Literal": inferred = current_type.values() for i in inferred: - # print("Inferred value:", i) if isinstance(i, nodes.Const): # If we have a Literal with a class, we can check its qname if i.qname() == parent_type.qname():