False positive for E1111 (assignment-from-no-return)

[ronf]

Hello!

I'm running into a false positive for assignment-from-no-return when doing inheritance of a method where some instances of it raise an exception and others return a value.

This issue is not fixed on the preview release.

Steps to reproduce

The following example should demonstrate the problem:

"""Pylint Test"""

class Parent:
    """Parent class"""

    def compute(self):
        """This isn't supported by all child classes"""

        # pylint: disable=no-self-use
        raise ValueError('Not supported for this object')

    def test(self):
        """Test"""

        result = self.compute()
        return result

class Child(Parent):
    """Child class"""

    def compute(self):
        """This is supported for this child class"""

        return 42

Current behavior

************* Module pylint_test
/tmp/pylint_test.py:15:8: E1111: Assigning result of a function call, where the function has no return (assignment-from-no-return)

-----------------------------------
Your code has been rated at 4.44/10

Expected behavior

Since a value is returned in all cases except when an exception is raised, I would not expect this to return an error.

pylint --version output

pylint 2.4.3
astroid 2.3.2
Python 3.8.0 (default, Oct 15 2019, 17:41:05)
[Clang 11.0.0 (clang-1100.0.20.17)]

Thanks for a great tool, and for your time on this!

[PCManticore]

Thanks for the report. This particular check skips functions that don't return as long as they are "abstract", and by abstract it expects to be either marked with abc.abstractmethod or either raises a NotImplementedError. From a quick look, it seems NotImplementedError would fit more nicely with this function, which would also silence the linter in complaining.

[ronf]

In the real code, these methods are doing format conversions for keys into different formats, and I have a custom KeyExportError exception (currently derived from ValueError) that I raise when you ask for a conversion into a format a particular key type doesn't support. While I could catch and re-raise a NotImplementedError as a KeyExportError in the calling code, that's kind of ugly.

More generally, I can see this pattern applying in scenarios where the error isn't due to the method being abstract. As long as code being called in the assignment never returns without providing a value or triggering an exception, it seems to me this warning shouldn't fire. It would only be a plain "return" or falling off the end of a function without a "return" that I would expect to trigger this.

[PCManticore]

Alright, thank you for the argument, I could see the message being more useful if we drop that NotImplementedError constraint.

[ronf]

Thanks very much - this works nicely!

[earonesty]

@PCManticore Note: if you derive from NotImplementedError, because you want to identify which particular class is raising... it doesn't work. isinstance or issubclass should always be used!