From 72444505ac26204c7c0168f56289c53c21d66382 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 17 Nov 2024 00:26:52 +0000 Subject: [PATCH 1/3] [`flake8-pie`] Mark fix as unsafe if the following statement is a string literal (`PIE790`) --- .../test/fixtures/flake8_pie/PIE790.py | 12 ++++ crates/ruff_linter/src/fix/edits.rs | 12 ++-- .../rules/unnecessary_placeholder.rs | 63 +++++++++++++------ ...__flake8_pie__tests__PIE790_PIE790.py.snap | 36 +++++++++++ 4 files changed, 99 insertions(+), 24 deletions(-) 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..af4a8b975dd99 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; @@ -82,19 +82,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 +103,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); } } +#[inline] +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..193f8c80915aa 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.' From 1bfabdd4532cd45ea4433d25028d9fb9f54e272d Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 17 Nov 2024 01:05:42 +0000 Subject: [PATCH 2/3] Update snapshot --- ..._linter__rules__flake8_pie__tests__PIE790_PIE790.py.snap | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 193f8c80915aa..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 @@ -688,7 +688,7 @@ PIE790.py:241:5: PIE790 [*] Unnecessary `pass` statement = help: Remove unnecessary `pass` ℹ Unsafe fix -238 238 | +238 238 | 239 239 | # https://github.com/astral-sh/ruff/issues/12616 240 240 | class PotentialDocstring1: 241 |- pass @@ -706,8 +706,8 @@ PIE790.py:248:5: PIE790 [*] Unnecessary `...` literal = help: Remove unnecessary `...` ℹ Unsafe fix -245 245 | -246 246 | +245 245 | +246 246 | 247 247 | class PotentialDocstring2: 248 |- ... 249 248 | 'Lorem ipsum dolor sit amet.' From 3503aada386c097a359fdf6f402337148b92ed8c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 17 Nov 2024 21:24:12 -0500 Subject: [PATCH 3/3] Docs --- .../src/rules/flake8_pie/rules/unnecessary_placeholder.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 af4a8b975dd99..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 @@ -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] @@ -108,7 +113,7 @@ pub(crate) fn unnecessary_placeholder(checker: &mut Checker, body: &[Stmt]) { } } -#[inline] +/// Add a diagnostic for the given statement. fn add_diagnostic( checker: &mut Checker, stmt: &Stmt,