Implements [flake8-commas](https://github.com/PyCQA/flake8-commas). Fixes#1058.
The plugin is mostly redundant with Black (and also deprecated upstream), but very useful for projects which can't/won't use an auto-formatter.
This linter works on tokens. Before porting to Rust, I cleaned up the Python code ([link](https://gist.github.com/bluetech/7c5dcbdec4a73dd5a74d4bc09c72b8b9)) and made sure the tests pass. In the Rust version I tried to add explanatory comments, to the best of my understanding of the original logic.
Some changes I did make:
- Got rid of rule C814 - "missing trailing comma in Python 2". Ruff doesn't support Python 2.
- Merged rules C815 - "missing trailing comma in Python 3.5+" and C816 - "missing trailing comma in Python 3.6+" into C812 - "missing trailing comma". These Python versions are outdated, didn't think it was worth the complication.
- Added autofixes for C812 and C819.
Autofix is missing for C818 - "trailing comma on bare tuple prohibited". It needs to turn e.g. `x = 1,` into `x = (1, )`, it's a bit difficult to do with tokens only, so I skipped it for now.
I ran the rules on cpython/Lib and on a big internal code base and it works as intended (though I only sampled the diffs).
The primary motivation is that we can now robustly detect `\` continuations due to the addition of `Tok::NonLogicalNewline`. This PR generalizes the approach we took to comments (track all lines that contain any comments), and applies it to continuations too.
define_rule_mapping! was previously implemented as a declarative macro,
which was however partially relying on an origin_by_code! proc macro
because declarative macros cannot match on substrings of identifiers.
Currently all define_rule_mapping! lines look like the following:
TID251 => violations::BannedApi,
TID252 => violations::BannedRelativeImport,
We want to break up violations.rs, moving the violation definitions to
the respective rule modules. To do this we want to change the previous
lines to:
TID251 => rules::flake8_tidy_imports::banned_api::BannedApi,
TID252 => rules::flake8_tidy_imports::relative_imports::RelativeImport,
This however doesn't work because the define_rule_mapping! macro is
currently defined as:
($($code:ident => $mod:ident::$name:ident,)+) => { ... }
That is it only supported $module::$name but not longer paths with
multiple modules. While we could define `=> $path:path`[1] then we
could no longer access the last path segment, which we need because
we use it for the DiagnosticKind variant names. And
`$path:path::$last:ident` doesn't work either because it would be
ambiguous (Rust wouldn't know where the path ends ... so path fragments
have to be followed by some punctuation/keyword that may not be part of
paths). And we also cannot just introduce a procedural macro like
path_basename!(...) because the following is not valid Rust code:
enum Foo { foo!(...), }
(macros cannot be called in the place where you define variants.)
So we have to convert define_rule_mapping! into a proc macro in order to
support paths of arbitrary length and this commit implements that.
[1]: https://doc.rust-lang.org/reference/macros-by-example.html#metavariables
Before
```
resources/test/fixtures/flake8_simplify/SIM208.py:1:13: SIM208 Use `a` instead of `not (not a)`
|
1 | if not (not a): # SIM208
| ^ SIM208
|
= help: Replace with `a`
resources/test/fixtures/flake8_simplify/SIM208.py:4:14: SIM208 Use `a == b` instead of `not (not a == b)`
|
4 | if not (not (a == b)): # SIM208
| ^^^^^^ SIM208
|
= help: Replace with `a == b`
```
After
```
resources/test/fixtures/flake8_simplify/SIM208.py:1:4: SIM208 Use `a` instead of `not (not a)`
|
1 | if not (not a): # SIM208
| ^^^^^^^^^^^ SIM208
|
= help: Replace with `a`
resources/test/fixtures/flake8_simplify/SIM208.py:4:4: SIM208 Use `a == b` instead of `not (not a == b)`
|
4 | if not (not (a == b)): # SIM208
| ^^^^^^^^^^^^^^^^^^ SIM208
|
= help: Replace with `a == b`
```
This makes it easier to see which rules you're enabling when selecting
one of the pylint codes (like `PLC`). This also makes it clearer what
those abbreviations stand for. When I first saw the pylint section, I
was very confused by that, so other might be as well.
See it rendered here:
https://github.com/thomkeh/ruff/blob/patch-1/README.md#pylint-plc-ple-plr-plw
This bumps RustPython so we can use the new `NonLogicalNewline` token.
A couple of rules needed a fix due to the new token. There might be more
that are not caught by tests (anything working with tokens directly with
lookaheads), I hope not.
This PR makes the following changes to improve `SIM117`:
- Avoid emitting `SIM117` multiple times within the same `with`
statement:
- Adjust the error range.
## Example
```python
with A() as a: # SIM117
with B() as b:
with C() as c:
print("hello")
```
### Current
```
resources/test/fixtures/flake8_simplify/SIM117.py:5:1: SIM117 Use a single `with` statement with multiple contexts instead of nested `with` statements
|
5 | / with A() as a: # SIM117
6 | | with B() as b:
7 | | with C() as c:
8 | | print("hello")
| |__________________________^ SIM117
|
resources/test/fixtures/flake8_simplify/SIM117.py:6:5: SIM117 Use a single `with` statement with multiple contexts instead of nested `with` statements
|
6 | with B() as b:
| _____^
7 | | with C() as c:
8 | | print("hello")
| |__________________________^ SIM117
|
```
### Improved
```
resources/test/fixtures/flake8_simplify/SIM117.py:5:1: SIM117 Use a single `with` statement with multiple contexts instead of nested `with` statements
|
5 | / with A() as a: # SIM117
6 | | with B() as b:
7 | | with C() as c:
| |______________________^ SIM117
|
```
Signed-off-by: harupy <hkawamura0130@gmail.com>
I initially attempted to run `wasm-pack build -p ruff` which gave the
error message:
Error: crate directory is missing a `Cargo.toml` file; is `-p` the wrong
directory?
I interpreted that as wasm-pack looking for the "ruff" directory because
I specified -p ruff, however actually the wasm-pack build usage is:
wasm-pack build [FLAGS] [OPTIONS] <path> <cargo-build-options>
And I was missing the `<path>` argument. So this actually wasn't at all
a bug in wasm-pack but just a confusing error message. And the symlink
hack I introduced in the previous commit didn't actually work ... I only
accidentally omitted the `-p` when testing (which ended up as `ruff`
being the <path> argument) ... CLIs are fun.
This lets you test the ruff linters or use the ruff library
without having to compile the ~100 additional dependencies
that are needed by the CLI.
Because we set the following in the [workspace] section of Cargo.toml:
default-members = [".", "ruff_cli"]
`cargo run` still runs the CLI and `cargo test` still tests
the code in src/ as well as the code in the new ruff_cli crate.
(But you can now also run `cargo test -p ruff` to only test the linters.)
This PR refactors our import-tracking logic to leverage our existing
logic for tracking bindings. It's both a significant simplification, a
significant improvement (as we can now track reassignments), and closes
out a bunch of subtle bugs.
Though the AST tracks all bindings (e.g., when parsing `import os as
foo`, we bind the name `foo` to a `BindingKind::Importation` that points
to the `os` module), when I went to implement import tracking (e.g., to
ensure that if the user references `List`, it's actually `typing.List`),
I added a parallel system specifically for this use-case.
That was a mistake, for a few reasons:
1. It didn't track reassignments, so if you had `from typing import
List`, but `List` was later overridden, we'd still consider any
reference to `List` to be `typing.List`.
2. It required a bunch of extra logic, include complex logic to try and
optimize the lookups, since it's such a hot codepath.
3. There were a few bugs in the implementation that were just hard to
correct under the existing abstractions (e.g., if you did `from typing
import Optional as Foo`, then we'd treat any reference to `Foo` _or_
`Optional` as `typing.Optional` (even though, in that case, `Optional`
was really unbound).
The new implementation goes through our existing binding tracking: when
we get a reference, we find the appropriate binding given the current
scope stack, and normalize it back to its original target.
Closes#1690.
Closes#1790.
Non-basename glob matches (e.g., for `--per-file-ignores`) assume that
the path has been converted to an absolute path. (We do this for
filenames as part of the directory traversal.) For filenames passed via
stdin, though, we're missing this conversion. So `--per-file-ignores`
that rely on the _basename_ worked as expected, but directory paths did
not.
Closes#1840.
