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

Update F541 to use new f-string tokens #7327

Merged
merged 3 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 2 additions & 4 deletions crates/ruff/resources/test/fixtures/pyflakes/F541.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}}"
4 changes: 2 additions & 2 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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};

Expand Down Expand Up @@ -47,45 +47,69 @@ 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<Item = (TextRange, TextRange)> + '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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work with multiple nested fstrings?

f"{f"{a + f"{b}"}"}"

I would expect that f"{b}" overrides the start position of f"{a + f"{b}"}"

Copy link
Member Author

@dhruvmanila dhruvmanila Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To give some context, the autofix for the rule F541 is to remove the f prefix from the f-string without any placeholders. The placeholder checks are done at an expression level by checking if there's any FormattedValue. The function you're referring to is to get the f prefix range and the f-string range. At this point there can't be any nested f-strings.

Also, the reason the function returns an iterator is because of implicitly concatenated strings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the function name is a bit confusing which I'll rename. I've also added docs to the function.

Copy link
Member

@MichaReiser MichaReiser Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but the documentation mentions that this function returns all nested f-string ranges, meaning there need to be placeholders? It's also unclear to me where the filtering out of f-strings without placeholders happens.

So I think we need to update the documentation to match its actual behavior (or it's too early in the morning and I shouldn't be reviewing PR's 😅 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but the documentation mentions that this function returns all nested f-string ranges, meaning there need to be placeholders?

I've updated the documentation but I'm curious as to where was "nested f-string ranges" mentioned? 😅

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,
})
}

/// 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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand All @@ -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

Expand All @@ -351,17 +352,17 @@ 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
|
40 | ""f""
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

Expand All @@ -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}"


Loading