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

Implement RUF027: Missing F-String Syntax lint #9728

Merged
merged 13 commits into from
Feb 3, 2024
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
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)))
snowsignal marked this conversation as resolved.
Show resolved Hide resolved
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) {
snowsignal marked this conversation as resolved.
Show resolved Hide resolved
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(
snowsignal marked this conversation as resolved.
Show resolved Hide resolved
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
Loading