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

[flake8-pie] Mark fix as unsafe if the following statement is a string literal (PIE790) #14393

Merged
merged 3 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
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
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 @@ -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;
}
Expand All @@ -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,
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.'
Loading