Support parenthesized expressions in string-dot-format fixes

crates/ruff/resources/test/fixtures/pyflakes/F522.py

@@ -2,6 +2,5 @@
 "{bar}{}".format(1, bar=2, spam=3)  # F522
 "{bar:{spam}}".format(bar=2, spam=3)  # No issues
 "{bar:{spam}}".format(bar=2, spam=3, eggs=4, ham=5)  # F522
-# Not fixable
 (''
- .format(x=2))
+ .format(x=2))  # F522

crates/ruff/resources/test/fixtures/pyflakes/F523.py

@@ -28,6 +28,6 @@
 "{1}{3}".format(1, 2, 3, 4)  # F523, # F524
 "{1} {8}".format(0, 1)  # F523, # F524
 
-# Not fixable
+# Multiline
 (''
 .format(2))

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

@@ -381,34 +381,34 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                                 Ok(summary) => {
                                     if checker.enabled(Rule::StringDotFormatExtraNamedArguments) {
                                         pyflakes::rules::string_dot_format_extra_named_arguments(
-                                            checker, &summary, keywords, location,
+                                            checker, call, &summary, keywords,
                                         );
                                     }
                                     if checker
                                         .enabled(Rule::StringDotFormatExtraPositionalArguments)
                                     {
                                         pyflakes::rules::string_dot_format_extra_positional_arguments(
-                                            checker, &summary, args, location,
+                                            checker, call, &summary, args,
                                         );
                                     }
                                     if checker.enabled(Rule::StringDotFormatMissingArguments) {
                                         pyflakes::rules::string_dot_format_missing_argument(
-                                            checker, &summary, args, keywords, location,
+                                            checker, call, &summary, args, keywords,
                                         );
                                     }
                                     if checker.enabled(Rule::StringDotFormatMixingAutomatic) {
                                         pyflakes::rules::string_dot_format_mixing_automatic(
-                                            checker, &summary, location,
+                                            checker, call, &summary,
                                         );
                                     }
                                     if checker.enabled(Rule::FormatLiterals) {
-                                        pyupgrade::rules::format_literals(checker, &summary, call);
+                                        pyupgrade::rules::format_literals(checker, call, &summary);
                                     }
                                     if checker.enabled(Rule::FString) {
                                         pyupgrade::rules::f_strings(
                                             checker,
+                                            call,
                                             &summary,
-                                            expr,
                                             value,
                                             checker.settings.line_length,
                                         );

crates/ruff/src/cst/matchers.rs

@@ -1,9 +1,11 @@
+use crate::autofix::codemods::CodegenStylist;
 use anyhow::{bail, Result};
 use libcst_native::{
     Arg, Attribute, Call, Comparison, CompoundStatement, Dict, Expression, FunctionDef,
     GeneratorExp, If, Import, ImportAlias, ImportFrom, ImportNames, IndentedBlock, Lambda,
     ListComp, Module, Name, SmallStatement, Statement, Suite, Tuple, With,
 };
+use ruff_python_codegen::Stylist;
 
 pub(crate) fn match_module(module_text: &str) -> Result<Module> {
     match libcst_native::parse_module(module_text, None) {
@@ -12,13 +14,6 @@ pub(crate) fn match_module(module_text: &str) -> Result<Module> {
     }
 }
 
-pub(crate) fn match_expression(expression_text: &str) -> Result<Expression> {
-    match libcst_native::parse_expression(expression_text) {
-        Ok(expression) => Ok(expression),
-        Err(_) => bail!("Failed to extract expression from source"),
-    }
-}
-
 pub(crate) fn match_statement(statement_text: &str) -> Result<Statement> {
     match libcst_native::parse_statement(statement_text) {
         Ok(statement) => Ok(statement),
@@ -205,3 +200,59 @@ pub(crate) fn match_if<'a, 'b>(statement: &'a mut Statement<'b>) -> Result<&'a m
         bail!("Expected Statement::Compound")
     }
 }
+
+/// Given the source code for an expression, return the parsed [`Expression`].
+///
+/// If the expression is not guaranteed to be valid as a standalone expression (e.g., if it may
+/// span multiple lines and/or require parentheses), use [`transform_expression`] instead.
+pub(crate) fn match_expression(expression_text: &str) -> Result<Expression> {
+    match libcst_native::parse_expression(expression_text) {
+        Ok(expression) => Ok(expression),
+        Err(_) => bail!("Failed to extract expression from source"),
+    }
+}
+
+/// Run a transformation function over an expression.
+///
+/// Passing an expression to [`match_expression`] directly can lead to parse errors if the
+/// expression is not a valid standalone expression (e.g., it was parenthesized in the original
+/// source). This method instead wraps the expression in "fake" parentheses, runs the
+/// transformation, then removes the "fake" parentheses.
+pub(crate) fn transform_expression(
+    source_code: &str,
+    stylist: &Stylist,
+    func: impl FnOnce(Expression) -> Result<Expression>,
+) -> Result<String> {
+    // Wrap the expression in parentheses.
+    let source_code = format!("({source_code})");
+    let expression = match_expression(&source_code)?;
+
+    // Run the function on the expression.
+    let expression = func(expression)?;
+
+    // Codegen the expression.
+    let mut source_code = expression.codegen_stylist(stylist);
+
+    // Drop the outer parentheses.
+    source_code.drain(0..1);
+    source_code.drain(source_code.len() - 1..source_code.len());
+    Ok(source_code)
+}
+
+/// Like [`transform_expression`], but operates on the source code of the expression, rather than
+/// the parsed [`Expression`]. This _shouldn't_ exist, but does to accommodate lifetime issues.
+pub(crate) fn transform_expression_text(
+    source_code: &str,
+    func: impl FnOnce(String) -> Result<String>,
+) -> Result<String> {
+    // Wrap the expression in parentheses.
+    let source_code = format!("({source_code})");
+
+    // Run the function on the expression.
+    let mut transformed = func(source_code)?;
+
+    // Drop the outer parentheses.
+    transformed.drain(0..1);
+    transformed.drain(transformed.len() - 1..transformed.len());
+    Ok(transformed)
+}

crates/ruff/src/rules/flake8_simplify/rules/yoda_conditions.rs

@@ -8,10 +8,9 @@ use ruff_python_codegen::Stylist;
 use ruff_python_stdlib::str::{self};
 use ruff_source_file::Locator;
 
-use crate::autofix::codemods::CodegenStylist;
 use crate::autofix::snippet::SourceCodeSnippet;
 use crate::checkers::ast::Checker;
-use crate::cst::matchers::{match_comparison, match_expression};
+use crate::cst::matchers::{match_comparison, transform_expression};
 use crate::registry::AsRule;
 
 /// ## What it does
@@ -96,68 +95,69 @@ fn is_constant_like(expr: &Expr) -> bool {
 /// Generate a fix to reverse a comparison.
 fn reverse_comparison(expr: &Expr, locator: &Locator, stylist: &Stylist) -> Result<String> {
     let range = expr.range();
-    let contents = locator.slice(range);
-
-    let mut expression = match_expression(contents)?;
-    let comparison = match_comparison(&mut expression)?;
-
-    let left = (*comparison.left).clone();
-
-    // Copy the right side to the left side.
-    comparison.left = Box::new(comparison.comparisons[0].comparator.clone());
-
-    // Copy the left side to the right side.
-    comparison.comparisons[0].comparator = left;
-
-    // Reverse the operator.
-    let op = comparison.comparisons[0].operator.clone();
-    comparison.comparisons[0].operator = match op {
-        CompOp::LessThan {
-            whitespace_before,
-            whitespace_after,
-        } => CompOp::GreaterThan {
-            whitespace_before,
-            whitespace_after,
-        },
-        CompOp::GreaterThan {
-            whitespace_before,
-            whitespace_after,
-        } => CompOp::LessThan {
-            whitespace_before,
-            whitespace_after,
-        },
-        CompOp::LessThanEqual {
-            whitespace_before,
-            whitespace_after,
-        } => CompOp::GreaterThanEqual {
-            whitespace_before,
-            whitespace_after,
-        },
-        CompOp::GreaterThanEqual {
-            whitespace_before,
-            whitespace_after,
-        } => CompOp::LessThanEqual {
-            whitespace_before,
-            whitespace_after,
-        },
-        CompOp::Equal {
-            whitespace_before,
-            whitespace_after,
-        } => CompOp::Equal {
-            whitespace_before,
-            whitespace_after,
-        },
-        CompOp::NotEqual {
-            whitespace_before,
-            whitespace_after,
-        } => CompOp::NotEqual {
-            whitespace_before,
-            whitespace_after,
-        },
-        _ => panic!("Expected comparison operator"),
-    };
+    let source_code = locator.slice(range);
+
+    transform_expression(source_code, stylist, |mut expression| {
+        let comparison = match_comparison(&mut expression)?;
+
+        let left = (*comparison.left).clone();
+
+        // Copy the right side to the left side.
+        comparison.left = Box::new(comparison.comparisons[0].comparator.clone());
+
+        // Copy the left side to the right side.
+        comparison.comparisons[0].comparator = left;
+
+        // Reverse the operator.
+        let op = comparison.comparisons[0].operator.clone();
+        comparison.comparisons[0].operator = match op {
+            CompOp::LessThan {
+                whitespace_before,
+                whitespace_after,
+            } => CompOp::GreaterThan {
+                whitespace_before,
+                whitespace_after,
+            },
+            CompOp::GreaterThan {
+                whitespace_before,
+                whitespace_after,
+            } => CompOp::LessThan {
+                whitespace_before,
+                whitespace_after,
+            },
+            CompOp::LessThanEqual {
+                whitespace_before,
+                whitespace_after,
+            } => CompOp::GreaterThanEqual {
+                whitespace_before,
+                whitespace_after,
+            },
+            CompOp::GreaterThanEqual {
+                whitespace_before,
+                whitespace_after,
+            } => CompOp::LessThanEqual {
+                whitespace_before,
+                whitespace_after,
+            },
+            CompOp::Equal {
+                whitespace_before,
+                whitespace_after,
+            } => CompOp::Equal {
+                whitespace_before,
+                whitespace_after,
+            },
+            CompOp::NotEqual {
+                whitespace_before,
+                whitespace_after,
+            } => CompOp::NotEqual {
+                whitespace_before,
+                whitespace_after,
+            },
+            _ => panic!("Expected comparison operator"),
+        };
 
-    Ok(expression.codegen_stylist(stylist))
+        Ok(expression)
+    })
 }
 
 /// SIM300