[flake8-bugbear] Catch yield expressions within other statements (B901)
#21200
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
Hii @ntBre would appreciate your views on some design decisions for this PR ,
Thank you : ) |
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| B901 | 35 | 35 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
Yes, I think we should use a visitor like in B901. We can't fully replace B901 since it also applies to sync functions, but we should make sure we catch all of the async cases B901 handles. I think the error message looks good currently and matches CPython, but I think it should point to |
Signed-off-by: 11happy <soni5happy@gmail.com>
Signed-off-by: 11happy <soni5happy@gmail.com>
44b4bbe to
91d57b1
Compare
|
its ready for review : ) |
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I think we could simplify things slightly by following the B901 implementation more closely.
crates/ruff_linter/resources/test/fixtures/syntax_errors/return_in_generator.py
Outdated
Show resolved
Hide resolved
|
Oh, one other thing I forgot to mention: I think we need to make B901 only run on sync generators and this syntax error only run on async generators. Then we'll need to convert the syntax error into a B901 error for now to preserve backwards compatibility. |
Signed-off-by: 11happy <soni5happy@gmail.com>
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this is looking great. I just had a few more nits.
crates/ruff_linter/src/rules/flake8_bugbear/rules/return_in_generator.rs
Show resolved
Hide resolved
Signed-off-by: 11happy <bhuminjaysoni@gmail.com>
Signed-off-by: 11happy <bhuminjaysoni@gmail.com>
ntBre
left a comment
There was a problem hiding this comment.
Thank you, this is looking great! I just had one more nit about test placement, and I noticed a preexisting error that I think we can fix here too.
Signed-off-by: 11happy <bhuminjaysoni@gmail.com>
Signed-off-by: 11happy <bhuminjaysoni@gmail.com>
Signed-off-by: 11happy <bhuminjaysoni@gmail.com>
Signed-off-by: 11happy <bhuminjaysoni@gmail.com>
these test cases were labeled as `not_broken`, but pasting them into the REPL as
async functions shows that they are all syntax errors:
```py
>>> async def not_broken6():
... return (yield from [])
...
File "<python-input-1>", line 2
return (yield from [])
^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: 'return' with value in async generator
>>> async def not_broken7():
... x = yield from []
... return x
...
File "<python-input-3>", line 2
x = yield from []
^^^^^^^^^^^^^
SyntaxError: 'yield from' inside async function
>>> async def not_broken8():
... x = None
...
... def inner(ex):
... nonlocal x
... x = ex
...
... inner((yield from []))
... return x
...
...
File "<python-input-4>", line 8
inner((yield from []))
^^^^^^^^^^^^^
SyntaxError: 'yield from' inside async function
```
so I think it's correct to flag the sync versions too. I'll update the names in
the next commit
ntBre
left a comment
There was a problem hiding this comment.
Thanks! I pushed a few commits. Just waiting for CI and then I'll merge!
|
Hmm, there are actually 35 new ecosystem hits. They all look like true positives to me, and this is a preview rule, so I think it's okay to accept this, but @amyreese, do you know if code like this is common enough that we should allow it? @pytest.hookimpl(wrapper=True, tryfirst=True)
def pytest_collection(session: Session) -> Generator[None, object, object]:
config = session.config
with catch_warnings_for_item(
config=config, ihook=config.hook, when="collect", item=None
):
return (yield) # <--It popped up many times in It seems to me that the motivation for the rule applies whether the yield is a standalone expression or not. But we should probably include this in the release notes, either by splitting out the visitor change/bug fix or at least by updating the title and labels. For a bit more context, my suggestion to start flagging these was in #21200 (comment). |
|
I've literally never seen Since most of the ecosystem hits seem related to pytest hooks, I think it would be ok as-is — it's a really weird pattern and seems to negate most of the "purpose" of a generator function. I presume that pytest is doing something like sending a value back into a generator and then capturing that in the |
|
2.2k hits on github, some of which at least seem mildly more useful than what I saw in pytest: https://github.com/search?q=%22return+%28yield%29%22&type=code I can't imagine people are actually using the actual value from their yield expressions as a return value, probably more as shorthand for I guess as exceptions to a rule go, it's not crazy to allow it, just weird. |
|
Thanks! Sounds like it's probably reasonable to land this, especially since the rule is still in preview. I'll try to update the title and labels so that it shows up in the release notes. |
flake8-bugbear] Catch yield expressions within other statements (B901)
CodSpeed Performance ReportMerging #21200 will not alter performanceComparing Summary
|
|
Hmm, these benchmarks don't seem right. I'll try to merge main again. |
The pydantic benchmark seems to fairly regularly vary around 5% or so since we landed #21467, unfortunately :/ |
|
Hmm maybe I should have searched around a bit more. I stumbled across #14453 and #15101 and #15254 for unrelated reasons this afternoon. Apparently this change has already been considered and rejected. I still think it makes sense to flag these, including after checking the upstream implementation (#21200 (comment)), but I guess we should either revert the visitor changes (not the syntax error part) like #15254 or close #14453 as now completed. As I say in my other comment, we at least have to visit sub-expressions in the async case because that's a syntax error: >>> async def foo():
... return (yield 1)
...
File "<python-input-0>", line 2
return (yield 1)
^^^^^^^^^^^^^^^^
SyntaxError: 'return' with value in async generatorbut I think that bolsters the sync case too. cc @MichaReiser who reviewed the earlier PR |
Summary
This PR re-implements return-in-generator (B901) for async generators as a semantic syntax error. This is not a syntax error for sync generators, so we'll need to preserve both the lint rule and the syntax error in this case.
It also updates B901 and the new implementation to catch cases where the generator's
yieldoryield fromexpression is part of another statement, as in:These were previously not caught because we only looked for
Stmt::Expr(Expr::Yield)invisit_stmtinstead of visitingyieldexpressions directly. I think this modification is within the spirit of the rule and safe to try out since the rule is in preview.Test Plan
I have written tests as directed in #17412