False Positives for PYL-W0613

Feature Requests
1

Hi.

I’ve noticed that DS reports unused arguments (PYL-W0613), when a subclass overrides a method of a parent class. In this case, this is a false positive IMO as changing the signature can introduce issues when using polymorphism in your code. For example:

class Parent:
    def foo(self, arg: int) -> int:
        return arg + 7

    def bar(self, arg: int) -> int:
        return arg + 7

class Child(Parent):
    def foo(self) -> int:
        return 8

    def bar(self, arg: int) -> int:  # reported as PYL-W0613 by DS
        return 8

def my_function(seq: Sequence[Parent], arg: int) -> None:
    for e in seq:
        print(e.bar(arg=arg))  # Will run without problems
        print(e.foo(arg=arg))  # Will fail if any of the elements is a `Child` instance

Real-life examples are here.

I have reported these kinds of issues as false positives a number of times over the last 2 or three month. Since they keep popping up, I wanted to kindly ask if my reports have made it onto a roadmap :slight_smile:

Cheers!

2

Hey there,

You’re correct in that if you were to change your method structure in Child, it will break polymorphism. Particularly, this would break the Liskov Substitution Principle.

However, I think that PYL-W0613 is still a valid report for overridden functions, for two reasons:

  • Silencing PYL-W0613 for child methods can lead to false negatives. As in, there’s no guarantee that, if you haven’t used a variable in an overridden method, it means that it was supposed to be unused. For example:

    class Child(Parent):
    def send_data(self, data):
        logger.log("Data was sent to server")
        # ... But the data was never actually sent to server

    If DeepSource were to ignore PYL-W0613 for overridden methods, this legitimate issue will not be shown to the user.

  • Your example code can be mitigated in other ways, by introducing subtle changes to the API. For example:

    class Parent:
    def bar(self, *, arg: int = 0) -> int:
        return arg + 7

class Child(Parent):
    def bar(self) -> int:
        return 8

    If the way that the code is structured leads to unused arguments in a child method, then that means the API design has scope for change/improvement.

Note that this doesn’t mean that you need to abide by the way DeepSource tells you to structure code. If you decide to not change the class structure that you have now, I’d suggest you use # skipcq: PYL-W0613 comments on the function definitions that have unused arguments.

1 Like
3

Thanks for the quick reply!

Silcencing PYL-W0613 for child methods can lead to false negatives .

I see your point. The example failures that I pointed to come from a structure where

  • the parents methods has a relatively large number of parameters so that the child methods can use the if needed
  • many of the child methods don’t actually need all of them

The idea here is that our library specifies a general interface which users can give custom meaning to, that may rely on different parts of information. In this setting I also disagree with

If the way that the code is structured leads to unused arguments in a child method, then that means the API design has scope for change/improvement.

Maybe one could specify in Parent.bar to DS that all child classes overriding bar are allowed to leave arguments unused?

Your example code can be mitigated in other ways, by introducing subtle changes to the API.

I don’t quite see how your proposed change would solve the issue. Child().bar(arg=42) will still raise an exception and from my understanding making arg a kwarg-only argument does not fix the Liskov Substitution Principle.
Other tools (pylint, mypy) will indeed complain about this approach.

4

My mistake on that. I was cross-checking with Pylint when making that code example, and Pylint doesn’t raise W0613 on that issue. But yeah, that doesn’t really solve LSP.

That’s an interesting proposal. It is true that many people might want to structure their library in a similar way python-telegram-bot does things. As we improve the fidelity of # skipcq comments, I’ll make sure to bring this up in the discussion.

P.S. I am a big fan of the change python-telegram-bot made when they moved from the

def msg_handler(bot, update, ... any other passed args):

structure, to the fixed signature of:

def msg_handler(update, context):

I bring this up because a change like this to your interface could also mitigate the problem you’re facing. Just a passing comment from my side.

1 Like