Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

B901: Misses yield sub-expresisons #14453

Open
MichaReiser opened this issue Nov 19, 2024 · 7 comments · May be fixed by #15254
Open

B901: Misses yield sub-expresisons #14453

MichaReiser opened this issue Nov 19, 2024 · 7 comments · May be fixed by #15254
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Nov 19, 2024

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...

@MichaReiser MichaReiser added bug Something isn't working good first issue Good for newcomers labels Nov 19, 2024
@AlexWaygood
Copy link
Member

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
Copy link
Member Author

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

@MichaReiser
Copy link
Member Author

Okay, found one

async def main():
    await (yield x)

@AlexWaygood
Copy link
Member

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
Copy link

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

@MichaReiser
Copy link
Member Author

@rabelmervin
Copy link

Thanks @MichaReiser excited to work on this !

kaspell added a commit to kaspell/ruff that referenced this issue Jan 4, 2025
Currently, the B901 rule misses yield expressions that are not
top-of-tree, for example as in

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

This commit refactors the rule to find such yield expressions.
Assignments are traversed and identifiers bound to yield or
yield from expressions are tracked, so that if those variables are
later returned (which is valid), the rule is not triggered.

The assignment traversal part is inspired by the match_value and
match_target functions from src/analyze/typing.rs in the
ruff_python_semantic crate.

The relevant issue is astral-sh#14453.
kaspell added a commit to kaspell/ruff that referenced this issue Jan 4, 2025
Currently, the B901 rule misses yield expressions that are not
top-of-tree, for example as in

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

This commit refactors the rule to find such yield expressions.
Assignments are traversed and identifiers bound to yield or
yield from expressions are tracked, so that if the value bound
to those identifiers are later returned (which is valid), the rule
is not triggered.

The assignment traversal part is inspired by the match_value and
match_target functions from src/analyze/typing.rs in the
ruff_python_semantic crate.

The relevant issue is astral-sh#14453.
kaspell added a commit to kaspell/ruff that referenced this issue Jan 4, 2025
Currently, the B901 rule misses yield expressions that are not
top-of-tree in a function body, for example as in

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

This commit refactors the rule to find such yield expressions.
Assignments are traversed and identifiers bound to yield or
yield from expressions are tracked, so that if a value bound to
those identifiers is later returned (which is valid), the rule
is not triggered.

The assignment traversal part is inspired by the match_value and
match_target functions from src/analyze/typing.rs in the
ruff_python_semantic crate.

The relevant issue is astral-sh#14453.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
3 participants