[UP031] When encountering "%s" % var offer unsafe fix

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_0.py

@@ -117,3 +117,22 @@
     cert.not_valid_after.date().isoformat().replace("-", ""), # expiration date
     hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix
 )
+
+s = set([x, y])
+"%s" % s
+# UP031: deref s to non-tuple, offer fix
+
+t1 = (x,)
+"%s" % t1
+# UP031: deref t1 to 1-tuple, offer fix
+
+t2 = (x,y)
+"%s" % t2
+# UP031: deref t2 to n-tuple, this is a bug
+
+# UP031 (no longer false negatives)
+'Hello %s' % bar
+
+'Hello %s' % bar.baz
+
+'Hello %s' % bar['bop']

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP031_1.py

@@ -32,10 +32,3 @@
 "%(1)s" % {1: 2, "1": 2}
 
 "%(and)s" % {"and": 2}
-
-# OK (arguably false negatives)
-'Hello %s' % bar
-
-'Hello %s' % bar.baz
-
-'Hello %s' % bar['bop']

crates/ruff_linter/src/rules/pyupgrade/rules/printf_string_formatting.rs

@@ -10,6 +10,7 @@ use ruff_python_literal::cformat::{
     CConversionFlags, CFormatPart, CFormatPrecision, CFormatQuantity, CFormatString,
 };
 use ruff_python_parser::{lexer, AsMode, Tok};
+use ruff_python_semantic::analyze;
 use ruff_python_stdlib::identifiers::is_identifier;
 use ruff_source_file::Locator;
 use ruff_text_size::{Ranged, TextRange};
@@ -19,47 +20,51 @@ use crate::checkers::ast::Checker;
 use crate::rules::pyupgrade::helpers::curly_escape;
 
 /// ## What it does
-/// Checks for `printf`-style string formatting.
+/// Checks for `printf`-style string formatting, and offers to replace it with
+/// `str.format` calls.
 ///
 /// ## Why is this bad?
 /// `printf`-style string formatting has a number of quirks, and leads to less
 /// readable code than using `str.format` calls or f-strings. In general, prefer
 /// the newer `str.format` and f-strings constructs over `printf`-style string
 /// formatting.
 ///
-/// ## Known problems
-/// This rule is unable to detect cases in which the format string contains
-/// a single, generic format specifier (e.g. `%s`), and the right-hand side
-/// is an ambiguous expression.
-///
-/// For example, given:
+/// ## Example
 /// ```python
-/// "%s" % value
+/// "%s, %s" % ("Hello", "World")  # "Hello, World"
 /// ```
 ///
-/// `value` could be a single-element tuple, _or_ it could be a single value.
-/// Both of these would resolve to the same formatted string when using
-/// `printf`-style formatting, but _not_ when using f-strings:
-///
+/// Use instead:
 /// ```python
-/// value = 1
-/// print("%s" % value)  # "1"
-/// print("{}".format(value))  # "1"
-///
-/// value = (1,)
-/// print("%s" % value)  # "1"
-/// print("{}".format(value))  # "(1,)"
+/// "{}, {}".format("Hello", "World")  # "Hello, World"
 /// ```
 ///
-/// ## Example
+/// ## Known problems
+/// This rule is unable to offer a fix in cases where the format string
+/// contains a single, generic format specifier (e.g. `%s`), and the right-hand side
+/// is an ambiguous expression.
+///
+/// For example, given:
 /// ```python
-/// "%s, %s" % ("Hello", "World")  # "Hello, World"
+/// "%s" % val
 /// ```
 ///
-/// Use instead:
+/// `val` could be a single-element tuple, _or_ a single value (not
+/// contained in a tuple). Both of these would resolve to the same
+/// formatted string when using `printf`-style formatting, but
+/// resolve differently when using f-strings:
+///
 /// ```python
-/// "{}, {}".format("Hello", "World")  # "Hello, World"
+/// val = 1
+/// print("%s" % val)  # "1"
+/// print("{}".format(val))  # "1"
+///
+/// val = (1,)
+/// print("%s" % val)  # "1"
+/// print("{}".format(val))  # "(1,)"
 /// ```
+/// In most cases, where `val` is not a literal that our analysis can locate,
+/// no fix will be offered (but in these specific examples, a fix exists).
 ///
 /// ## References
 /// - [Python documentation: `printf`-style String Formatting](https://docs.python.org/3/library/stdtypes.html#old-string-formatting)
@@ -286,6 +291,55 @@ fn clean_params_dictionary(right: &Expr, locator: &Locator, stylist: &Stylist) -
     Some(contents)
 }
 
+/// (In the case where the format string has only a single %-placeholder) and the righthand operand
+/// to % is a variable, try to offer a fix.
+///
+/// If the variable is not a tuple, then offer `({})`.
+/// ```python
+/// x = set(["hi", "hello"])
+/// print("%s" % x)
+/// print("{}".format(x))
+/// ```
+///
+/// If the variable is a 1-tuple, then offer `({}[0])`.
+/// ```python
+/// x = (1,)
+/// print("%s" % x)
+/// print("{}".format(x[0]))
+/// ```
+///
+/// If the variable is an n-tuple, the user has a bug in their code. Offer no fix.
+fn clean_params_variable<'a>(checker: &mut Checker, right: &Expr) -> Option<Cow<'a, str>> {
+    let Expr::Name(ast::ExprName { id, .. }) = right else {
+        return None;
+    };
+    let semantic = checker.semantic();
+    let binding_id = semantic.lookup_symbol(id.as_str())?;
+    let binding = semantic.binding(binding_id);
+    let rt = checker.locator().slice(right);
+    if let Some(expr) = analyze::typing::find_binding_value(binding, semantic) {
+        match expr {
+            // n-tuple
+            Expr::Tuple(ast::ExprTuple { elts, .. }) if 1 < elts.len() => {
+                return None;
+            }
+            // 1-tuple
+            Expr::Tuple(ast::ExprTuple { elts, .. }) if 1 == elts.len() => {
+                return Some(Cow::Owned(format!("({rt}[0])")));
+            }
+            // non-tuple
+            _ => {
+                return Some(Cow::Owned(format!("({rt})")));
+            }
+        }
+    }
+    // when find_binding_value doesn't work, fall back to offering a fix only for non-tuples
+    if analyze::typing::is_tuple(binding, semantic) {
+        return None;
+    }
+    return Some(Cow::Owned(format!("({rt})")));
+}
+
 /// Returns `true` if the sequence of [`PercentFormatPart`] indicate that an
 /// [`Expr`] can be converted.
 fn convertible(format_string: &CFormatString, params: &Expr) -> bool {
@@ -424,16 +478,16 @@ pub(crate) fn printf_string_formatting(checker: &mut Checker, expr: &Expr, right
                 // If we have multiple fields, but no named fields, assume the right-hand side is a
                 // tuple.
                 Cow::Owned(format!("(*{})", checker.locator().slice(right)))
+            } else if let Some(cow) = clean_params_variable(checker, right) {
+                // With only one field, if we have a variable we can try to deref it to handle a
+                // few cases.
+                cow
             } else {
-                // Otherwise, if we have a single field, we can't make any assumptions about the
-                // right-hand side. It _could_ be a tuple, but it could also be a single value,
-                // and we can't differentiate between them.
-                // For example:
-                // ```python
-                // x = (1,)
-                // print("%s" % x)
-                // print("{}".format(x))
-                // ```
+                // Otherwise we might have an attribute/subscript/call or a variable that we were
+                // unable to deref, so emit a diagnostic with no fix.
+                checker
+                    .diagnostics
+                    .push(Diagnostic::new(PrintfStringFormatting, expr.range()));
                 return;
             }
         }

crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP031_0.py.snap

@@ -926,6 +926,8 @@ UP031_0.py:115:8: UP031 [*] Use format specifiers instead of percent format
 118 | |     hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix
 119 | | )
     | |_^ UP031
+120 |   
+121 |   s = set([x, y])
     |
     = help: Replace with format specifiers
 
@@ -939,4 +941,89 @@ UP031_0.py:115:8: UP031 [*] Use format specifiers instead of percent format
 117 117 |     cert.not_valid_after.date().isoformat().replace("-", ""), # expiration date
 118 118 |     hexlify(cert.fingerprint(hashes.SHA256())).decode("ascii")[0:8], # fingerprint prefix
 
+UP031_0.py:122:1: UP031 [*] Use format specifiers instead of percent format
+    |
+121 | s = set([x, y])
+122 | "%s" % s
+    | ^^^^^^^^ UP031
+123 | # UP031: deref s to non-tuple, offer fix
+    |
+    = help: Replace with format specifiers
+
+ℹ Unsafe fix
+119 119 | )
+120 120 | 
+121 121 | s = set([x, y])
+122     |-"%s" % s
+    122 |+"{}".format(s)
+123 123 | # UP031: deref s to non-tuple, offer fix
+124 124 | 
+125 125 | t1 = (x,)
+
+UP031_0.py:126:1: UP031 [*] Use format specifiers instead of percent format
+    |
+125 | t1 = (x,)
+126 | "%s" % t1
+    | ^^^^^^^^^ UP031
+127 | # UP031: deref t1 to 1-tuple, offer fix
+    |
+    = help: Replace with format specifiers
+
+ℹ Unsafe fix
+123 123 | # UP031: deref s to non-tuple, offer fix
+124 124 | 
+125 125 | t1 = (x,)
+126     |-"%s" % t1
+    126 |+"{}".format(t1[0])
+127 127 | # UP031: deref t1 to 1-tuple, offer fix
+128 128 | 
+129 129 | t2 = (x,y)
+
+UP031_0.py:130:1: UP031 Use format specifiers instead of percent format
+    |
+129 | t2 = (x,y)
+130 | "%s" % t2
+    | ^^^^^^^^^ UP031
+131 | # UP031: deref t2 to n-tuple, this is a bug
+    |
+    = help: Replace with format specifiers
 
+UP031_0.py:134:1: UP031 [*] Use format specifiers instead of percent format
+    |
+133 | # UP031 (no longer false negatives)
+134 | 'Hello %s' % bar
+    | ^^^^^^^^^^^^^^^^ UP031
+135 | 
+136 | 'Hello %s' % bar.baz
+    |
+    = help: Replace with format specifiers
+
+ℹ Unsafe fix
+131 131 | # UP031: deref t2 to n-tuple, this is a bug
+132 132 | 
+133 133 | # UP031 (no longer false negatives)
+134     |-'Hello %s' % bar
+    134 |+'Hello {}'.format(bar)
+135 135 | 
+136 136 | 'Hello %s' % bar.baz
+137 137 | 
+
+UP031_0.py:136:1: UP031 Use format specifiers instead of percent format
+    |
+134 | 'Hello %s' % bar
+135 | 
+136 | 'Hello %s' % bar.baz
+    | ^^^^^^^^^^^^^^^^^^^^ UP031
+137 | 
+138 | 'Hello %s' % bar['bop']
+    |
+    = help: Replace with format specifiers
+
+UP031_0.py:138:1: UP031 Use format specifiers instead of percent format
+    |
+136 | 'Hello %s' % bar.baz
+137 | 
+138 | 'Hello %s' % bar['bop']
+    | ^^^^^^^^^^^^^^^^^^^^^^^ UP031
+    |
+    = help: Replace with format specifiers