Skip to content

Commit

Permalink
Implement RUF027: Missing F-String Syntax lint (#9728)
Browse files Browse the repository at this point in the history
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

Fixes #8151

This PR implements a new rule, `RUF027`.

## What it does
Checks for strings that contain f-string syntax but are not f-strings.

### Why is this bad?
An f-string missing an `f` at the beginning won't format anything, and
instead treat the interpolation syntax as literal.

### Example

```python
name = "Sarah"
dayofweek = "Tuesday"
msg = "Hello {name}! It is {dayofweek} today!"
```

It should instead be:
```python
name = "Sarah"
dayofweek = "Tuesday"
msg = f"Hello {name}! It is {dayofweek} today!"
```

## Heuristics
Since there are many possible string literals which contain syntax
similar to f-strings yet are not intended to be,
this lint will disqualify any literal that satisfies any of the
following conditions:
1. The string literal is a standalone expression. For example, a
docstring.
2. The literal is part of a function call with keyword arguments that
match at least one variable (for example: `format("Message: {value}",
value = "Hello World")`)
3. The literal (or a parent expression of the literal) has a direct
method call on it (for example: `"{value}".format(...)`)
4. The string has no `{...}` expression sections, or uses invalid
f-string syntax.
5. The string references variables that are not in scope, or it doesn't
capture variables at all.
6. Any format specifiers in the potential f-string are invalid.

## Test Plan

I created a new test file, `RUF027.py`, which is both an example of what
the lint should catch and a way to test edge cases that may trigger
false positives.
  • Loading branch information
snowsignal authored Feb 3, 2024
1 parent 25d9305 commit e0a6034
Show file tree
Hide file tree
Showing 8 changed files with 563 additions and 1 deletion.
85 changes: 85 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF027.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
val = 2

def simple_cases():
a = 4
b = "{a}" # RUF027
c = "{a} {b} f'{val}' " # RUF027

def escaped_string():
a = 4
b = "escaped string: {{ brackets surround me }}" # RUF027

def raw_string():
a = 4
b = r"raw string with formatting: {a}" # RUF027
c = r"raw string with \backslashes\ and \"escaped quotes\": {a}" # RUF027

def print_name(name: str):
a = 4
print("Hello, {name}!") # RUF027
print("The test value we're using today is {a}") # RUF027

def do_nothing(a):
return a

def nested_funcs():
a = 4
print(do_nothing(do_nothing("{a}"))) # RUF027

def tripled_quoted():
a = 4
c = a
single_line = """ {a} """ # RUF027
# RUF027
multi_line = a = """b { # comment
c} d
"""

def single_quoted_multi_line():
a = 4
# RUF027
b = " {\
a} \
"

def implicit_concat():
a = 4
b = "{a}" "+" "{b}" r" \\ " # RUF027 for the first part only
print(f"{a}" "{a}" f"{b}") # RUF027

def escaped_chars():
a = 4
b = "\"not escaped:\" \'{a}\' \"escaped:\": \'{{c}}\'" # RUF027

def alternative_formatter(src, **kwargs):
src.format(**kwargs)

def format2(src, *args):
pass

# These should not cause an RUF027 message
def negative_cases():
a = 4
positive = False
"""{a}"""
"don't format: {a}"
c = """ {b} """
d = "bad variable: {invalid}"
e = "incorrect syntax: {}"
json = "{ positive: false }"
json2 = "{ 'positive': false }"
json3 = "{ 'positive': 'false' }"
alternative_formatter("{a}", a = 5)
formatted = "{a}".fmt(a = 7)
print(do_nothing("{a}".format(a=3)))
print(do_nothing(alternative_formatter("{a}", a = 5)))
print(format(do_nothing("{a}"), a = 5))
print("{a}".to_upper())
print(do_nothing("{a}").format(a = "Test"))
print(do_nothing("{a}").format2(a))

a = 4

"always ignore this: {a}"

print("but don't ignore this: {val}") # RUF027
22 changes: 21 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,16 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
pyupgrade::rules::unicode_kind_prefix(checker, string_literal);
}
}
if checker.enabled(Rule::MissingFStringSyntax) {
for string_literal in value.literals() {
ruff::rules::missing_fstring_syntax(
&mut checker.diagnostics,
string_literal,
checker.locator,
&checker.semantic,
);
}
}
}
Expr::BinOp(ast::ExprBinOp {
left,
Expand Down Expand Up @@ -1313,12 +1323,22 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
refurb::rules::math_constant(checker, number_literal);
}
}
Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
Expr::StringLiteral(ast::ExprStringLiteral { value, range: _ }) => {
if checker.enabled(Rule::UnicodeKindPrefix) {
for string_part in value {
pyupgrade::rules::unicode_kind_prefix(checker, string_part);
}
}
if checker.enabled(Rule::MissingFStringSyntax) {
for string_literal in value.as_slice() {
ruff::rules::missing_fstring_syntax(
&mut checker.diagnostics,
string_literal,
checker.locator,
&checker.semantic,
);
}
}
}
Expr::IfExp(
if_exp @ ast::ExprIfExp {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "024") => (RuleGroup::Preview, rules::ruff::rules::MutableFromkeysValue),
(Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable),
(Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg),
(Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
#[cfg(feature = "test-rules")]
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ mod tests {
#[test_case(Rule::MutableFromkeysValue, Path::new("RUF024.py"))]
#[test_case(Rule::UnnecessaryDictComprehensionForIterable, Path::new("RUF025.py"))]
#[test_case(Rule::DefaultFactoryKwarg, Path::new("RUF026.py"))]
#[test_case(Rule::MissingFStringSyntax, Path::new("RUF027.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
157 changes: 157 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
use memchr::memchr2_iter;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast};
use ruff_python_literal::format::FormatSpec;
use ruff_python_parser::parse_expression;
use ruff_python_semantic::SemanticModel;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};
use rustc_hash::FxHashSet;

/// ## What it does
/// Checks for strings that contain f-string syntax but are not f-strings.
///
/// ## Why is this bad?
/// An f-string missing an `f` at the beginning won't format anything, and instead
/// treat the interpolation syntax as literal.
///
/// Since there are many possible string literals which contain syntax similar to f-strings yet are not intended to be,
/// this lint will disqualify any literal that satisfies any of the following conditions:
/// 1. The string literal is a standalone expression. For example, a docstring.
/// 2. The literal is part of a function call with keyword arguments that match at least one variable (for example: `format("Message: {value}", value = "Hello World")`)
/// 3. The literal (or a parent expression of the literal) has a direct method call on it (for example: `"{value}".format(...)`)
/// 4. The string has no `{...}` expression sections, or uses invalid f-string syntax.
/// 5. The string references variables that are not in scope, or it doesn't capture variables at all.
/// 6. Any format specifiers in the potential f-string are invalid.
///
/// ## Example
///
/// ```python
/// name = "Sarah"
/// dayofweek = "Tuesday"
/// msg = "Hello {name}! It is {dayofweek} today!"
/// ```
///
/// Use instead:
/// ```python
/// name = "Sarah"
/// dayofweek = "Tuesday"
/// msg = f"Hello {name}! It is {dayofweek} today!"
/// ```
#[violation]
pub struct MissingFStringSyntax;

impl AlwaysFixableViolation for MissingFStringSyntax {
#[derive_message_formats]
fn message(&self) -> String {
format!(r#"Possible f-string without an `f` prefix"#)
}

fn fix_title(&self) -> String {
"Add `f` prefix".into()
}
}

/// RUF027
pub(crate) fn missing_fstring_syntax(
diagnostics: &mut Vec<Diagnostic>,
literal: &ast::StringLiteral,
locator: &Locator,
semantic: &SemanticModel,
) {
// we want to avoid statement expressions that are just a string literal.
// there's no reason to have standalone f-strings and this lets us avoid docstrings too
if let ast::Stmt::Expr(ast::StmtExpr { value, .. }) = semantic.current_statement() {
match value.as_ref() {
ast::Expr::StringLiteral(_) | ast::Expr::FString(_) => return,
_ => {}
}
}

if should_be_fstring(literal, locator, semantic) {
let diagnostic = Diagnostic::new(MissingFStringSyntax, literal.range())
.with_fix(fix_fstring_syntax(literal.range()));
diagnostics.push(diagnostic);
}
}

/// Returns `true` if `literal` is likely an f-string with a missing `f` prefix.
/// See [`MissingFStringSyntax`] for the validation criteria.
fn should_be_fstring(
literal: &ast::StringLiteral,
locator: &Locator,
semantic: &SemanticModel,
) -> bool {
if !has_brackets(&literal.value) {
return false;
}

let Ok(ast::Expr::FString(ast::ExprFString { value, .. })) =
parse_expression(&format!("f{}", locator.slice(literal.range())))
else {
return false;
};

let mut kwargs = vec![];
for expr in semantic.current_expressions() {
match expr {
ast::Expr::Call(ast::ExprCall {
arguments: ast::Arguments { keywords, .. },
func,
..
}) => {
if let ast::Expr::Attribute(ast::ExprAttribute { .. }) = func.as_ref() {
return false;
}
kwargs.extend(keywords.iter());
}
_ => continue,
}
}

let kw_idents: FxHashSet<&str> = kwargs
.iter()
.filter_map(|k| k.arg.as_ref())
.map(ast::Identifier::as_str)
.collect();

for f_string in value.f_strings() {
let mut has_name = false;
for element in f_string
.elements
.iter()
.filter_map(|element| element.as_expression())
{
if let ast::Expr::Name(ast::ExprName { id, .. }) = element.expression.as_ref() {
if kw_idents.contains(id.as_str()) || semantic.lookup_symbol(id).is_none() {
return false;
}
has_name = true;
}
if let Some(spec) = &element.format_spec {
let spec = locator.slice(spec.range());
if FormatSpec::parse(spec).is_err() {
return false;
}
}
}
if !has_name {
return false;
}
}

true
}

// fast check to disqualify any string literal without brackets
#[inline]
fn has_brackets(possible_fstring: &str) -> bool {
// this qualifies rare false positives like "{ unclosed bracket"
// but it's faster in the general case
memchr2_iter(b'{', b'}', possible_fstring.as_bytes()).count() > 1
}

fn fix_fstring_syntax(range: TextRange) -> Fix {
Fix::unsafe_edit(Edit::insertion("f".into(), range.start()))
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub(crate) use function_call_in_dataclass_default::*;
pub(crate) use implicit_optional::*;
pub(crate) use invalid_index_type::*;
pub(crate) use invalid_pyproject_toml::*;
pub(crate) use missing_fstring_syntax::*;
pub(crate) use mutable_class_default::*;
pub(crate) use mutable_dataclass_default::*;
pub(crate) use mutable_fromkeys_value::*;
Expand Down Expand Up @@ -37,6 +38,7 @@ mod helpers;
mod implicit_optional;
mod invalid_index_type;
mod invalid_pyproject_toml;
mod missing_fstring_syntax;
mod mutable_class_default;
mod mutable_dataclass_default;
mod mutable_fromkeys_value;
Expand Down
Loading

0 comments on commit e0a6034

Please sign in to comment.