Suppress useless-super-delegation if return type changed (#5822)

[timmartin]

Type of Changes

	Type
✓	🐛 Bug fix

Description

In the useless-super-delegation checker, check whether the return type annotation has been specialized in the derived class. If so, then the override is not useless and we should not issue the warning.

Closes #5822

[coveralls]

Pull Request Test Coverage Report for Build 2171288819

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• 13 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.003%) to 93.379%

---

Files with Coverage Reduction	New Missed Lines	%
pylint/lint/pylinter.py	13	95.89%

Totals
Change from base Build 2169544822:	0.003%
Covered Lines:	15979
Relevant Lines:	17112

---

💛 - Coveralls

[DanielNoord]

This annotation is incorrect, is that intentional?

[timmartin]

Well spotted, that wasn't intentional.

While I was fixing that, I realized there's another case we need to think about:

import typing
from typing import Any

class Base:
    def foo(self) -> Any:
        pass

class Derived:
    def foo(self) -> typing.Any:
        return super().foo()

This will be harder to fix, and it looks like the existing rules around type hints on parameters don't cope with this case either, so I suggest this can be handled as a later fix to both parameter type hints and return type hint.

[Pierre-Sassoulas]

LGTM.

[DanielNoord]

@cdce8p I'd like to get your input here. I'm not sure if this change is reasonable. We're not a type checker, but should we support this? You're probably the best person to judge this.

[tushar-deepsource]

I'd say this is a good change. It's a change that will lead to less false positives, and it's something that we can infer, so why not take advantage of it?

[Pierre-Sassoulas]

@cdce8p could you check this, if you have some time ? It's blocking the release of 2.13.6 :)

[cdce8p]

I'm not fully convinced it is all that useful, but I guess we can add it. Especially since we do something similar for the parameter annotations already.

Just two suggestions:

• Do not return if one function doesn't have a return type (similar to parameter annotations).
• Move it below the if called_annotations and overridden_annotations block.

[timmartin]

I'm not sure what you mean by "Do not return if one function doesn't have a return type (similar to parameter annotations)." Currently my reasoning is that if the derived method adds a type missing from the base method we should not warn useless-super-delegation, but if the base method has the annotation and the derived method doesn't then we should warn. Is that what you're meaning?

[cdce8p]

I'm not sure what you mean by "Do not return if one function doesn't have a return type (similar to parameter annotations)." Currently my reasoning is that if the derived method adds a type missing from the base method we should not warn useless-super-delegation, but if the base method has the annotation and the derived method doesn't then we should warn. Is that what you're meaning?

My idea was to be careful not to block too many errors. I.e. my suggestion would be to only ignore useless-super-delegation if both the parent and child method have function annotations and they differ.

[cdce8p]

Please don't force push changes new here. That will make it difficult / impossible to see and compare old revisions.

[cdce8p]

Suggested change

	if (
	function.returns is not None
	and meth_node.returns is None
	or meth_node.returns.as_string() != function.returns.as_string()
	):
	if function.returns is not None and (
	meth_node.returns is None
	or meth_node.returns.as_string() != function.returns.as_string()
	):

To fix the tests, you would need to use the old condition.

For my #6141 (comment), I suggested something a little bit different.

Suggested change

	if (
	function.returns is not None
	and meth_node.returns is None
	or meth_node.returns.as_string() != function.returns.as_string()
	):
	if (
	function.returns is not None
	and meth_node.returns is not None
	and meth_node.returns.as_string() != function.returns.as_string()
	):

[timmartin]

My thinking was that the most likely reason I could think to override a method just to change the return type annotation is that I was extending some library code that doesn't have type declarations and I wanted my library to have type declarations, in which case issuing a diagnostic isn't helpful. I'm not sure I care enough about this to prolong the discussion, though.

[cdce8p]

My thinking was that the most likely reason I could think to override a method just to change the return type annotation is that I was extending some library code that doesn't have type declarations and I wanted my library to have type declarations, in which case issuing a diagnostic isn't helpful. I'm not sure I care enough about this to prolong the discussion, though.

Usually, I think it would be better if the original library added type hints itself. However, I do agree that it's not worth the discussion. If you think it would be helpful, I guess we could do that, too. Will leave that up to you.

[timmartin]

No, I'm done with this change. Merge it or close the PR as you see fit.

[Pierre-Sassoulas]

Thank you for the fix @timmartin !