astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
33.01k stars 1.1k forks source link

B901: Misses `yield` sub-expresisons #14453

Open MichaReiser opened 4 days ago

MichaReiser commented 4 days ago

A very contrived example but B901 misses yield expressions where they're not the expression of a ExprStmt.

def get_file_paths(file_types: Iterable[str] | None = None) -> Iterable[Path]:
    dir_path = Path(".")
    if file_types is None:
        return dir_path.glob("*")

    for file_type in file_types:
        (yield a for a in dir_path.glob(f"*.{file_type}"))

Note: we should make sure that we don't traverse into any lambda expressions...

AlexWaygood commented 4 days ago
def get_file_paths(file_types: Iterable[str] | None = None) -> Iterable[Path]:
    dir_path = Path(".")
    if file_types is None:
        return dir_path.glob("*")

    for file_type in file_types:
        (yield a for a in dir_path.glob(f"*.{file_type}"))

hmm, this isn't valid syntax:

>>> from typing import *
>>> from pathlib import Path
>>> def get_file_paths(file_types: Iterable[str] | None = None) -> Iterable[Path]:
...     dir_path = Path(".")
...     if file_types is None:
...         return dir_path.glob("*")
... 
...     for file_type in file_types:
...         (yield a for a in dir_path.glob(f"*.{file_type}"))
...         
  File "<python-input-2>", line 7
    (yield a for a in dir_path.glob(f"*.{file_type}"))
             ^^^
SyntaxError: invalid syntax
MichaReiser commented 4 days ago

hmm, I only tested with our parser. nested yield expressions are definitely valid :)

MichaReiser commented 4 days ago

Okay, found one

async def main():
    await (yield x)
AlexWaygood commented 4 days ago

Here's another function that is a generator function with a return statement and is not flagged by B901:

def f():
    x = yield
    print(x)
    return 42

This is a generator function that you can send values into:

>>> def f():
...     x = yield
...     print(x)
...     return 42
...     
>>> y = f()
>>> next(y)
>>> y.send('foo')
foo
Traceback (most recent call last):
  File "<python-input-13>", line 1, in <module>
    y.send('foo')
    ~~~~~~^^^^^^^
StopIteration: 42

Sending valued into a generator is, like returning values from a generator, quite an advanced feature. So if I saw a function like this in code review, I would assume the user knew what they were doing. That doesn't feel like a very principled reason not to emit B901 on it, though; maybe we should. Not sure.

rabelmervin commented 3 days ago

Hi @MichaReiser @AlexWaygood can I help you fix this issue ?

MichaReiser commented 3 days ago

Sure. You can find the code for this rule in this file

https://github.com/astral-sh/ruff/blob/b7e32b0a18487e8c9813e0d189d7a5d625ff5c3f/crates/ruff_linter/src/rules/flake8_bugbear/rules/return_in_generator.rs

rabelmervin commented 3 days ago

Thanks @MichaReiser excited to work on this !