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

Implement more flake8-bugbear opinionated rules #3758

Open
5 of 8 tasks
sjdemartini opened this issue Mar 27, 2023 · 14 comments · Fixed by #9680
Open
5 of 8 tasks

Implement more flake8-bugbear opinionated rules #3758

sjdemartini opened this issue Mar 27, 2023 · 14 comments · Fixed by #9680
Labels
rule Implementing or modifying a lint rule

Comments

@sjdemartini
Copy link
Contributor

sjdemartini commented Mar 27, 2023

Picking up where #2954 left off, there were a few opinionated (B9xx flake8-bugbear rules) checks left to be implemented in Ruff:

  • B901: Using return x in a generator function. (Somewhat bad reasoning in flake8-bugbear description, talks about Python 2, see comment here Update bugbear #2954 (comment) around its utility.)
  • B902: Implemented as N804 and N805.
  • B903: Use collections.namedtuple (or typing.NamedTuple) for data classes that only set attributes in an __init__ method, and do nothing else. (Probably should include dataclasses recommendation? NamedTuple injects extra tuple methods and is meant for backward compat, not a data class replacement. That's dataclasses nowadays.)
  • B906: visit_ function with no further call to a visit function.
  • B907: Consider replacing f"'{foo}'" with f"{foo!r}" which is both easier to read and will escape quotes inside foo if that would appear.
  • B908: Partly implemented as PT012.

There's an open question on how these should be included, since it would deviate from flake8-bugbear to have these on by default just by turning on the rest of the bugbear rules (see comment #2954 (comment)).

There's also one outstanding non-opinionated rule:

  • B036: Found except BaseException: without re-raising (no raise in the top-level of the except block). This catches all kinds of things (Exception, SystemExit, KeyboardInterrupt...) and may prevent a program from exiting as expected. Implemented as BLE001.
  • B038: Renamed to B909 in bugbear; implemented as B909 is Ruff.
@sjdemartini sjdemartini mentioned this issue Mar 27, 2023
9 tasks
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Mar 27, 2023
@Pierre-Sassoulas
Copy link
Contributor

B906 is flake8 plugins specific and has a false positive when the visit function is visiting an astroid node in a pylint plugin. It would be nice to not have this FP in ruff :)

@charliermarsh charliermarsh added the accepted Ready for implementation label Jul 10, 2023
@mikaelarguedas
Copy link
Contributor

mikaelarguedas commented Jan 6, 2024

Since then some new rule apppeared

  • B907: (previous B028) Consider replacing f"'{foo}'" with f"{foo!r}" which is both easier to read and will escape quotes inside foo if that would appear. The check tries to filter out any format specs that are invalid together with !r. If you're using other conversion flags then e.g. f"'{foo!a}'" can be replaced with f"{ascii(foo)!r}". Not currently implemented for python<3.8 or str.format() calls.
  • B908: (looks like PT012 to me): Contexts with exceptions assertions like with self.assertRaises or with pytest.raises should not have multiple top-level statements. Each statement should be in its own context. That way, the test ensures that the exception is raised only in the exact statement where you expect it.

@charliermarsh charliermarsh added good first issue Good for newcomers help wanted Contributions especially welcome and removed accepted Ready for implementation labels Jan 6, 2024
@charliermarsh
Copy link
Member

Good first issues for anyone interested :)

@mikaelarguedas
Copy link
Contributor

mikaelarguedas commented Jan 14, 2024

Another 2 rules appeared in flake8-bugbear:

  • B037: Found return <value>, yield, yield <value>, or yield from <value> in class __init__() method. No values should be returned or yielded, only bare returns are ok. (PLE0100)

  • B038: Found a mutation of a mutable loop iterable inside the loop body. Changes to the iterable of a loop such as calls to list.remove() or via del can cause unintended bugs. Rule: Flake8-bugbear B038 " detect changes to iterable object of loop" #9511

@Skylion007
Copy link
Contributor

B037 is already implemented as PLE0100, @charliermarsh maybe we should consider migrating PLE0100 to B037 though?

@charliermarsh
Copy link
Member

@Skylion007 - Yes good call -- I prefer indexing under bugbear over Pylint since it's more popular for Ruff users. I'll add a note to the 0.2.0 release list.

@charliermarsh charliermarsh mentioned this issue Jan 14, 2024
13 tasks
@MichaReiser MichaReiser added this to the v0.2.0 milestone Jan 19, 2024
@zanieb zanieb mentioned this issue Jan 30, 2024
@mikaelarguedas
Copy link
Contributor

@zanieb Was this closed on purpose ?

I'm under the impression that some rules listed here are not part of ruff yet:
B036, B038, B901, B902, B903, B907, B908

@charliermarsh
Copy link
Member

@mikaelarguedas - I think it was an oversight, but I'll leave it to @zanieb to confirm in the AM.

@mscheifer
Copy link

I think B036 is covered/aliased by BLE001.

@mikaelarguedas
Copy link
Contributor

@charliermarsh @zanieb friendly 🛎️ if it was closed by mistake, is it possible to reopen and update according to current state ?

Current state:
B036 -> covered by BLE001
B038 -> no implemented
B901 -> no implemented
B902 ->no implemented
B903 ->no implemented
B907 ->no implemented
B908 ->no implemented

@charliermarsh charliermarsh reopened this May 8, 2024
@charliermarsh
Copy link
Member

Will update the list, thanks.

@charliermarsh
Copy link
Member

Updated. We do actually have B038 (which bugbear moved to B909).

charliermarsh added a commit that referenced this issue May 31, 2024
## Summary

This PR implements the rule B901, which is part of the opinionated rules
of `flake8-bugbear`.

This rule seems to be desired in `ruff` as per
#3758 and
#2954 (comment).

## Test Plan

As this PR was made closely following the
[CONTRIBUTING.md](https://github.com/astral-sh/ruff/blob/8a25531a7144fd4a6b62c54efde1ef28e2dc18c4/CONTRIBUTING.md),
it tests using the snapshot approach, that is described there.

## Sources

The implementation is inspired by [the original implementation in the
`flake8-bugbear`
repository](https://github.com/PyCQA/flake8-bugbear/blob/d1aec4cbef7c4a49147c428b7e4a97e497b5d163/bugbear.py#L1092).
The error message and [test
file](https://github.com/PyCQA/flake8-bugbear/blob/d1aec4cbef7c4a49147c428b7e4a97e497b5d163/tests/b901.py)
where also copied from there.

The documentation I came up with on my own and needs improvement. Maybe
the example given in
#2954 (comment)
could be used, but maybe they are too complex, I'm not sure.

## Open Questions

- [ ] Documentation. (See above.)

- [x] Can I access the parent in a visitor?

The [original
implementation](https://github.com/PyCQA/flake8-bugbear/blob/d1aec4cbef7c4a49147c428b7e4a97e497b5d163/bugbear.py#L1100)
references the `yield` statement's parent to check if it is an
expression statement. I didn't find a way to do this in `ruff` and used
the `is_expresssion_statement` field on the visitor instead. What are
your thoughts on this? Is it possible and / or desired to access the
parent node here?

- [x] Is `Option::is_some(...)` -> `...unwrap()` the right thing to do?

Referring to [this piece of
code](https://github.com/tobb10001/ruff/blob/9d5a280f71103ef33df5676d00a6c68c601261ac/crates/ruff_linter/src/rules/flake8_bugbear/rules/return_x_in_generator.rs?plain=1#L91-L96).
From my understanding, the `.unwrap()` is safe, because it is checked
that `return_` is not `None`. However, I feel like I missed a more
elegant solution that does both in one.

## Other

I don't know a lot about this rule, I just implemented it because I
found it in a
https://github.com/astral-sh/ruff/labels/good%20first%20issue.

I'm new to Rust, so any constructive critisism is appreciated.

---------

Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
@dhruvmanila dhruvmanila removed this from the v0.2.0 milestone Jun 29, 2024
@jnrbsn
Copy link

jnrbsn commented Aug 23, 2024

This issue says that B902 is "Implemented as N804 and N805", but that is not accurate because N804 incorrectly says you should use cls for "metaclass class methods" (e.g. __new__, __prepare__). These methods are the metaclass-equivalent of class methods and get passed the metaclass itself as the first argument. flake8-bugbear recommends metacls, and pylint recommends mcs (which flake8-bugbear will also accept).

@gagandeepp
Copy link

Interesting task,can I help?

@charliermarsh charliermarsh removed good first issue Good for newcomers help wanted Contributions especially welcome labels Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants