Relax conditions in bad-string-format-type (#2731)

crates/ruff/resources/test/fixtures/pylint/bad_string_format_type.py

@@ -4,14 +4,15 @@ print("foo %(foo)d bar %(bar)d" % {"foo": "1", "bar": "2"})
 "foo %e bar %s" % ("1", 2)
 
 "%d" % "1"
+"%o" % "1"
 "%(key)d" % {"key": "1"}
 "%x" % 1.1
 "%(key)x" % {"key": 1.1}
 "%d" % []
 "%d" % ([],)
 "%(key)d" % {"key": []}
-
 print("%d" % ("%s" % ("nested",),))
+"%d" % ((1, 2, 3),)
 
 # False negatives
 WORD = "abc"
@@ -22,15 +23,11 @@ VALUES_TO_FORMAT = (1, "2", 3.0)
 
 # OK
 "%d %s %f" % VALUES_TO_FORMAT
-
 "%s" % "1"
-
 "%s %s %s" % ("1", 2, 3.5)
-
 print("%d %d"
       %
 (1, 1.1))
-
 "%s" % 1
 "%d" % 1
 "%f" % 1
@@ -50,3 +47,11 @@ print("%s" % ("%s" % ("nested",),))
 print("%s" % ("%d" % (5,),))
 "%d %d" % "1"
 "%d" "%d" % "1"
+"-%f" % time.time()
+"%r" % (object['dn'],)
+r'\%03o' % (ord(c),)
+('%02X' % int(_) for _ in o)
+"%s;range=%d-*" % (attr, upper + 1)
+"%d" % (len(foo),)
+'(%r, %r, %r, %r)' % (hostname, address, username, '$PASSWORD')
+'%r' % ({'server_school_roles': server_school_roles, 'is_school_multiserver_domain': is_school_multiserver_domain}, )

crates/ruff/src/rules/pylint/rules/bad_string_format_type.rs

@@ -1,12 +1,13 @@
 use std::str::FromStr;
 
-use ruff_macros::{define_violation, derive_message_formats};
 use rustc_hash::FxHashMap;
 use rustpython_common::cformat::{CFormatPart, CFormatSpec, CFormatStrOrBytes, CFormatString};
-use rustpython_parser::ast::{Constant, Expr, ExprKind, Location};
+use rustpython_parser::ast::{Constant, Expr, ExprKind, Location, Operator};
 use rustpython_parser::lexer;
 use rustpython_parser::lexer::Tok;
 
+use ruff_macros::{define_violation, derive_message_formats};
+
 use crate::ast::types::Range;
 use crate::checkers::ast::Checker;
 use crate::registry::Diagnostic;
@@ -44,48 +45,104 @@ enum DataType {
     String,
     Integer,
     Float,
-    Number,
+    Object,
-    Other,
+    Unknown,
+}
+
+impl From<&Expr> for DataType {
+    fn from(expr: &Expr) -> Self {
+        match &expr.node {
+            ExprKind::NamedExpr { value, .. } => (&**value).into(),
+            ExprKind::UnaryOp { operand, .. } => (&**operand).into(),
+            ExprKind::Dict { .. } => DataType::Object,
+            ExprKind::Set { .. } => DataType::Object,
+            ExprKind::ListComp { .. } => DataType::Object,
+            ExprKind::SetComp { .. } => DataType::Object,
+            ExprKind::DictComp { .. } => DataType::Object,
+            ExprKind::GeneratorExp { .. } => DataType::Object,
+            ExprKind::JoinedStr { .. } => DataType::String,
+            ExprKind::BinOp { left, op, .. } => {
+                // Ex) "a" % "b"
+                if matches!(
+                    left.node,
+                    ExprKind::Constant {
+                        value: Constant::Str(..),
+                        ..
+                    }
+                ) && matches!(op, Operator::Mod)
+                {
+                    return DataType::String;
+                }
+                DataType::Unknown
+            }
+            ExprKind::Constant { value, .. } => match value {
+                Constant::Str(_) => DataType::String,
+                Constant::Int(_) => DataType::Integer,
+                Constant::Float(_) => DataType::Float,
+                _ => DataType::Unknown,
+            },
+            ExprKind::List { .. } => DataType::Object,
+            ExprKind::Tuple { .. } => DataType::Object,
+            _ => DataType::Unknown,
+        }
+    }
 }
 
 impl DataType {
-    fn is_compatible_with(&self, other: &Self) -> bool {
+    fn is_compatible_with(&self, format: &FormatType) -> bool {
         match self {
-            DataType::String => matches!(other, DataType::String),
+            DataType::String => matches!(
-            DataType::Integer => matches!(other, DataType::Integer | DataType::Number),
+                format,
-            DataType::Float => matches!(other, DataType::Float | DataType::Number),
+                FormatType::Unknown | FormatType::String | FormatType::Repr
-            DataType::Number => matches!(
-                other,
-                DataType::Number | DataType::Integer | DataType::Float
             ),
-            DataType::Other => false,
+            DataType::Object => matches!(
+                format,
+                FormatType::Unknown | FormatType::String | FormatType::Repr
+            ),
+            DataType::Integer => matches!(
+                format,
+                FormatType::Unknown
+                    | FormatType::String
+                    | FormatType::Repr
+                    | FormatType::Integer
+                    | FormatType::Float
+                    | FormatType::Number
+            ),
+            DataType::Float => matches!(
+                format,
+                FormatType::Unknown
+                    | FormatType::String
+                    | FormatType::Repr
+                    | FormatType::Float
+                    | FormatType::Number
+            ),
+            DataType::Unknown => true,
         }
     }
 }
 
-impl From<&Constant> for DataType {
+#[derive(Debug)]
-    fn from(value: &Constant) -> Self {
+enum FormatType {
-        match value {
+    Repr,
-            Constant::Str(_) => DataType::String,
+    String,
-            // All float codes also work for integers.
+    Integer,
-            Constant::Int(_) => DataType::Number,
+    Float,
-            Constant::Float(_) => DataType::Float,
+    Number,
-            _ => DataType::Other,
+    Unknown,
-        }
-    }
 }
 
-impl From<char> for DataType {
+impl From<char> for FormatType {
     fn from(format: char) -> Self {
         match format {
-            's' => DataType::String,
+            'r' => FormatType::Repr,
+            's' => FormatType::String,
             // The python documentation says "d" only works for integers, but it works for floats as
             // well: https://docs.python.org/3/library/string.html#formatstrings
             // I checked the rest of the integer codes, and none of them work with floats
-            'n' | 'd' => DataType::Number,
+            'n' | 'd' => FormatType::Number,
-            'b' | 'c' | 'o' | 'x' | 'X' => DataType::Integer,
+            'b' | 'c' | 'o' | 'x' | 'X' => FormatType::Integer,
-            'e' | 'E' | 'f' | 'F' | 'g' | 'G' | '%' => DataType::Float,
+            'e' | 'E' | 'f' | 'F' | 'g' | 'G' | '%' => FormatType::Float,
-            _ => DataType::Other,
+            _ => FormatType::Unknown,
         }
     }
 }
@@ -103,24 +160,14 @@ fn collect_specs(formats: &[CFormatStrOrBytes<String>]) -> Vec<&CFormatSpec> {
 }
 
 /// Return `true` if the format string is equivalent to the constant type
-fn equivalent(format: &CFormatSpec, value: &Constant) -> bool {
+fn equivalent(format: &CFormatSpec, value: &Expr) -> bool {
     let constant: DataType = value.into();
-    let format: DataType = format.format_char.into();
+    let format: FormatType = format.format_char.into();
-    if matches!(format, DataType::String) {
+    constant.is_compatible_with(&format)
-        // We can always format as type `String`.
-        return true;
-    }
-
-    if let DataType::Other = constant {
-        // If the format is not string, we cannot format as type `Other`.
-        false
-    } else {
-        constant.is_compatible_with(&format)
-    }
 }
 
 /// Return `true` if the [`Constnat`] aligns with the format type.
-fn is_valid_constant(formats: &[CFormatStrOrBytes<String>], value: &Constant) -> bool {
+fn is_valid_constant(formats: &[CFormatStrOrBytes<String>], value: &Expr) -> bool {
     let formats = collect_specs(formats);
     // If there is more than one format, this is not valid python and we should
     // return true so that no error is reported
@@ -142,14 +189,7 @@ fn is_valid_tuple(formats: &[CFormatStrOrBytes<String>], elts: &[Expr]) -> bool
     }
 
     for (format, elt) in formats.iter().zip(elts) {
-        if let ExprKind::Constant { value, .. } = &elt.node {
+        if !equivalent(format, elt) {
-            if !equivalent(format, value) {
-                return false;
-            }
-        } else if let ExprKind::Name { .. } = &elt.node {
-            continue;
-        } else if format.format_char != 's' {
-            // Non-`ExprKind::Constant` values can only be formatted as strings.
             return false;
         }
     }
@@ -191,14 +231,7 @@ fn is_valid_dict(
             let Some(format) = formats_hash.get(mapping_key.as_str()) else {
                 return true;
             };
-            if let ExprKind::Constant { value, .. } = &value.node {
+            if !equivalent(format, value) {
-                if !equivalent(format, value) {
-                    return false;
-                }
-            } else if let ExprKind::Name { .. } = &value.node {
-                continue;
-            } else if format.format_char != 's' {
-                // Non-`ExprKind::Constant` values can only be formatted as strings.
                 return false;
             }
         } else {
@@ -209,18 +242,6 @@ fn is_valid_dict(
     true
 }
 
-/// Return `true` if the format string is valid for "other" types.
-fn is_valid_other(formats: &[CFormatStrOrBytes<String>]) -> bool {
-    let formats = collect_specs(formats);
-
-    // If there's more than one format, abort.
-    if formats.len() != 1 {
-        return true;
-    }
-
-    formats.get(0).unwrap().format_char == 's'
-}
-
 /// PLE1307
 pub fn bad_string_format_type(checker: &mut Checker, expr: &Expr, right: &Expr) {
     // Grab each string segment (in case there's an implicit concatenation).
@@ -263,9 +284,8 @@ pub fn bad_string_format_type(checker: &mut Checker, expr: &Expr, right: &Expr)
     let is_valid = match &right.node {
         ExprKind::Tuple { elts, .. } => is_valid_tuple(&format_strings, elts),
         ExprKind::Dict { keys, values } => is_valid_dict(&format_strings, keys, values),
-        ExprKind::Constant { value, .. } => is_valid_constant(&format_strings, value),
+        ExprKind::Constant { .. } => is_valid_constant(&format_strings, right),
-        ExprKind::Name { .. } => true,
+        _ => true,
-        _ => is_valid_other(&format_strings),
     };
     if !is_valid {
         checker.diagnostics.push(Diagnostic::new(

crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1307_bad_string_format_type.py.snap

@@ -1,5 +1,5 @@
 ---
-source: src/rules/pylint/mod.rs
+source: crates/ruff/src/rules/pylint/mod.rs
 expression: diagnostics
 ---
 - kind:
@@ -39,56 +39,56 @@ expression: diagnostics
     column: 0
   end_location:
     row: 7
-    column: 24
-  fix: ~
-  parent: ~
-- kind:
-    BadStringFormatType: ~
-  location:
-    row: 8
-    column: 0
-  end_location:
-    row: 8
     column: 10
   fix: ~
   parent: ~
 - kind:
     BadStringFormatType: ~
   location:
-    row: 9
+    row: 8
     column: 0
   end_location:
-    row: 9
+    row: 8
     column: 24
   fix: ~
   parent: ~
 - kind:
     BadStringFormatType: ~
   location:
-    row: 10
+    row: 9
     column: 0
   end_location:
-    row: 10
+    row: 9
-    column: 9
+    column: 10
   fix: ~
   parent: ~
 - kind:
     BadStringFormatType: ~
   location:
-    row: 11
+    row: 10
     column: 0
   end_location:
-    row: 11
+    row: 10
+    column: 24
+  fix: ~
+  parent: ~
+- kind:
+    BadStringFormatType: ~
+  location:
+    row: 12
+    column: 0
+  end_location:
+    row: 12
     column: 12
   fix: ~
   parent: ~
 - kind:
     BadStringFormatType: ~
   location:
-    row: 12
+    row: 13
     column: 0
   end_location:
-    row: 12
+    row: 13
     column: 23
   fix: ~
   parent: ~
@@ -102,4 +102,14 @@ expression: diagnostics
     column: 34
   fix: ~
   parent: ~
+- kind:
+    BadStringFormatType: ~
+  location:
+    row: 15
+    column: 0
+  end_location:
+    row: 15
+    column: 19
+  fix: ~
+  parent: ~