Warn against using yield and return x

[untitaker]

Python 3 allows mixing of return-with-value and yield in the same function. I got bitten by this behavior multiple times, so I want to write a flake8 plugin for it. I wonder if I should add the lint to this package instead of making a separate one. What do you think?

[ambv]

As Victor and Xiang noted on the original bug, this is a new feature of Python and not a bug. In this sense, linting against this would for sure create false positives. Even in your pull request you don't really suggest what the user should do (note that all other messages have suggestions). And if you suggested users to not use return-with-value, then that's wrong. Consider def broken2 in your example. This is perfectly valid asyncio code:

def old_style_coroutine():
  data = yield from get_data()
  return data.some_value

How did this "bite" you?

[untitaker]

See the attached example in that bug. I wrote the return in an actual codebase (not intentionally using any async features) because I forgot that the function used yield further down, the function generated an empty sequence when the return was executed. This was completely surprising to me, I was used to Python 2 catching such errors with a SyntaxError. Of course it's valid code, but you can shoot yourself in the foot with it (I did at least, it took a while to debug this). That's why lints exist.

[ambv]

What we are trying to explain to you is that mere existence of yield/yield from and return-with-value in the body of a generator is not enough for a linter to generate a valid warning. In general, bugbear has two kinds of warnings now:

B0xx - downright bugs or design issues. No excuse having code like this.

B3xx - likely valid Python 2 code that will not work on Python 3. While these can have false positives, they would trip up a human reviewer in the same way.

Your proposed warning is in a different category of "it might sometimes be a problem for people unfamiliar with a new feature of the language". You would be better served type annotating your code and running a type checker against it. The generator annotation is specifically designed for the problem you are encountering, see here:

https://www.python.org/dev/peps/pep-0484/#annotating-generator-functions-and-coroutines

[untitaker]

I'm aware that this falls into a different category. It's a quite opinionated check, and whether it has a place in a linter simply depends on what kind of linter the author wants to make. If you don't want it, that's totally fine.

Regarding type annotations -- I might as well use an actually statically typed language :)

[ambv]

I was pondering adding a class of disabled-by-default guidance warnings. If you can figure out how we can introduce a warning that wouldn't be enabled by default unless specifically listed in your config, and creating a new class for them (say B9xx), I would be willing to merge your proposed warning.

[untitaker]

Happy to hear that! I suspect the best way to go about this are options, unless there's a Flake8-specific API I'm not aware of. @sigmavirus24 is there a way to do disabled-by-default warnings in Flake8 plugins?

[ambv]

pycodestyle (formerly pep8) does have a bunch of disabled-by-default warning codes:
https://pycodestyle.readthedocs.io/en/latest/intro.html#error-codes

Check out how they do it.

[untitaker]

pycodestyle is not a flake8 extension as far as I can see, i.e. flake8 hooks into pycodestyle, not the other way around. It comes with its own CLI and defines its own --ignore option there.

Anyway I updated my code in #3 by reading through flake8's sourcecode. I think the used methods are supposed to be used like that, but I'm not sure. I didn't find any documentation.

[sigmavirus24]

So Flake8 2.4+ (I think it's 2.4) allows you to annotate a check with an off_by_default attribute that you can set to True. 3.0+ allows you to extend the default ignore list directly instead of doing that. Depending on your compatibility matrix, depends on your solution. =)

Sorry for the delay, I'm not quite well.

[untitaker]

allows you to annotate a check with an off_by_default attribute that you can set to True

That appears to be per-plugin, we need a per-warning-code solution. Probably could still be used by defining multiple plugins + entrypoints in one package.

3.0+ allows you to extend the default ignore list directly instead of doing that.

You mean optmanager.extend_default_ignore? That's what I currently use.

So @ambv, do we need to support Flake8 < 3.0?

[untitaker]

Also @sigmavirus24 I respond to some mail after two months, so... which delay? 😆

Get well though.

[sigmavirus24]

That appears to be per-plugin, we need a per-warning-code solution. Probably could still be used by defining multiple plugins + entrypoints in one package.

Correct. Hacking presently uses it with multiple plugins in the one package.

You mean optmanager.extend_default_ignore? That's what I currently use.

I hadn't looked but that is exactly what I mean. =)

[ambv]

We don't need to support flake8 < 3.0, no.

[untitaker]

Cool, I bumped the requirement in setup.py to flake8 >= 3.0, now I think it's done.

[untitaker]

Hmm, flake8==3.0.4 and the latest flake8-bugbear apparently don't have this new check disabled by default! Sorry, I should've checked this before asking for review.

At first glance I can't see anything in flake8-bugbear that would be the cause of this.

[ambv]

Are you sure? This is not my experience:

$ flake8 tests/b901.py
tests/b901.py:6:1: E302 expected 2 blank lines, found 1
$ flake8 --select=B tests/b901.py
$ flake8 --select=B901 tests/b901.py
tests/b901.py:8:9: B901: Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.
tests/b901.py:35:5: B901: Using `yield` together with `return x`. Use native `async def` coroutines or put a `# noqa` comment on this line if this was intentional.

The one thing that DOESN'T work is that selecting B9 still filters out B901 (you have to explicitly name each warning):

$ flake8 --select=B9 tests/b901.py
$

[untitaker]

Yeah I'm a fool, forgot that I 1.) have a setup.cfg in my repo 2.) have a custom ignore option in there