Skip to content

Commit

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

Implements `PYI017` or `Y017` from `flake8-pyi` plug-in. Mirrors
[upstream
implementation](https://github.com/PyCQA/flake8-pyi/blob/ceab86d16b822d12abae1d8087850d0bc66d2f52/pyi.py#L1039-L1048).
It checks for any assignment with more than 1 target or an assignment to
anything other than a name, and raises a violation for these in stub
files.

Couldn't find a clear and concise explanation for why this is to be
avoided and what is preferred for attribute cases like:

```python
a.b = int
```
So welcome some input there, to learn and to finish up the docs.

## Test Plan

Added test cases from upstream plug-in in a fixture (both `.py` and
`.pyi`). Added a few more.

## Issue link

Refers: #848
  • Loading branch information
qdegraaf authored Jul 20, 2023
1 parent c948dcc commit 2cde9b8
Show file tree
Hide file tree
Showing 10 changed files with 141 additions and 1 deletion.
14 changes: 14 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI017.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
var: int
a = var # OK

b = c = int # OK

a.b = int # OK

d, e = int, str # OK

f, g, h = int, str, TypeVar("T") # OK

i: TypeAlias = int | str # OK

j: TypeAlias = int # OK
14 changes: 14 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI017.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
var: int
a = var # OK

b = c = int # PYI017

a.b = int # PYI017

d, e = int, str # PYI017

f, g, h = int, str, TypeVar("T") # PYI017

i: TypeAlias = int | str # OK

j: TypeAlias = int # OK
6 changes: 5 additions & 1 deletion crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1492,7 +1492,7 @@ where
tryceratops::rules::error_instead_of_exception(self, handlers);
}
}
Stmt::Assign(ast::StmtAssign { targets, value, .. }) => {
Stmt::Assign(stmt_assign @ ast::StmtAssign { targets, value, .. }) => {
if self.enabled(Rule::LambdaAssignment) {
if let [target] = &targets[..] {
pycodestyle::rules::lambda_assignment(self, target, value, None, stmt);
Expand Down Expand Up @@ -1557,6 +1557,7 @@ where
Rule::UnprefixedTypeParam,
Rule::AssignmentDefaultInStub,
Rule::UnannotatedAssignmentInStub,
Rule::ComplexAssignmentInStub,
Rule::TypeAliasWithoutAnnotation,
]) {
// Ignore assignments in function bodies; those are covered by other rules.
Expand All @@ -1576,6 +1577,9 @@ where
self, targets, value,
);
}
if self.enabled(Rule::ComplexAssignmentInStub) {
flake8_pyi::rules::complex_assignment_in_stub(self, stmt_assign);
}
if self.enabled(Rule::TypeAliasWithoutAnnotation) {
flake8_pyi::rules::type_alias_without_annotation(
self, value, targets,
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 @@ -629,6 +629,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "014") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::ArgumentDefaultInStub),
(Flake8Pyi, "015") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::AssignmentDefaultInStub),
(Flake8Pyi, "016") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DuplicateUnionMember),
(Flake8Pyi, "017") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::ComplexAssignmentInStub),
(Flake8Pyi, "020") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::QuotedAnnotationInStub),
(Flake8Pyi, "021") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DocstringInStub),
(Flake8Pyi, "024") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::CollectionsNamedTuple),
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 @@ -25,6 +25,8 @@ mod tests {
#[test_case(Rule::BadVersionInfoComparison, Path::new("PYI006.pyi"))]
#[test_case(Rule::CollectionsNamedTuple, Path::new("PYI024.py"))]
#[test_case(Rule::CollectionsNamedTuple, Path::new("PYI024.pyi"))]
#[test_case(Rule::ComplexAssignmentInStub, Path::new("PYI017.py"))]
#[test_case(Rule::ComplexAssignmentInStub, Path::new("PYI017.pyi"))]
#[test_case(Rule::ComplexIfStatementInStub, Path::new("PYI002.py"))]
#[test_case(Rule::ComplexIfStatementInStub, Path::new("PYI002.pyi"))]
#[test_case(Rule::DocstringInStub, Path::new("PYI021.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
use rustpython_parser::ast::{Expr, StmtAssign};

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

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

/// ## What it does
/// Checks for assignments with multiple or non-name targets in stub files.
///
/// ## Why is this bad?
/// In general, stub files should be thought of as "data files" for a type
/// checker, and are not intended to be executed. As such, it's useful to
/// enforce that only a subset of Python syntax is allowed in a stub file, to
/// ensure that everything in the stub is unambiguous for the type checker.
///
/// The need to perform multi-assignment, or assignment to a non-name target,
/// likely indicates a misunderstanding of how stub files are intended to be
/// used.
///
/// ## Example
/// ```python
/// a = b = int
/// a.b = int
/// ```
///
/// Use instead:
/// ```python
/// a: TypeAlias = int
/// b: TypeAlias = int
///
///
/// class a:
/// b: int
/// ```
#[violation]
pub struct ComplexAssignmentInStub;

impl Violation for ComplexAssignmentInStub {
#[derive_message_formats]
fn message(&self) -> String {
format!("Stubs should not contain assignments to attributes or multiple targets")
}
}

/// PYI017
pub(crate) fn complex_assignment_in_stub(checker: &mut Checker, stmt: &StmtAssign) {
if matches!(stmt.targets.as_slice(), [Expr::Name(_)]) {
return;
}
checker
.diagnostics
.push(Diagnostic::new(ComplexAssignmentInStub, stmt.range));
}
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
@@ -1,6 +1,7 @@
pub(crate) use any_eq_ne_annotation::*;
pub(crate) use bad_version_info_comparison::*;
pub(crate) use collections_named_tuple::*;
pub(crate) use complex_assignment_in_stub::*;
pub(crate) use complex_if_statement_in_stub::*;
pub(crate) use docstring_in_stubs::*;
pub(crate) use duplicate_union_member::*;
Expand Down Expand Up @@ -31,6 +32,7 @@ pub(crate) use unrecognized_version_info::*;
mod any_eq_ne_annotation;
mod bad_version_info_comparison;
mod collections_named_tuple;
mod complex_assignment_in_stub;
mod complex_if_statement_in_stub;
mod docstring_in_stubs;
mod duplicate_union_member;
Expand Down
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,44 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---
PYI017.pyi:4:1: PYI017 Stubs should not contain assignments to attributes or multiple targets
|
2 | a = var # OK
3 |
4 | b = c = int # PYI017
| ^^^^^^^^^^^ PYI017
5 |
6 | a.b = int # PYI017
|

PYI017.pyi:6:1: PYI017 Stubs should not contain assignments to attributes or multiple targets
|
4 | b = c = int # PYI017
5 |
6 | a.b = int # PYI017
| ^^^^^^^^^ PYI017
7 |
8 | d, e = int, str # PYI017
|

PYI017.pyi:8:1: PYI017 Stubs should not contain assignments to attributes or multiple targets
|
6 | a.b = int # PYI017
7 |
8 | d, e = int, str # PYI017
| ^^^^^^^^^^^^^^^ PYI017
9 |
10 | f, g, h = int, str, TypeVar("T") # PYI017
|

PYI017.pyi:10:1: PYI017 Stubs should not contain assignments to attributes or multiple targets
|
8 | d, e = int, str # PYI017
9 |
10 | f, g, h = int, str, TypeVar("T") # PYI017
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI017
11 |
12 | i: TypeAlias = int | str # 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 2cde9b8

Please sign in to comment.