From 787ae4e74dcbcb21f630060708bcfc8d454cfacb Mon Sep 17 00:00:00 2001
From: Jane Lewis <jane@astral.sh>
Date: Wed, 7 Feb 2024 15:44:51 -0500
Subject: [PATCH] Clarify some logic branches in `should_be_fstring` and
 exclude literals that use variables which any method calls also use

---
 .../resources/test/fixtures/ruff/RUF027_1.py  |  2 ++
 .../ruff/rules/missing_fstring_syntax.rs      | 32 ++++++++++++-------
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py
index 4848e40c60cf4..3684f77a39de2 100644
--- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py
+++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF027_1.py
@@ -32,3 +32,5 @@ def negative_cases():
     print(do_nothing("{a}").format(a="Test"))
     print(do_nothing("{a}").format2(a))
     print(("{a}" "{c}").format(a=1, c=2))
+    print("{a}".attribute.chaining.call(a=2))
+    print("{a} {c}".format(a))
diff --git a/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs b/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs
index d5f3ecfe706b7..de4319f167d08 100644
--- a/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs
+++ b/crates/ruff_linter/src/rules/ruff/rules/missing_fstring_syntax.rs
@@ -19,7 +19,7 @@ use rustc_hash::FxHashSet;
 /// 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")`)
+/// 2. The literal is part of a function call with argument names 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.
@@ -93,41 +93,51 @@ fn should_be_fstring(
         return false;
     };
 
-    let mut kwargs = vec![];
+    let mut arg_names = FxHashSet::default();
     let mut last_expr: Option<&ast::Expr> = None;
     for expr in semantic.current_expressions() {
         match expr {
             ast::Expr::Call(ast::ExprCall {
-                arguments: ast::Arguments { keywords, .. },
+                arguments: ast::Arguments { keywords, args, .. },
                 func,
                 ..
             }) => {
                 if let ast::Expr::Attribute(ast::ExprAttribute { value, .. }) = func.as_ref() {
                     match value.as_ref() {
+                        // if the first part of the attribute is the string literal,
+                        // we want to ignore this literal from the lint.
+                        // for example: `"{x}".some_method(...)`
                         ast::Expr::StringLiteral(expr_literal)
                             if expr_literal.value.as_slice().contains(literal) =>
                         {
                             return false;
                         }
+                        // if the first part of the attribute was the expression we
+                        // just went over in the last iteration, then we also want to pass
+                        // this over in the lint.
+                        // for example: `some_func("{x}").some_method(...)`
                         value if last_expr == Some(value) => {
                             return false;
                         }
                         _ => {}
                     }
                 }
-                kwargs.extend(keywords.iter());
+                for keyword in keywords {
+                    if let Some(ident) = keyword.arg.as_ref() {
+                        arg_names.insert(ident.as_str());
+                    }
+                }
+                for arg in args {
+                    if let ast::Expr::Name(ast::ExprName { id, .. }) = arg {
+                        arg_names.insert(id.as_str());
+                    }
+                }
             }
             _ => continue,
         }
         last_expr.replace(expr);
     }
 
-    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
@@ -136,7 +146,7 @@ fn should_be_fstring(
             .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()) {
+                if arg_names.contains(id.as_str()) {
                     return false;
                 }
                 if semantic