Try to reduce false positives more

crates/ruff_linter/resources/test/fixtures/ruff/RUF027_0.py

@@ -38,7 +38,7 @@ def tripled_quoted():
     c = a
     single_line = """ {a} """  # RUF027
     # RUF027
-    multi_line = a = """b { # comment
+    multi_line = """b { # comment
     c}  d
     """
 
@@ -72,3 +72,16 @@ def method_calls():
 def format_specifiers():
     a = 4
     b = "{a:b} {a:^5}"
+
+
+def implicitly_concatenated_fstring(x: int):
+    y = f"{x}" "{x}"  # RUF027
+    z = "{x}" f"{x}"  # RUF027
+
+
+def method_call_passed_to_logging(x: int):
+    # We special-case bindings that are passed to logging calls later on,
+    # but not bindings that involve arbitrary call expressions!
+    y = foo("{x}")  # RUF027
+    import logging
+    logging.log(0, y)

crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py

@@ -68,3 +68,33 @@ def negative_cases():
     @app.get("/items/{item_id}")
     async def read_item(item_id):
         return {"item_id": item_id}
+
+
+# we shouldn't flag either of these, because the bindings are used elsewhere
+# in contexts that make it clear that they're not meant to be f-strings
+GLOBAL_STRING = "foo {bar}"
+LOGGING_TEMPLATE = "{foo} bar"
+
+def uses_global_strings():
+    print(GLOBAL_STRING.format(bar="whatever"))
+
+    import logging
+    logging.error(LOGGING_TEMPLATE, 42)
+
+
+def binding_defined_after_string():
+    if bool():
+        x = "{foo}"
+    else:
+        x = "{foo}"
+    foo = 42
+    print(x.format(foo=foo))
+
+
+def variable_immediately_used_after_more_complex_binding(arg: str, as_pypath: bool):
+    msg = (
+        "module or package not found: {arg} (missing __init__.py?)"
+        if as_pypath
+        else "file or directory not found: {arg}"
+    )
+    raise TypeError(msg.format(arg=arg))

crates/ruff_linter/src/checkers/ast/analyze/bindings.rs

@@ -15,6 +15,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
         Rule::UnconventionalImportAlias,
         Rule::UnsortedDunderSlots,
         Rule::UnusedVariable,
+        Rule::MissingFStringSyntax,
     ]) {
         return;
     }
@@ -77,5 +78,11 @@ pub(crate) fn bindings(checker: &mut Checker) {
                 checker.diagnostics.push(diagnostic);
             }
         }
+        if checker.enabled(Rule::MissingFStringSyntax) {
+            if let Some(diagnostic) = ruff::rules::missing_fstring_syntax_binding(checker, binding)
+            {
+                checker.diagnostics.push(diagnostic);
+            }
+        }
     }
 }

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

@@ -1080,7 +1080,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             }
             if checker.enabled(Rule::MissingFStringSyntax) {
                 for string_literal in value.literals() {
-                    ruff::rules::missing_fstring_syntax(checker, string_literal);
+                    ruff::rules::missing_fstring_syntax_expr(checker, string_literal);
                 }
             }
         }
@@ -1376,7 +1376,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
             }
             if checker.enabled(Rule::MissingFStringSyntax) {
                 for string_literal in value.as_slice() {
-                    ruff::rules::missing_fstring_syntax(checker, string_literal);
+                    ruff::rules::missing_fstring_syntax_expr(checker, string_literal);
                 }
             }
         }

crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs

@@ -4,7 +4,7 @@ use ruff_python_ast as ast;
 use ruff_python_literal::format::FormatSpec;
 use ruff_python_parser::parse_expression;
 use ruff_python_semantic::analyze::logging::is_logger_candidate;
-use ruff_python_semantic::{Modules, SemanticModel};
+use ruff_python_semantic::{Binding, Modules, ScopeId, SemanticModel};
 use ruff_source_file::Locator;
 use ruff_text_size::{Ranged, TextRange};
 
@@ -36,6 +36,8 @@ use crate::rules::fastapi::rules::is_fastapi_route_call;
 /// 6. Any format specifiers in the potential f-string are invalid.
 /// 7. The string is part of a function call that is known to expect a template string rather than an
 ///    evaluated f-string: for example, a [`logging`] call, a [`gettext`] call, or a [`fastAPI` path].
+/// 8. The string is assigned to a symbol where any of the *references* to that symbol are part of a
+///    function call that is known to expect a template string rather than an evaluated f-string.
 ///
 /// ## Example
 ///
@@ -61,16 +63,27 @@ pub struct MissingFStringSyntax;
 impl AlwaysFixableViolation for MissingFStringSyntax {
     #[derive_message_formats]
     fn message(&self) -> String {
-        format!(r#"Possible f-string without an `f` prefix"#)
+        format!("Possible f-string without an `f` prefix")
     }
 
     fn fix_title(&self) -> String {
-        "Add `f` prefix".into()
+        "Add `f` prefix".to_string()
     }
 }
 
 /// RUF027
-pub(crate) fn missing_fstring_syntax(checker: &mut Checker, literal: &ast::StringLiteral) {
+///
+/// Analyze a [`ast::StringLiteral`] node to see if it looks like it could be a string that was
+/// meant to be an f-string, but is missing an `f` prefix. If so, emit a diagnostic.
+///
+/// This routine skips any string literals that are part of "simple assignments", e.g. `x = "foo"`
+/// or `x: str = "foo"`. These are checked by the [`missing_fstring_syntax_binding`] function
+/// below, which is part of the same check.
+pub(crate) fn missing_fstring_syntax_expr(checker: &mut Checker, literal: &ast::StringLiteral) {
+    if !has_brackets(&literal.value) {
+        return;
+    }
+
     let semantic = checker.semantic();
 
     // we want to avoid statement expressions that are just a string literal.
@@ -82,35 +95,168 @@ pub(crate) fn missing_fstring_syntax(checker: &mut Checker, literal: &ast::Strin
         }
     }
 
+    // Simple assignments will be dealt with by `missing_fstring_syntax_binding`
+    if is_simple_assignment_to_literal(literal, semantic) {
+        return;
+    }
+
     let logger_objects = &checker.settings.logger_objects;
     let fastapi_seen = semantic.seen_module(Modules::FASTAPI);
 
+    let mut arg_names = FxHashSet::default();
+
     // We also want to avoid:
     // - Expressions inside `gettext()` calls
     // - Expressions passed to logging calls (since the `logging` module evaluates them lazily:
     //   https://docs.python.org/3/howto/logging-cookbook.html#using-particular-formatting-styles-throughout-your-application)
     // - `fastAPI` paths: https://fastapi.tiangolo.com/tutorial/path-params/
     // - Expressions where a method is immediately called on the string literal
-    if semantic
+    for call_expr in semantic
         .current_expressions()
         .filter_map(ast::Expr::as_call_expr)
-        .any(|call_expr| {
-            is_method_call_on_literal(call_expr, literal)
-                || is_gettext(call_expr, semantic)
-                || is_logger_candidate(&call_expr.func, semantic, logger_objects)
-                || (fastapi_seen && is_fastapi_route_call(call_expr, semantic))
-        })
     {
-        return;
+        if is_method_call_on_literal(call_expr, literal) {
+            return;
+        }
+        if is_gettext(call_expr, semantic) {
+            return;
+        }
+        if is_logger_candidate(&call_expr.func, semantic, logger_objects) {
+            return;
+        }
+        if fastapi_seen && is_fastapi_route_call(call_expr, semantic) {
+            return;
+        }
+        let ast::Arguments { keywords, args, .. } = &call_expr.arguments;
+        for keyword in &**keywords {
+            if let Some(ident) = keyword.arg.as_ref() {
+                arg_names.insert(&ident.id);
+            }
+        }
+        for arg in &**args {
+            if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
+                arg_names.insert(id);
+            }
+        }
     }
 
-    if should_be_fstring(literal, checker.locator(), semantic) {
+    if should_be_fstring(
+        literal,
+        checker.locator(),
+        semantic,
+        semantic.scope_id,
+        &arg_names,
+    ) {
         let diagnostic = Diagnostic::new(MissingFStringSyntax, literal.range())
             .with_fix(fix_fstring_syntax(literal.range()));
         checker.diagnostics.push(diagnostic);
     }
 }
 
+/// RUF027
+///
+/// Analyze a [`Binding`] to see if the bound variable is a string literal
+/// that's part of a "simple assignment", e.g. `x = "foo"` or `x: str = "foo"`.
+/// If it is, check to see if the string looks like it's meant to be an f-string,
+/// but is missing an `f` prefix. False positives are minimized by analyzing the references
+/// to the binding, to see if the binding is ever used in a way that indicates that the
+/// string is clearly meant to be a string template rather than an f-string.
+pub(crate) fn missing_fstring_syntax_binding(
+    checker: &Checker,
+    binding: &Binding,
+) -> Option<Diagnostic> {
+    let semantic = checker.semantic();
+    let locator = checker.locator();
+
+    let stmt = binding.statement(semantic)?;
+    let string_literal = match stmt {
+        ast::Stmt::Assign(ast::StmtAssign { targets, value, .. }) => match targets.as_slice() {
+            [_] => value.as_string_literal_expr()?,
+            _ => return None,
+        },
+        ast::Stmt::AnnAssign(ast::StmtAnnAssign {
+            value: Some(value), ..
+        }) => value.as_string_literal_expr()?,
+        _ => return None,
+    };
+    let [string_literal] = string_literal.value.as_slice() else {
+        return None;
+    };
+    if !has_brackets(&string_literal.value) {
+        return None;
+    }
+
+    let logger_objects = &checker.settings.logger_objects;
+    let fastapi_seen = semantic.seen_module(Modules::FASTAPI);
+    let mut arg_names = FxHashSet::default();
+
+    for reference in binding.references().map(|id| semantic.reference(id)) {
+        let Some(expr_id) = reference.expression_id() else {
+            continue;
+        };
+        for call_expr in semantic
+            .expressions(expr_id)
+            .filter_map(ast::Expr::as_call_expr)
+        {
+            if is_method_call_on_name(call_expr, binding.name(locator)) {
+                return None;
+            }
+            if is_gettext(call_expr, semantic) {
+                return None;
+            }
+            if is_logger_candidate(&call_expr.func, semantic, logger_objects) {
+                return None;
+            }
+            if fastapi_seen && is_fastapi_route_call(call_expr, semantic) {
+                return None;
+            }
+            let ast::Arguments { keywords, args, .. } = &call_expr.arguments;
+            for keyword in &**keywords {
+                if let Some(ident) = keyword.arg.as_ref() {
+                    arg_names.insert(&ident.id);
+                }
+            }
+            for arg in &**args {
+                if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
+                    arg_names.insert(id);
+                }
+            }
+        }
+    }
+
+    should_be_fstring(string_literal, locator, semantic, binding.scope, &arg_names).then(|| {
+        Diagnostic::new(MissingFStringSyntax, string_literal.range())
+            .with_fix(fix_fstring_syntax(string_literal.range()))
+    })
+}
+
+/// Determine whether `stmt` represents a "simple assignment" to the string literal `literal`.
+///
+/// For our purposes here, a "simple assignment" is any assignment
+/// where `literal` appears on the right-hand side
+/// and `literal` is not part of an implicitly concatenated string.
+fn is_simple_assignment_to_literal(literal: &ast::StringLiteral, semantic: &SemanticModel) -> bool {
+    if semantic.current_expressions().any(|expr| match expr {
+        ast::Expr::Call(_) => true,
+        ast::Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => {
+            value.is_implicit_concatenated()
+        }
+        ast::Expr::FString(ast::ExprFString { value, .. }) => value.is_implicit_concatenated(),
+        _ => false,
+    }) {
+        return false;
+    }
+    match semantic.current_statement() {
+        ast::Stmt::Assign(ast::StmtAssign { value, .. }) => {
+            value.range().contains_range(literal.range())
+        }
+        ast::Stmt::AnnAssign(ast::StmtAnnAssign {
+            value: Some(value), ..
+        }) => value.range().contains_range(literal.range()),
+        _ => false,
+    }
+}
+
 /// Returns `true` if an expression appears to be a `gettext` call.
 ///
 /// We want to avoid statement expressions and assignments related to aliases
@@ -162,17 +308,28 @@ fn is_method_call_on_literal(call_expr: &ast::ExprCall, literal: &ast::StringLit
     value.as_slice().contains(literal)
 }
 
+/// Determine whether `call_expr` represents a method call on a symbol bound to the name `name`.
+///
+/// E.g., `foo.format(bar=3)`, where `name == "foo"`.
+fn is_method_call_on_name(call_expr: &ast::ExprCall, name: &str) -> bool {
+    let ast::Expr::Attribute(ast::ExprAttribute { value, .. }) = &*call_expr.func else {
+        return false;
+    };
+    let ast::Expr::Name(ast::ExprName { id, .. }) = &**value else {
+        return false;
+    };
+    id == name
+}
+
 /// 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,
+    scope: ScopeId,
+    relevant_argument_names: &FxHashSet<&ast::name::Name>,
 ) -> bool {
-    if !has_brackets(&literal.value) {
-        return false;
-    }
-
     let fstring_expr = format!("f{}", locator.slice(literal));
     let Ok(parsed) = parse_expression(&fstring_expr) else {
         return false;
@@ -183,35 +340,21 @@ fn should_be_fstring(
         return false;
     };
 
-    let mut arg_names = FxHashSet::default();
-    for expr in semantic
-        .current_expressions()
-        .filter_map(ast::Expr::as_call_expr)
-    {
-        let ast::Arguments { keywords, args, .. } = &expr.arguments;
-        for keyword in &**keywords {
-            if let Some(ident) = keyword.arg.as_ref() {
-                arg_names.insert(&ident.id);
-            }
-        }
-        for arg in &**args {
-            if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
-                arg_names.insert(id);
-            }
-        }
-    }
-
     for f_string in value.f_strings() {
         let mut has_name = false;
         for element in f_string.elements.expressions() {
             if let ast::Expr::Name(ast::ExprName { id, .. }) = element.expression.as_ref() {
-                if arg_names.contains(id) {
+                if relevant_argument_names.contains(id) {
                     return false;
                 }
-                if semantic
-                    .lookup_symbol(id)
-                    .map_or(true, |id| semantic.binding(id).kind.is_builtin())
-                {
+                let Some(binding_id) = semantic.lookup_symbol_in_scope(id, scope, false) else {
+                    return false;
+                };
+                let binding = semantic.binding(binding_id);
+                if binding.kind.is_builtin() {
+                    return false;
+                }
+                if binding.start() > literal.end() {
                     return false;
                 }
                 has_name = true;

crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF027_RUF027_0.py.snap

@@ -163,7 +163,7 @@ RUF027_0.py:39:19: RUF027 [*] Possible f-string without an `f` prefix
 39 |     single_line = """ {a} """  # RUF027
    |                   ^^^^^^^^^^^ RUF027
 40 |     # RUF027
-41 |     multi_line = a = """b { # comment
+41 |     multi_line = """b { # comment
    |
    = help: Add `f` prefix
 
@@ -174,15 +174,15 @@ RUF027_0.py:39:19: RUF027 [*] Possible f-string without an `f` prefix
 39    |-    single_line = """ {a} """  # RUF027
    39 |+    single_line = f""" {a} """  # RUF027
 40 40 |     # RUF027
-41 41 |     multi_line = a = """b { # comment
+41 41 |     multi_line = """b { # comment
 42 42 |     c}  d
 
-RUF027_0.py:41:22: RUF027 [*] Possible f-string without an `f` prefix
+RUF027_0.py:41:18: RUF027 [*] Possible f-string without an `f` prefix
    |
 39 |       single_line = """ {a} """  # RUF027
 40 |       # RUF027
-41 |       multi_line = a = """b { # comment
-   |  ______________________^
+41 |       multi_line = """b { # comment
+   |  __________________^
 42 | |     c}  d
 43 | |     """
    | |_______^ RUF027
@@ -193,8 +193,8 @@ RUF027_0.py:41:22: RUF027 [*] Possible f-string without an `f` prefix
 38 38 |     c = a
 39 39 |     single_line = """ {a} """  # RUF027
 40 40 |     # RUF027
-41    |-    multi_line = a = """b { # comment
-   41 |+    multi_line = a = f"""b { # comment
+41    |-    multi_line = """b { # comment
+   41 |+    multi_line = f"""b { # comment
 42 42 |     c}  d
 43 43 |     """
 44 44 | 
@@ -315,5 +315,64 @@ RUF027_0.py:74:9: RUF027 [*] Possible f-string without an `f` prefix
 73 73 |     a = 4
 74    |-    b = "{a:b} {a:^5}"
    74 |+    b = f"{a:b} {a:^5}"
+75 75 | 
+76 76 | 
+77 77 | def implicitly_concatenated_fstring(x: int):
 
+RUF027_0.py:78:16: RUF027 [*] Possible f-string without an `f` prefix
+   |
+77 | def implicitly_concatenated_fstring(x: int):
+78 |     y = f"{x}" "{x}"  # RUF027
+   |                ^^^^^ RUF027
+79 |     z = "{x}" f"{x}"  # RUF027
+   |
+   = help: Add `f` prefix
 
+ℹ Unsafe fix
+75 75 | 
+76 76 | 
+77 77 | def implicitly_concatenated_fstring(x: int):
+78    |-    y = f"{x}" "{x}"  # RUF027
+   78 |+    y = f"{x}" f"{x}"  # RUF027
+79 79 |     z = "{x}" f"{x}"  # RUF027
+80 80 | 
+81 81 | 
+
+RUF027_0.py:79:9: RUF027 [*] Possible f-string without an `f` prefix
+   |
+77 | def implicitly_concatenated_fstring(x: int):
+78 |     y = f"{x}" "{x}"  # RUF027
+79 |     z = "{x}" f"{x}"  # RUF027
+   |         ^^^^^ RUF027
+   |
+   = help: Add `f` prefix
+
+ℹ Unsafe fix
+76 76 | 
+77 77 | def implicitly_concatenated_fstring(x: int):
+78 78 |     y = f"{x}" "{x}"  # RUF027
+79    |-    z = "{x}" f"{x}"  # RUF027
+   79 |+    z = f"{x}" f"{x}"  # RUF027
+80 80 | 
+81 81 | 
+82 82 | def method_call_passed_to_logging(x: int):
+
+RUF027_0.py:85:13: RUF027 [*] Possible f-string without an `f` prefix
+   |
+83 |     # We special-case bindings that are passed to logging calls later on,
+84 |     # but not bindings that involve arbitrary call expressions!
+85 |     y = foo("{x}")  # RUF027
+   |             ^^^^^ RUF027
+86 |     import logging
+87 |     logging.log(0, y)
+   |
+   = help: Add `f` prefix
+
+ℹ Unsafe fix
+82 82 | def method_call_passed_to_logging(x: int):
+83 83 |     # We special-case bindings that are passed to logging calls later on,
+84 84 |     # but not bindings that involve arbitrary call expressions!
+85    |-    y = foo("{x}")  # RUF027
+   85 |+    y = foo(f"{x}")  # RUF027
+86 86 |     import logging
+87 87 |     logging.log(0, y)

crates/ruff_python_semantic/src/model.rs

@@ -621,10 +621,22 @@ impl<'a> SemanticModel<'a> {
         }
     }
 
-    /// Lookup a symbol in the current scope. This is a carbon copy of [`Self::resolve_load`], but
-    /// doesn't add any read references to the resolved symbol.
+    /// Lookup a symbol in the current scope
     pub fn lookup_symbol(&self, symbol: &str) -> Option<BindingId> {
-        if self.in_forward_reference() {
+        self.lookup_symbol_in_scope(symbol, self.scope_id, self.in_forward_reference())
+    }
+
+    /// Lookup a symbol in a certain scope
+    ///
+    /// This is a carbon copy of [`Self::resolve_load`], but
+    /// doesn't add any read references to the resolved symbol.
+    pub fn lookup_symbol_in_scope(
+        &self,
+        symbol: &str,
+        scope: ScopeId,
+        in_forward_reference: bool,
+    ) -> Option<BindingId> {
+        if in_forward_reference {
             if let Some(binding_id) = self.scopes.global().get(symbol) {
                 if !self.bindings[binding_id].is_unbound() {
                     return Some(binding_id);
@@ -634,7 +646,7 @@ impl<'a> SemanticModel<'a> {
 
         let mut seen_function = false;
         let mut class_variables_visible = true;
-        for (index, scope_id) in self.scopes.ancestor_ids(self.scope_id).enumerate() {
+        for (index, scope_id) in self.scopes.ancestor_ids(scope).enumerate() {
             let scope = &self.scopes[scope_id];
             if scope.kind.is_class() {
                 if seen_function && matches!(symbol, "__class__") {