Skip to content

Commit

Permalink
[flake8-pie] Mark fix as unsafe if the following statement is a str…
Browse files Browse the repository at this point in the history
…ing literal (`PIE790`) (#14393)

## Summary

Resolves #12616.

## Test Plan

`cargo nextest run` and `cargo insta test`.

---------

Co-authored-by: Charlie Marsh <[email protected]>
  • Loading branch information
InSyncWithFoo and charliermarsh authored Nov 18, 2024
1 parent 3c9e76e commit 0a27c9d
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 24 deletions.
12 changes: 12 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 @@ -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.'
12 changes: 6 additions & 6 deletions crates/ruff_linter/src/fix/edits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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;
}
Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.'

0 comments on commit 0a27c9d

Please sign in to comment.