From 641c4a74259a914a1677886cd0e58d9806849875 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 18 Sep 2023 22:14:27 +0530 Subject: [PATCH] Update `F541` to use new f-string tokens (#7327) ## Summary This PR updates the `F541` rule to use the new f-string tokens. ## Test Plan Add new test case and uncomment a broken test case. fixes: #7292 --- .../resources/test/fixtures/pyflakes/F541.py | 6 +- .../src/checkers/ast/analyze/expression.rs | 4 +- .../rules/f_string_missing_placeholders.rs | 60 +++++++++++++------ ..._rules__pyflakes__tests__F541_F541.py.snap | 50 +++++++++++++--- 4 files changed, 88 insertions(+), 32 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F541.py b/crates/ruff/resources/test/fixtures/pyflakes/F541.py index 09c10216eb3374..1e59967fdb4f88 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F541.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F541.py @@ -40,7 +40,5 @@ ""f"" ''f"" (""f""r"") - -# To be fixed -# Error: f-string: single '}' is not allowed at line 41 column 8 -# f"\{{x}}" +f"{v:{f"0.2f"}}" +f"\{{x}}" diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index f0b2a05fce4b87..9dd242b2da0bad 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -946,9 +946,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { pylint::rules::await_outside_async(checker, expr); } } - Expr::FString(ast::ExprFString { values, .. }) => { + Expr::FString(fstring @ ast::ExprFString { values, .. }) => { if checker.enabled(Rule::FStringMissingPlaceholders) { - pyflakes::rules::f_string_missing_placeholders(expr, values, checker); + pyflakes::rules::f_string_missing_placeholders(fstring, checker); } if checker.enabled(Rule::HardcodedSQLExpression) { flake8_bandit::rules::hardcoded_sql_expression(checker, expr); diff --git a/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs b/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs index 779ed539bb7c1f..cf02d85deca4b9 100644 --- a/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs +++ b/crates/ruff/src/rules/pyflakes/rules/f_string_missing_placeholders.rs @@ -1,7 +1,7 @@ use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{Expr, PySourceType}; -use ruff_python_parser::{lexer, AsMode, StringKind, Tok}; +use ruff_python_ast::{self as ast, Expr, PySourceType}; +use ruff_python_parser::{lexer, AsMode, Tok}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -47,31 +47,54 @@ impl AlwaysAutofixableViolation for FStringMissingPlaceholders { } } -/// Find f-strings that don't contain any formatted values in an [`FString`]. -fn find_useless_f_strings<'a>( - expr: &'a Expr, +/// Return an iterator containing a two-element tuple for each f-string part +/// in the given [`ExprFString`] expression. +/// +/// The first element of the tuple is the f-string prefix range, and the second +/// element is the entire f-string range. It returns an iterator because of the +/// possibility of multiple f-strings implicitly concatenated together. +/// +/// For example, +/// +/// ```python +/// f"first" rf"second" +/// # ^ ^ (prefix range) +/// # ^^^^^^^^ ^^^^^^^^^^ (token range) +/// ``` +/// +/// would return `[(0..1, 0..8), (10..11, 9..19)]`. +/// +/// This function assumes that the given f-string expression is without any +/// placeholder expressions. +/// +/// [`ExprFString`]: `ruff_python_ast::ExprFString` +fn fstring_prefix_and_tok_range<'a>( + fstring: &'a ast::ExprFString, locator: &'a Locator, source_type: PySourceType, ) -> impl Iterator + 'a { - let contents = locator.slice(expr); - lexer::lex_starts_at(contents, source_type.as_mode(), expr.start()) + let contents = locator.slice(fstring); + let mut current_f_string_start = fstring.start(); + lexer::lex_starts_at(contents, source_type.as_mode(), fstring.start()) .flatten() - .filter_map(|(tok, range)| match tok { - Tok::String { - kind: StringKind::FString | StringKind::RawFString, - .. - } => { - let first_char = locator.slice(TextRange::at(range.start(), TextSize::from(1))); + .filter_map(move |(tok, range)| match tok { + Tok::FStringStart => { + current_f_string_start = range.start(); + None + } + Tok::FStringEnd => { + let first_char = + locator.slice(TextRange::at(current_f_string_start, TextSize::from(1))); // f"..." => f_position = 0 // fr"..." => f_position = 0 // rf"..." => f_position = 1 let f_position = u32::from(!(first_char == "f" || first_char == "F")); Some(( TextRange::at( - range.start() + TextSize::from(f_position), + current_f_string_start + TextSize::from(f_position), TextSize::from(1), ), - range, + TextRange::new(current_f_string_start, range.end()), )) } _ => None, @@ -79,13 +102,14 @@ fn find_useless_f_strings<'a>( } /// F541 -pub(crate) fn f_string_missing_placeholders(expr: &Expr, values: &[Expr], checker: &mut Checker) { - if !values +pub(crate) fn f_string_missing_placeholders(fstring: &ast::ExprFString, checker: &mut Checker) { + if !fstring + .values .iter() .any(|value| matches!(value, Expr::FormattedValue(_))) { for (prefix_range, tok_range) in - find_useless_f_strings(expr, checker.locator(), checker.source_type) + fstring_prefix_and_tok_range(fstring, checker.locator(), checker.source_type) { let mut diagnostic = Diagnostic::new(FStringMissingPlaceholders, tok_range); if checker.patch(diagnostic.kind.rule()) { diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F541_F541.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F541_F541.py.snap index 7ca008c27e17a1..163910d143f79b 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F541_F541.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F541_F541.py.snap @@ -332,7 +332,7 @@ F541.py:40:3: F541 [*] f-string without any placeholders 40 |+"" "" 41 41 | ''f"" 42 42 | (""f""r"") -43 43 | +43 43 | f"{v:{f"0.2f"}}" F541.py:41:3: F541 [*] f-string without any placeholders | @@ -341,6 +341,7 @@ F541.py:41:3: F541 [*] f-string without any placeholders 41 | ''f"" | ^^^ F541 42 | (""f""r"") +43 | f"{v:{f"0.2f"}}" | = help: Remove extraneous `f` prefix @@ -351,8 +352,8 @@ F541.py:41:3: F541 [*] f-string without any placeholders 41 |-''f"" 41 |+''"" 42 42 | (""f""r"") -43 43 | -44 44 | # To be fixed +43 43 | f"{v:{f"0.2f"}}" +44 44 | f"\{{x}}" F541.py:42:4: F541 [*] f-string without any placeholders | @@ -360,8 +361,8 @@ F541.py:42:4: F541 [*] f-string without any placeholders 41 | ''f"" 42 | (""f""r"") | ^^^ F541 -43 | -44 | # To be fixed +43 | f"{v:{f"0.2f"}}" +44 | f"\{{x}}" | = help: Remove extraneous `f` prefix @@ -371,8 +372,41 @@ F541.py:42:4: F541 [*] f-string without any placeholders 41 41 | ''f"" 42 |-(""f""r"") 42 |+("" ""r"") -43 43 | -44 44 | # To be fixed -45 45 | # Error: f-string: single '}' is not allowed at line 41 column 8 +43 43 | f"{v:{f"0.2f"}}" +44 44 | f"\{{x}}" + +F541.py:43:7: F541 [*] f-string without any placeholders + | +41 | ''f"" +42 | (""f""r"") +43 | f"{v:{f"0.2f"}}" + | ^^^^^^^ F541 +44 | f"\{{x}}" + | + = help: Remove extraneous `f` prefix + +ℹ Fix +40 40 | ""f"" +41 41 | ''f"" +42 42 | (""f""r"") +43 |-f"{v:{f"0.2f"}}" + 43 |+f"{v:{"0.2f"}}" +44 44 | f"\{{x}}" + +F541.py:44:1: F541 [*] f-string without any placeholders + | +42 | (""f""r"") +43 | f"{v:{f"0.2f"}}" +44 | f"\{{x}}" + | ^^^^^^^^^ F541 + | + = help: Remove extraneous `f` prefix + +ℹ Fix +41 41 | ''f"" +42 42 | (""f""r"") +43 43 | f"{v:{f"0.2f"}}" +44 |-f"\{{x}}" + 44 |+"\{x}"