jendrikseipp / vulture

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

Allow unreachable trailing `yield`, to make an abstract generator method #298

Open Zac-HD opened 1 year ago

Zac-HD commented 1 year ago

Consider the following example, where an abstract method is a generator:

class AbstractFoo(ABC):
    @abstractmethod
    def some_generator(self):
        raise NotImplementedError
        yield

We want to raise NotImplementedError to prevent accidental calls, but then Vulture errors out because the yield is unreachable. Indeed it is unreachable - but we need the yield statement there to make the method into a generator!

Decent solutions might include supporting some way to ignore unreachable-code warnings (as is possible for unused variables), or more direct heuristics like "ignore an unreachable bare yield if there are no following statements and only a raise statement preceding" - I'm not attached to any particular solution, but would love something to unblock this so I can enforce Vulture rather than just running it by hand occasionally 🙂

kreathon commented 1 year ago

Just randomly reading this.

but we need the yield statement there to make the method into a generator!

May I ask what the yield makes for a difference? So where does it break if it is missing? Runtime, type checking, other static checkers or just that the developer knows about it?

I am just asking such that I can think about other possible solutions.

Zac-HD commented 1 year ago

Runtime (e.g. inspect.isgeneratorfunction() as well as static typechecking. I guess it's also useful for human readers but the runtime requirement is the hard one.

kreathon commented 1 year ago

I agree that that type checker and code readability can probably easily be managed (type annotation, custom decorator, custom exception).

For runtime checks I thought about the following:

class AbstractFoo(ABC):
    @abstractmethod
    def some_generator(self):
        yield from GeneratorNotImplemented()

where GeneratorNotImplemented is something like

class GeneratorNotImplemented:
    def __init__(self):
        raise NotImplementedError

However, I guess it it less readable than the original code.

Sorry, if this goes into a wrong direction for you. I am not maintainer of this project.

Bye the way, the more I think about it, the more sense it makes to me to not mark them as unreachable yields (similar to yield followed by a return to create an empty iterator). Because the yield was put there on purpose and changes the behavior of the program.

I also checked mypy and PyCharm both of them will mark the code as unreachable (like vulture).

sanmai-NL commented 1 year ago

Don't raise NotImplementedError (class) but NotImplementedError() (instance).

jakkdl commented 1 year ago

It's not documented in the README, but you can suppress unreachable-code errors for a line with noqa: V201. Vulture should perhaps handle this case automatically though.

jfcherng commented 1 month ago

Here's another use case. I don't force a child class to implement the method and I have a default empty one.

    def my_generator(self) -> Generator[str, None, None]:
        return
        yield  # I have to add "noqa: V201"