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

Extend unnecessary-pass (PIE790) to include ellipses in preview #8641

Merged
merged 3 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
21 changes: 20 additions & 1 deletion crates/ruff_linter/src/rules/flake8_pie/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,15 @@ mod tests {
use test_case::test_case;

use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::DuplicateClassFieldDefinition, Path::new("PIE794.py"))]
#[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::ReimplementedListBuiltin, Path::new("PIE807.py"))]
#[test_case(Rule::NonUniqueEnums, Path::new("PIE796.py"))]
Expand All @@ -29,4 +30,22 @@ mod tests {
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::UnnecessaryPlaceholder, Path::new("PIE790.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("flake8_pie").join(path).as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
}
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_list_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_list_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,114 @@
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 (`...`)
/// expressions.
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
///
/// ## Why is this bad?
/// In Python, the `pass` statement and ellipsis (`...`) expression 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
/// ```
charliermarsh marked this conversation as resolved.
Show resolved Hide resolved
///
/// 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 `...` expression"),
}
}

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,
}