jendrikseipp / vulture

Find dead Python code
MIT License
3.38k stars 148 forks source link

Wrong processing of typing.Protocol #309

Open akhundMurad opened 1 year ago

akhundMurad commented 1 year ago

Suppose I have protocol class:

from typing import Protocol

class LoggerProtocol(Protocol):
    def log(
        self, level: int, msg: str, *args, extra: Mapping[str, object] | None = None
    ) -> None:
        ...

After execution of vulture I get the following output:

logging.py:24: unused variable 'args' (100% confidence)
logging.py:24: unused variable 'extra' (100% confidence)
logging.py:24: unused variable 'msg' (100% confidence)

In my opinion, vulture should ignore this type of "confidence", due to the nature of typing.Protocol

pm3512 commented 1 year ago

Hi, can I work on this?

jendrikseipp commented 1 year ago

Can't you just add del args etc. in the method body @akhundMurad ?

akhundMurad commented 1 year ago

@jendrikseipp It will make my code messy. Just imagine that each protocol method will have del statement in it...

jendrikseipp commented 1 year ago

Can you paste the content of the method to get a clearer picture for the use case?

Also, is it possible to change the method signature, i.e., to change msg to msg_ to signal that the argument is unused?

akhundMurad commented 1 year ago

https://github.com/akhundMurad/diator/blob/main/src/diator/middlewares/logging.py

Here you can see the full code. I added # noqa comment as a temporary fix.

jendrikseipp commented 1 year ago

Thanks! And is it possible to change the method signature, i.e., to change msg to msg_ to signal that the argument is unused?

akhundMurad commented 1 year ago

The purpose of the Protocol is to define the signature of class and its methods. Therefore, I cannot change the argument name

jendrikseipp commented 1 year ago

OK, thanks. Then the best solution would be to detect that this class inherits from Protocol and ignore the unused method arguments. Doing this cleanly will probably require adding scope information to Vulture, see #304. Until then, I don't think it makes sense to tackle this issue.

akhundMurad commented 1 year ago

Got it, thank you!