From ea3cbcc362d2bd5c285634e42aead7e8985e54ac Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 2 Jun 2023 00:00:31 -0400 Subject: [PATCH] Avoid enforcing native-literals rule within nested f-strings (#4488) --- .../test/fixtures/pyupgrade/UP018.py | 1 + crates/ruff/src/checkers/ast/mod.rs | 11 +- .../rules/pyupgrade/rules/native_literals.rs | 14 +- ...ff__rules__pyupgrade__tests__UP018.py.snap | 204 +++++++++--------- crates/ruff_python_semantic/src/model.rs | 27 ++- 5 files changed, 142 insertions(+), 115 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyupgrade/UP018.py b/crates/ruff/resources/test/fixtures/pyupgrade/UP018.py index 4b15a416a3c69..adcaa1c4802b5 100644 --- a/crates/ruff/resources/test/fixtures/pyupgrade/UP018.py +++ b/crates/ruff/resources/test/fixtures/pyupgrade/UP018.py @@ -15,6 +15,7 @@ bytes(b"foo" b"bar") bytes("foo") +f"{f'{str()}'}" # These become string or byte literals str() diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 8c63f245c8b2a..5506e01bc08b9 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -116,13 +116,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(crate) fn patch(&self, code: Rule) -> bool { self.settings.rules.should_fix(code) } - /// Return `true` if a `Rule` is disabled by a `noqa` directive. + /// Return `true` if a [`Rule`] is disabled by a `noqa` directive. pub(crate) 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 @@ -3977,7 +3976,11 @@ where } } Expr::JoinedStr(_) => { - self.semantic_model.flags |= SemanticModelFlags::F_STRING; + self.semantic_model.flags |= if self.semantic_model.in_f_string() { + SemanticModelFlags::NESTED_F_STRING + } else { + SemanticModelFlags::F_STRING + }; visitor::walk_expr(self, expr); } _ => visitor::walk_expr(self, expr), diff --git a/crates/ruff/src/rules/pyupgrade/rules/native_literals.rs b/crates/ruff/src/rules/pyupgrade/rules/native_literals.rs index 5c5172791bfdc..a7849d2c4f404 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/native_literals.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/native_literals.rs @@ -38,7 +38,10 @@ impl AlwaysAutofixableViolation for NativeLiterals { fn autofix_title(&self) -> String { let NativeLiterals { literal_type } = self; - format!("Replace with `{literal_type}`") + match literal_type { + LiteralType::Str => "Replace with empty string".to_string(), + LiteralType::Bytes => "Replace with empty bytes".to_string(), + } } } @@ -50,12 +53,19 @@ pub(crate) fn native_literals( args: &[Expr], keywords: &[Keyword], ) { - let Expr::Name(ast::ExprName { id, .. }) = func else { return; }; + let Expr::Name(ast::ExprName { id, .. }) = func else { + return; + }; if !keywords.is_empty() || args.len() > 1 { return; } + // There's no way to rewrite, e.g., `f"{f'{str()}'}"` within a nested f-string. + if checker.semantic_model().in_nested_f_string() { + return; + } + if (id == "str" || id == "bytes") && checker.semantic_model().is_builtin(id) { let Some(arg) = args.get(0) else { let mut diagnostic = Diagnostic::new(NativeLiterals{literal_type:if id == "str" { diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP018.py.snap b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP018.py.snap index e392df3212121..874b172d50059 100644 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP018.py.snap +++ b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP018.py.snap @@ -1,148 +1,148 @@ --- source: crates/ruff/src/rules/pyupgrade/mod.rs --- -UP018.py:20:1: UP018 [*] Unnecessary call to `str` +UP018.py:21:1: UP018 [*] Unnecessary call to `str` | -20 | # These become string or byte literals -21 | str() +21 | # These become string or byte literals +22 | str() | ^^^^^ UP018 -22 | str("foo") -23 | str(""" +23 | str("foo") +24 | str(""" | - = help: Replace with `str` + = help: Replace with empty string ℹ Suggested fix -17 17 | bytes("foo") -18 18 | -19 19 | # These become string or byte literals -20 |-str() - 20 |+"" -21 21 | str("foo") -22 22 | str(""" -23 23 | foo""") +18 18 | f"{f'{str()}'}" +19 19 | +20 20 | # These become string or byte literals +21 |-str() + 21 |+"" +22 22 | str("foo") +23 23 | str(""" +24 24 | foo""") -UP018.py:21:1: UP018 [*] Unnecessary call to `str` +UP018.py:22:1: UP018 [*] Unnecessary call to `str` | -21 | # These become string or byte literals -22 | str() -23 | str("foo") +22 | # These become string or byte literals +23 | str() +24 | str("foo") | ^^^^^^^^^^ UP018 -24 | str(""" -25 | foo""") +25 | str(""" +26 | foo""") | - = help: Replace with `str` + = help: Replace with empty string ℹ Suggested fix -18 18 | -19 19 | # These become string or byte literals -20 20 | str() -21 |-str("foo") - 21 |+"foo" -22 22 | str(""" -23 23 | foo""") -24 24 | bytes() +19 19 | +20 20 | # These become string or byte literals +21 21 | str() +22 |-str("foo") + 22 |+"foo" +23 23 | str(""" +24 24 | foo""") +25 25 | bytes() -UP018.py:22:1: UP018 [*] Unnecessary call to `str` +UP018.py:23:1: UP018 [*] Unnecessary call to `str` | -22 | str() -23 | str("foo") -24 | / str(""" -25 | | foo""") +23 | str() +24 | str("foo") +25 | / str(""" +26 | | foo""") | |_______^ UP018 -26 | bytes() -27 | bytes(b"foo") +27 | bytes() +28 | bytes(b"foo") | - = help: Replace with `str` + = help: Replace with empty string ℹ Suggested fix -19 19 | # These become string or byte literals -20 20 | str() -21 21 | str("foo") -22 |-str(""" -23 |-foo""") - 22 |+""" - 23 |+foo""" -24 24 | bytes() -25 25 | bytes(b"foo") -26 26 | bytes(b""" +20 20 | # These become string or byte literals +21 21 | str() +22 22 | str("foo") +23 |-str(""" +24 |-foo""") + 23 |+""" + 24 |+foo""" +25 25 | bytes() +26 26 | bytes(b"foo") +27 27 | bytes(b""" -UP018.py:24:1: UP018 [*] Unnecessary call to `bytes` +UP018.py:25:1: UP018 [*] Unnecessary call to `bytes` | -24 | str(""" -25 | foo""") -26 | bytes() +25 | str(""" +26 | foo""") +27 | bytes() | ^^^^^^^ UP018 -27 | bytes(b"foo") -28 | bytes(b""" +28 | bytes(b"foo") +29 | bytes(b""" | - = help: Replace with `bytes` + = help: Replace with empty bytes ℹ Suggested fix -21 21 | str("foo") -22 22 | str(""" -23 23 | foo""") -24 |-bytes() - 24 |+b"" -25 25 | bytes(b"foo") -26 26 | bytes(b""" -27 27 | foo""") +22 22 | str("foo") +23 23 | str(""" +24 24 | foo""") +25 |-bytes() + 25 |+b"" +26 26 | bytes(b"foo") +27 27 | bytes(b""" +28 28 | foo""") -UP018.py:25:1: UP018 [*] Unnecessary call to `bytes` +UP018.py:26:1: UP018 [*] Unnecessary call to `bytes` | -25 | foo""") -26 | bytes() -27 | bytes(b"foo") +26 | foo""") +27 | bytes() +28 | bytes(b"foo") | ^^^^^^^^^^^^^ UP018 -28 | bytes(b""" -29 | foo""") +29 | bytes(b""" +30 | foo""") | - = help: Replace with `bytes` + = help: Replace with empty bytes ℹ Suggested fix -22 22 | str(""" -23 23 | foo""") -24 24 | bytes() -25 |-bytes(b"foo") - 25 |+b"foo" -26 26 | bytes(b""" -27 27 | foo""") -28 28 | f"{str()}" +23 23 | str(""" +24 24 | foo""") +25 25 | bytes() +26 |-bytes(b"foo") + 26 |+b"foo" +27 27 | bytes(b""" +28 28 | foo""") +29 29 | f"{str()}" -UP018.py:26:1: UP018 [*] Unnecessary call to `bytes` +UP018.py:27:1: UP018 [*] Unnecessary call to `bytes` | -26 | bytes() -27 | bytes(b"foo") -28 | / bytes(b""" -29 | | foo""") +27 | bytes() +28 | bytes(b"foo") +29 | / bytes(b""" +30 | | foo""") | |_______^ UP018 -30 | f"{str()}" +31 | f"{str()}" | - = help: Replace with `bytes` + = help: Replace with empty bytes ℹ Suggested fix -23 23 | foo""") -24 24 | bytes() -25 25 | bytes(b"foo") -26 |-bytes(b""" -27 |-foo""") - 26 |+b""" - 27 |+foo""" -28 28 | f"{str()}" +24 24 | foo""") +25 25 | bytes() +26 26 | bytes(b"foo") +27 |-bytes(b""" +28 |-foo""") + 27 |+b""" + 28 |+foo""" +29 29 | f"{str()}" -UP018.py:28:4: UP018 [*] Unnecessary call to `str` +UP018.py:29:4: UP018 [*] Unnecessary call to `str` | -28 | bytes(b""" -29 | foo""") -30 | f"{str()}" +29 | bytes(b""" +30 | foo""") +31 | f"{str()}" | ^^^^^ UP018 | - = 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"{''}" +26 26 | bytes(b"foo") +27 27 | bytes(b""" +28 28 | foo""") +29 |-f"{str()}" + 29 |+f"{''}" diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 63b23330dbbbd..363e8053ed46f 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -734,6 +734,11 @@ impl<'a> SemanticModel<'a> { self.flags.contains(SemanticModelFlags::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(SemanticModelFlags::NESTED_F_STRING) + } + /// Return `true` if the context is in boolean test. pub const fn in_boolean_test(&self) -> bool { self.flags.contains(SemanticModelFlags::BOOLEAN_TEST) @@ -850,6 +855,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: @@ -860,7 +873,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. /// @@ -869,7 +882,7 @@ bitflags! { /// def f(x: Literal["A", "B", "C"]): /// ... /// ``` - const LITERAL = 1 << 8; + const LITERAL = 1 << 9; /// The context is in a subscript expression. /// @@ -877,7 +890,7 @@ bitflags! { /// ```python /// x["a"]["b"] /// ``` - const SUBSCRIPT = 1 << 9; + const SUBSCRIPT = 1 << 10; /// The context is in a type-checking block. /// @@ -889,7 +902,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. @@ -903,7 +916,7 @@ bitflags! { /// /// x: int = 1 /// ``` - const IMPORT_BOUNDARY = 1 << 11; + const IMPORT_BOUNDARY = 1 << 12; /// The context has traversed past the `__future__` import boundary. /// @@ -918,7 +931,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. /// @@ -930,7 +943,7 @@ bitflags! { /// def f(x: int) -> int: /// ... /// ``` - const FUTURE_ANNOTATIONS = 1 << 13; + const FUTURE_ANNOTATIONS = 1 << 14; } }