Skip to content

Commit

Permalink
Extend unnecessary-pass (PIE790) to include ellipses in preview (#…
Browse files Browse the repository at this point in the history
…8641)

## Summary

This PR extends `unnecessary-pass` (`PIE790`) to flag unnecessary
ellipsis expressions in addition to `pass` statements. A `pass` is
equivalent to a standalone `...`, so it feels correct to me that a
single rule should cover both cases.

When we look to v0.2.0, we should also consider deprecating `PYI013`,
which flags ellipses only for classes.

Closes #8602.
  • Loading branch information
charliermarsh committed Nov 13, 2023
1 parent df9ade7 commit 534fc34
Show file tree
Hide file tree
Showing 9 changed files with 836 additions and 81 deletions.
29 changes: 29 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,32 @@ def foo():
for i in range(10):
pass # comment
pass


def foo():
print("foo")
...


def foo():
"""A docstring."""
print("foo")
...


for i in range(10):
...
...

for i in range(10):
...

...

for i in range(10):
... # comment
...

for i in range(10):
...
pass
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/suite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::rules::flake8_pie;

/// Run lint rules over a suite of [`Stmt`] syntax nodes.
pub(crate) fn suite(suite: &[Stmt], checker: &mut Checker) {
if checker.enabled(Rule::UnnecessaryPass) {
flake8_pie::rules::no_unnecessary_pass(checker, suite);
if checker.enabled(Rule::UnnecessaryPlaceholder) {
flake8_pie::rules::unnecessary_placeholder(checker, suite);
}
}
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8PytestStyle, "027") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestUnittestRaisesAssertion),

// flake8-pie
(Flake8Pie, "790") => (RuleGroup::Stable, rules::flake8_pie::rules::UnnecessaryPass),
(Flake8Pie, "790") => (RuleGroup::Stable, rules::flake8_pie::rules::UnnecessaryPlaceholder),
(Flake8Pie, "794") => (RuleGroup::Stable, rules::flake8_pie::rules::DuplicateClassFieldDefinition),
(Flake8Pie, "796") => (RuleGroup::Stable, rules::flake8_pie::rules::NonUniqueEnums),
(Flake8Pie, "800") => (RuleGroup::Stable, rules::flake8_pie::rules::UnnecessarySpread),
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff_linter/src/rules/flake8_pie/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ mod tests {
#[test_case(Rule::UnnecessaryDictKwargs, Path::new("PIE804.py"))]
#[test_case(Rule::MultipleStartsEndsWith, Path::new("PIE810.py"))]
#[test_case(Rule::UnnecessaryRangeStart, Path::new("PIE808.py"))]
#[test_case(Rule::UnnecessaryPass, Path::new("PIE790.py"))]
#[test_case(Rule::UnnecessaryPlaceholder, Path::new("PIE790.py"))]
#[test_case(Rule::UnnecessarySpread, Path::new("PIE800.py"))]
#[test_case(Rule::ReimplementedContainerBuiltin, Path::new("PIE807.py"))]
#[test_case(Rule::NonUniqueEnums, Path::new("PIE796.py"))]
Expand All @@ -31,6 +31,7 @@ mod tests {
Ok(())
}

#[test_case(Rule::UnnecessaryPlaceholder, Path::new("PIE790.py"))]
#[test_case(Rule::ReimplementedContainerBuiltin, Path::new("PIE807.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_pie/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ pub(crate) use multiple_starts_ends_with::*;
pub(crate) use non_unique_enums::*;
pub(crate) use reimplemented_container_builtin::*;
pub(crate) use unnecessary_dict_kwargs::*;
pub(crate) use unnecessary_pass::*;
pub(crate) use unnecessary_placeholder::*;
pub(crate) use unnecessary_range_start::*;
pub(crate) use unnecessary_spread::*;

Expand All @@ -12,6 +12,6 @@ mod multiple_starts_ends_with;
mod non_unique_enums;
mod reimplemented_container_builtin;
mod unnecessary_dict_kwargs;
mod unnecessary_pass;
mod unnecessary_placeholder;
mod unnecessary_range_start;
mod unnecessary_spread;
75 changes: 0 additions & 75 deletions crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_pass.rs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
use ruff_diagnostics::AlwaysFixableViolation;
use ruff_diagnostics::{Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::whitespace::trailing_comment_start_offset;
use ruff_python_ast::Stmt;
use ruff_text_size::Ranged;

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

/// ## What it does
/// Checks for unnecessary `pass` statements in functions, classes, and other
/// blocks.
///
/// In [preview], this rule also checks for unnecessary ellipsis (`...`)
/// literals.
///
/// ## Why is this bad?
/// In Python, the `pass` statement and ellipsis (`...`) literal serve as
/// placeholders, allowing for syntactically correct empty code blocks. The
/// primary purpose of these nodes is to avoid syntax errors in situations
/// where a statement or expression is syntactically required, but no code
/// needs to be executed.
///
/// If a `pass` or ellipsis is present in a code block that includes at least
/// one other statement (even, e.g., a docstring), it is unnecessary and should
/// be removed.
///
/// ## Example
/// ```python
/// def func():
/// """Placeholder docstring."""
/// pass
/// ```
///
/// Use instead:
/// ```python
/// def func():
/// """Placeholder docstring."""
/// ```
///
/// In [preview]:
/// ```python
/// def func():
/// """Placeholder docstring."""
/// ...
/// ```
///
/// Use instead:
/// ```python
/// def func():
/// """Placeholder docstring."""
/// ```
///
/// ## References
/// - [Python documentation: The `pass` statement](https://docs.python.org/3/reference/simple_stmts.html#the-pass-statement)
///
/// [preview]: https://docs.astral.sh/ruff/preview/
#[violation]
pub struct UnnecessaryPlaceholder {
kind: Placeholder,
}

impl AlwaysFixableViolation for UnnecessaryPlaceholder {
#[derive_message_formats]
fn message(&self) -> String {
let Self { kind } = self;
match kind {
Placeholder::Pass => format!("Unnecessary `pass` statement"),
Placeholder::Ellipsis => format!("Unnecessary `...` literal"),
}
}

fn fix_title(&self) -> String {
let Self { kind } = self;
match kind {
Placeholder::Pass => format!("Remove unnecessary `pass`"),
Placeholder::Ellipsis => format!("Remove unnecessary `...`"),
}
}
}

/// PIE790
pub(crate) fn unnecessary_placeholder(checker: &mut Checker, body: &[Stmt]) {
if body.len() < 2 {
return;
}

for stmt in body {
let kind = match stmt {
Stmt::Pass(_) => Placeholder::Pass,
Stmt::Expr(expr)
if expr.value.is_ellipsis_literal_expr()
&& checker.settings.preview.is_enabled() =>
{
Placeholder::Ellipsis
}
_ => continue,
};

let mut diagnostic = Diagnostic::new(UnnecessaryPlaceholder { kind }, stmt.range());
let edit = if let Some(index) = trailing_comment_start_offset(stmt, checker.locator()) {
Edit::range_deletion(stmt.range().add_end(index))
} else {
fix::edits::delete_stmt(stmt, None, checker.locator(), checker.indexer())
};
diagnostic.set_fix(Fix::safe_edit(edit).isolate(Checker::isolation(
checker.semantic().current_statement_id(),
)));
checker.diagnostics.push(diagnostic);
}
}

#[derive(Debug, PartialEq, Eq)]
enum Placeholder {
Pass,
Ellipsis,
}

impl std::fmt::Display for Placeholder {
fn fmt(&self, fmt: &mut std::fmt::Formatter) -> std::fmt::Result {
match self {
Self::Pass => fmt.write_str("pass"),
Self::Ellipsis => fmt.write_str("..."),
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,8 @@ PIE790.py:149:5: PIE790 [*] Unnecessary `pass` statement
149 |- pass # comment
149 |+ # comment
150 150 | pass
151 151 |
152 152 |

PIE790.py:150:5: PIE790 [*] Unnecessary `pass` statement
|
Expand All @@ -461,5 +463,23 @@ PIE790.py:150:5: PIE790 [*] Unnecessary `pass` statement
148 148 | for i in range(10):
149 149 | pass # comment
150 |- pass
151 150 |
152 151 |
153 152 | def foo():

PIE790.py:179:5: PIE790 [*] Unnecessary `pass` statement
|
177 | for i in range(10):
178 | ...
179 | pass
| ^^^^ PIE790
|
= help: Remove unnecessary `pass`

Safe fix
176 176 |
177 177 | for i in range(10):
178 178 | ...
179 |- pass


Loading

0 comments on commit 534fc34

Please sign in to comment.