Skip to content

Commit

Permalink
Avoid autofixing within nested f-strings
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed May 18, 2023
1 parent 867c7bd commit 3f59ab1
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 23 deletions.
1 change: 1 addition & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP018.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@
bytes(b"""
foo""")
f"{str()}"
f"{f'{str()}'}"
13 changes: 8 additions & 5 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,13 +115,12 @@ impl<'a> Checker<'a> {
}

impl<'a> Checker<'a> {
/// Return `true` if a patch should be generated under the given autofix
/// `Mode`.
/// Return `true` if a patch should be generated for a given [`Rule`].
pub fn patch(&self, code: Rule) -> bool {
self.settings.rules.should_fix(code)
self.settings.rules.should_fix(code) && !self.ctx.in_nested_f_string()
}

/// Return `true` if a `Rule` is disabled by a `noqa` directive.
/// Return `true` if a [`Rule`] is disabled by a `noqa` directive.
pub fn rule_is_ignored(&self, code: Rule, offset: TextSize) -> bool {
// TODO(charlie): `noqa` directives are mostly enforced in `check_lines.rs`.
// However, in rare cases, we need to check them here. For example, when
Expand Down Expand Up @@ -4105,7 +4104,11 @@ where
}
}
Expr::JoinedStr(_) => {
self.ctx.flags |= ContextFlags::F_STRING;
self.ctx.flags |= if self.ctx.in_f_string() {
ContextFlags::NESTED_F_STRING
} else {
ContextFlags::F_STRING
};
visitor::walk_expr(self, expr);
}
_ => visitor::walk_expr(self, expr),
Expand Down
13 changes: 9 additions & 4 deletions crates/ruff/src/rules/pyupgrade/rules/native_literals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::fmt;

use rustpython_parser::ast::{self, Constant, Expr, Keyword, Ranged};

use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::unparse_constant;
use ruff_python_ast::str::is_implicit_concatenation;
Expand Down Expand Up @@ -30,16 +30,21 @@ pub struct NativeLiterals {
literal_type: LiteralType,
}

impl AlwaysAutofixableViolation for NativeLiterals {
impl Violation for NativeLiterals {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let NativeLiterals { literal_type } = self;
format!("Unnecessary call to `{literal_type}`")
}

fn autofix_title(&self) -> String {
fn autofix_title(&self) -> Option<String> {
let NativeLiterals { literal_type } = self;
format!("Replace with `{literal_type}`")
Some(match literal_type {
LiteralType::Str => "Replace with empty string".to_string(),
LiteralType::Bytes => "Replace with empty bytes".to_string(),
})
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ UP018.py:20:1: UP018 [*] Unnecessary call to `str`
22 | str("foo")
23 | str("""
|
= help: Replace with `str`
= help: Replace with empty string

Suggested fix
17 17 | bytes("foo")
Expand All @@ -30,7 +30,7 @@ UP018.py:21:1: UP018 [*] Unnecessary call to `str`
24 | str("""
25 | foo""")
|
= help: Replace with `str`
= help: Replace with empty string

Suggested fix
18 18 |
Expand All @@ -52,7 +52,7 @@ UP018.py:22:1: UP018 [*] Unnecessary call to `str`
26 | bytes()
27 | bytes(b"foo")
|
= help: Replace with `str`
= help: Replace with empty string

Suggested fix
19 19 | # These become string or byte literals
Expand All @@ -75,7 +75,7 @@ UP018.py:24:1: UP018 [*] Unnecessary call to `bytes`
27 | bytes(b"foo")
28 | bytes(b"""
|
= help: Replace with `bytes`
= help: Replace with empty bytes

Suggested fix
21 21 | str("foo")
Expand All @@ -96,7 +96,7 @@ UP018.py:25:1: UP018 [*] Unnecessary call to `bytes`
28 | bytes(b"""
29 | foo""")
|
= help: Replace with `bytes`
= help: Replace with empty bytes

Suggested fix
22 22 | str("""
Expand All @@ -116,8 +116,9 @@ UP018.py:26:1: UP018 [*] Unnecessary call to `bytes`
29 | | foo""")
| |_______^ UP018
30 | f"{str()}"
31 | f"{f'{str()}'}"
|
= help: Replace with `bytes`
= help: Replace with empty bytes

Suggested fix
23 23 | foo""")
Expand All @@ -128,21 +129,33 @@ UP018.py:26:1: UP018 [*] Unnecessary call to `bytes`
26 |+b"""
27 |+foo"""
28 28 | f"{str()}"
29 29 | f"{f'{str()}'}"

UP018.py:28:4: UP018 [*] Unnecessary call to `str`
|
28 | bytes(b"""
29 | foo""")
30 | f"{str()}"
| ^^^^^ UP018
31 | f"{f'{str()}'}"
|
= help: Replace with `str`
= help: Replace with empty string

Suggested fix
25 25 | bytes(b"foo")
26 26 | bytes(b"""
27 27 | foo""")
28 |-f"{str()}"
28 |+f"{''}"
29 29 | f"{f'{str()}'}"

UP018.py:29:7: UP018 Unnecessary call to `str`
|
29 | foo""")
30 | f"{str()}"
31 | f"{f'{str()}'}"
| ^^^^^ UP018
|
= help: Replace with empty string


27 changes: 20 additions & 7 deletions crates/ruff_python_semantic/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,11 @@ impl<'a> Context<'a> {
self.flags.contains(ContextFlags::F_STRING)
}

/// Return `true` if the context is in a nested f-string.
pub const fn in_nested_f_string(&self) -> bool {
self.flags.contains(ContextFlags::NESTED_F_STRING)
}

/// Return `true` if the context is in boolean test.
pub const fn in_boolean_test(&self) -> bool {
self.flags.contains(ContextFlags::BOOLEAN_TEST)
Expand Down Expand Up @@ -606,6 +611,14 @@ bitflags! {
/// ```
const F_STRING = 1 << 6;

/// The context is in a nested f-string.
///
/// For example, the context could be visiting `x` in:
/// ```python
/// f'{f"{x}"}'
/// ```
const NESTED_F_STRING = 1 << 7;

/// The context is in a boolean test.
///
/// For example, the context could be visiting `x` in:
Expand All @@ -616,7 +629,7 @@ bitflags! {
///
/// The implication is that the actual value returned by the current expression is
/// not used, only its truthiness.
const BOOLEAN_TEST = 1 << 7;
const BOOLEAN_TEST = 1 << 8;

/// The context is in a `typing::Literal` annotation.
///
Expand All @@ -625,15 +638,15 @@ bitflags! {
/// def f(x: Literal["A", "B", "C"]):
/// ...
/// ```
const LITERAL = 1 << 8;
const LITERAL = 1 << 9;

/// The context is in a subscript expression.
///
/// For example, the context could be visiting `x["a"]` in:
/// ```python
/// x["a"]["b"]
/// ```
const SUBSCRIPT = 1 << 9;
const SUBSCRIPT = 1 << 10;

/// The context is in a type-checking block.
///
Expand All @@ -645,7 +658,7 @@ bitflags! {
/// if TYPE_CHECKING:
/// x: int = 1
/// ```
const TYPE_CHECKING_BLOCK = 1 << 10;
const TYPE_CHECKING_BLOCK = 1 << 11;


/// The context has traversed past the "top-of-file" import boundary.
Expand All @@ -659,7 +672,7 @@ bitflags! {
///
/// x: int = 1
/// ```
const IMPORT_BOUNDARY = 1 << 11;
const IMPORT_BOUNDARY = 1 << 12;

/// The context has traversed past the `__future__` import boundary.
///
Expand All @@ -674,7 +687,7 @@ bitflags! {
///
/// Python considers it a syntax error to import from `__future__` after
/// any other non-`__future__`-importing statements.
const FUTURES_BOUNDARY = 1 << 12;
const FUTURES_BOUNDARY = 1 << 13;

/// `__future__`-style type annotations are enabled in this context.
///
Expand All @@ -686,7 +699,7 @@ bitflags! {
/// def f(x: int) -> int:
/// ...
/// ```
const FUTURE_ANNOTATIONS = 1 << 13;
const FUTURE_ANNOTATIONS = 1 << 14;
}
}

Expand Down

0 comments on commit 3f59ab1

Please sign in to comment.