diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py index f1b3d25301806..4b6f2652c69b6 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE790.py @@ -235,3 +235,15 @@ def impl(self) -> str: def contains_meaningful_ellipsis() -> list[int]: """Allow this in a TYPE_CHECKING block.""" ... + +# https://github.com/astral-sh/ruff/issues/12616 +class PotentialDocstring1: + pass + """ + Lorem ipsum dolor sit amet. + """ + + +class PotentialDocstring2: + ... + 'Lorem ipsum dolor sit amet.' diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs index e58e9c43412e1..407ffb0492d88 100644 --- a/crates/ruff_linter/src/fix/edits.rs +++ b/crates/ruff_linter/src/fix/edits.rs @@ -22,18 +22,18 @@ use crate::fix::codemods::CodegenStylist; use crate::line_width::{IndentWidth, LineLength, LineWidthBuilder}; use crate::Locator; -/// Return the `Fix` to use when deleting a `Stmt`. +/// Return the [`Edit`] to use when deleting a [`Stmt`]. /// -/// In some cases, this is as simple as deleting the `Range` of the `Stmt` +/// In some cases, this is as simple as deleting the [`TextRange`] of the [`Stmt`] /// itself. However, there are a few exceptions: -/// - If the `Stmt` is _not_ the terminal statement in a multi-statement line, +/// - If the [`Stmt`] is _not_ the terminal statement in a multi-statement line, /// we need to delete up to the start of the next statement (and avoid /// deleting any content that precedes the statement). -/// - If the `Stmt` is the terminal statement in a multi-statement line, we need +/// - If the [`Stmt`] is the terminal statement in a multi-statement line, we need /// to avoid deleting any content that precedes the statement. -/// - If the `Stmt` has no trailing and leading content, then it's convenient to +/// - If the [`Stmt`] has no trailing and leading content, then it's convenient to /// remove the entire start and end lines. -/// - If the `Stmt` is the last statement in its parent body, replace it with a +/// - If the [`Stmt`] is the last statement in its parent body, replace it with a /// `pass` instead. pub(crate) fn delete_stmt( stmt: &Stmt, diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs index e933b89f5e089..cc424e986ba32 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs +++ b/crates/ruff_linter/src/rules/flake8_pie/rules/unnecessary_placeholder.rs @@ -1,9 +1,9 @@ -use ruff_diagnostics::AlwaysFixableViolation; +use ruff_diagnostics::{AlwaysFixableViolation, Applicability}; use ruff_diagnostics::{Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::map_subscript; use ruff_python_ast::whitespace::trailing_comment_start_offset; -use ruff_python_ast::Stmt; +use ruff_python_ast::{Expr, ExprStringLiteral, Stmt, StmtExpr}; use ruff_python_semantic::{ScopeKind, SemanticModel}; use ruff_text_size::Ranged; @@ -51,6 +51,11 @@ use crate::fix; /// """Placeholder docstring.""" /// ``` /// +/// ## Fix safety +/// This rule's fix is marked as unsafe in the rare case that the `pass` or ellipsis +/// is followed by a string literal, since removal of the placeholder would convert the +/// subsequent string literal into a docstring. +/// /// ## References /// - [Python documentation: The `pass` statement](https://docs.python.org/3/reference/simple_stmts.html#the-pass-statement) #[violation] @@ -82,19 +87,19 @@ pub(crate) fn unnecessary_placeholder(checker: &mut Checker, body: &[Stmt]) { return; } - for stmt in body { + for (index, stmt) in body.iter().enumerate() { let kind = match stmt { Stmt::Pass(_) => Placeholder::Pass, Stmt::Expr(expr) if expr.value.is_ellipsis_literal_expr() => { - // In a type-checking block, a trailing ellipsis might be meaningful. A - // user might be using the type-checking context to declare a stub. + // In a type-checking block, a trailing ellipsis might be meaningful. + // A user might be using the type-checking context to declare a stub. if checker.semantic().in_type_checking_block() { return; } - // Ellipses are significant in protocol methods and abstract methods. Specifically, - // Pyright uses the presence of an ellipsis to indicate that a method is a stub, - // rather than a default implementation. + // Ellipses are significant in protocol methods and abstract methods. + // Specifically, Pyright uses the presence of an ellipsis to indicate that + // a method is a stub, rather than a default implementation. if in_protocol_or_abstract_method(checker.semantic()) { return; } @@ -103,19 +108,46 @@ pub(crate) fn unnecessary_placeholder(checker: &mut Checker, body: &[Stmt]) { _ => continue, }; - let mut diagnostic = Diagnostic::new(UnnecessaryPlaceholder { kind }, stmt.range()); - let edit = if let Some(index) = trailing_comment_start_offset(stmt, checker.source()) { - 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); + let next_stmt = body.get(index + 1); + add_diagnostic(checker, stmt, next_stmt, kind); } } +/// Add a diagnostic for the given statement. +fn add_diagnostic( + checker: &mut Checker, + stmt: &Stmt, + next_stmt: Option<&Stmt>, + placeholder_kind: Placeholder, +) { + let edit = if let Some(index) = trailing_comment_start_offset(stmt, checker.source()) { + Edit::range_deletion(stmt.range().add_end(index)) + } else { + fix::edits::delete_stmt(stmt, None, checker.locator(), checker.indexer()) + }; + let applicability = match next_stmt { + // Mark the fix as unsafe if the following statement is a string literal, + // as it will become the module/class/function's docstring after the fix. + Some(Stmt::Expr(StmtExpr { value, .. })) => match value.as_ref() { + Expr::StringLiteral(ExprStringLiteral { .. }) => Applicability::Unsafe, + _ => Applicability::Safe, + }, + _ => Applicability::Safe, + }; + + let isolation_level = Checker::isolation(checker.semantic().current_statement_id()); + let fix = Fix::applicable_edit(edit, applicability).isolate(isolation_level); + + let diagnostic = Diagnostic::new( + UnnecessaryPlaceholder { + kind: placeholder_kind, + }, + stmt.range(), + ); + + checker.diagnostics.push(diagnostic.with_fix(fix)); +} + #[derive(Debug, PartialEq, Eq)] enum Placeholder { Pass, diff --git a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap index 702980195d8dd..bda688be365ea 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap @@ -675,3 +675,39 @@ PIE790.py:209:9: PIE790 [*] Unnecessary `...` literal 210 209 | 211 210 | 212 211 | class Repro(Protocol[int]): + +PIE790.py:241:5: PIE790 [*] Unnecessary `pass` statement + | +239 | # https://github.com/astral-sh/ruff/issues/12616 +240 | class PotentialDocstring1: +241 | pass + | ^^^^ PIE790 +242 | """ +243 | Lorem ipsum dolor sit amet. + | + = help: Remove unnecessary `pass` + +ℹ Unsafe fix +238 238 | +239 239 | # https://github.com/astral-sh/ruff/issues/12616 +240 240 | class PotentialDocstring1: +241 |- pass +242 241 | """ +243 242 | Lorem ipsum dolor sit amet. +244 243 | """ + +PIE790.py:248:5: PIE790 [*] Unnecessary `...` literal + | +247 | class PotentialDocstring2: +248 | ... + | ^^^ PIE790 +249 | 'Lorem ipsum dolor sit amet.' + | + = help: Remove unnecessary `...` + +ℹ Unsafe fix +245 245 | +246 246 | +247 247 | class PotentialDocstring2: +248 |- ... +249 248 | 'Lorem ipsum dolor sit amet.'