Skip to content

Commit

Permalink
[flake8-pyi] Implement PYI056 (#5959)
Browse files Browse the repository at this point in the history
## Summary

Checks that `append`, `extend` and `remove` methods are not called on
`__all__`. See [original
implementation](https://github.com/PyCQA/flake8-pyi/blob/2a86db8271dc500247a8a69419536240334731cf/pyi.py#L1133-L1138).

```
$ flake8 --select Y026 crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi

crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi:3:1: Y056 Calling ".append()" on "__all__" may not be supported by all type checkers (use += instead)
crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi:4:1: Y056 Calling ".extend()" on "__all__" may not be supported by all type checkers (use += instead)
crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi:5:1: Y056 Calling ".remove()" on "__all__" may not be supported by all type checkers (use += instead)
```

```
$ ./target/debug/ruff --select PYI026 crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi --no-cache

crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi:3:1: PYI056 Calling ".append()" on "__all__" may not be supported by all type checkers (use += instead)
crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi:4:1: PYI056 Calling ".extend()" on "__all__" may not be supported by all type checkers (use += instead)
crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi:5:1: PYI056 Calling ".remove()" on "__all__" may not be supported by all type checkers (use += instead)
Found 3 errors.
```

ref #848

## Test Plan

Snapshots and manual runs of flake8.
  • Loading branch information
LaBatata101 authored Jul 22, 2023
1 parent 45318d0 commit 33657d3
Show file tree
Hide file tree
Showing 10 changed files with 134 additions and 0 deletions.
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
__all__ = ["A", "B", "C"]

# Errors
__all__.append("D")
__all__.extend(["E", "Foo"])
__all__.remove("A")

# OK
__all__ += ["D"]
foo = ["Hello"]
foo.append("World")
foo.bar.append("World")
12 changes: 12 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI056.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
__all__ = ["A", "B", "C"]

# Errors
__all__.append("D")
__all__.extend(["E", "Foo"])
__all__.remove("A")

# OK
__all__ += ["D"]
foo = ["Hello"]
foo.append("World")
foo.bar.append("World")
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3037,6 +3037,9 @@ where
if self.enabled(Rule::DjangoLocalsInRenderFunction) {
flake8_django::rules::locals_in_render_function(self, func, args, keywords);
}
if self.is_stub && self.enabled(Rule::UnsupportedMethodCallOnAll) {
flake8_pyi::rules::unsupported_method_call_on_all(self, func);
}
}
Expr::Dict(ast::ExprDict {
keys,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "052") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnannotatedAssignmentInStub),
(Flake8Pyi, "054") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::NumericLiteralTooLong),
(Flake8Pyi, "053") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::StringOrBytesTooLong),
(Flake8Pyi, "056") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnsupportedMethodCallOnAll),

// flake8-pytest-style
(Flake8PytestStyle, "001") => (RuleGroup::Unspecified, rules::flake8_pytest_style::rules::PytestFixtureIncorrectParenthesesStyle),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ mod tests {
#[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.pyi"))]
#[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.py"))]
#[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.pyi"))]
#[test_case(Rule::UnsupportedMethodCallOnAll, Path::new("PYI056.py"))]
#[test_case(Rule::UnsupportedMethodCallOnAll, Path::new("PYI056.pyi"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub(crate) use unaliased_collections_abc_set_import::*;
pub(crate) use unnecessary_literal_union::*;
pub(crate) use unrecognized_platform::*;
pub(crate) use unrecognized_version_info::*;
pub(crate) use unsupported_method_call_on_all::*;

mod any_eq_ne_annotation;
mod bad_version_info_comparison;
Expand Down Expand Up @@ -59,3 +60,4 @@ mod unaliased_collections_abc_set_import;
mod unnecessary_literal_union;
mod unrecognized_platform;
mod unrecognized_version_info;
mod unsupported_method_call_on_all;
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
use rustpython_parser::ast::{self, Expr, Ranged};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks that `append`, `extend` and `remove` methods are not called on
/// `__all__`.
///
/// ## Why is this bad?
/// Different type checkers have varying levels of support for calling these
/// methods on `__all__`. Instead, use the `+=` operator to add items to
/// `__all__`, which is known to be supported by all major type checkers.
///
/// ## Example
/// ```python
/// __all__ = ["A"]
/// __all__.append("B")
/// ```
///
/// Use instead:
/// ```python
/// __all__ = ["A"]
/// __all__ += "B"
/// ```
#[violation]
pub struct UnsupportedMethodCallOnAll {
name: String,
}

impl Violation for UnsupportedMethodCallOnAll {
#[derive_message_formats]
fn message(&self) -> String {
let UnsupportedMethodCallOnAll { name } = self;
format!("Calling `.{name}()` on `__all__` may not be supported by all type checkers (use `+=` instead)")
}
}

/// PYI056
pub(crate) fn unsupported_method_call_on_all(checker: &mut Checker, func: &Expr) {
let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func else {
return;
};
let Expr::Name(ast::ExprName { id, .. }) = value.as_ref() else {
return;
};
if id.as_str() != "__all__" {
return;
}
if !is_unsupported_method(attr.as_str()) {
return;
}
checker.diagnostics.push(Diagnostic::new(
UnsupportedMethodCallOnAll {
name: attr.to_string(),
},
func.range(),
));
}

fn is_unsupported_method(name: &str) -> bool {
matches!(name, "append" | "extend" | "remove")
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---
PYI056.pyi:4:1: PYI056 Calling `.append()` on `__all__` may not be supported by all type checkers (use `+=` instead)
|
3 | # Errors
4 | __all__.append("D")
| ^^^^^^^^^^^^^^ PYI056
5 | __all__.extend(["E", "Foo"])
6 | __all__.remove("A")
|

PYI056.pyi:5:1: PYI056 Calling `.extend()` on `__all__` may not be supported by all type checkers (use `+=` instead)
|
3 | # Errors
4 | __all__.append("D")
5 | __all__.extend(["E", "Foo"])
| ^^^^^^^^^^^^^^ PYI056
6 | __all__.remove("A")
|

PYI056.pyi:6:1: PYI056 Calling `.remove()` on `__all__` may not be supported by all type checkers (use `+=` instead)
|
4 | __all__.append("D")
5 | __all__.extend(["E", "Foo"])
6 | __all__.remove("A")
| ^^^^^^^^^^^^^^ PYI056
7 |
8 | # OK
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 33657d3

Please sign in to comment.