[pylint] Do not report calls when object type and argument type mis…

…match, remove custom escape handling logic (`PLE1310`)
astral-sh · Feb 6, 2025 · 0186a72 · 0186a72
1 parent 0906554
commit 0186a72
Show file tree

Hide file tree

Showing 4 changed files with 170 additions and 64 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/bad_str_strip_call.py b/crates/ruff_linter/resources/test/fixtures/pylint/bad_str_strip_call.py
@@ -64,10 +64,39 @@
 u''.lstrip('http://')
 
 # PLE1310
-b''.rstrip('http://')
+b''.rstrip(b'http://')
 
 # OK
 ''.strip('Hi')
 
 # OK
 ''.strip()
+
+
+### https://github.com/astral-sh/ruff/issues/15968
+
+# Errors: Multiple backslashes
+''.strip('\\b\\x09')
+''.strip(r'\b\x09')
+''.strip('\\\x5C')
+
+# OK: Different types
+b"".strip("//")
+"".strip(b"//")
+
+# OK: Extra/missing arguments
+"".strip("//", foo)
+b"".lstrip(b"//", foo = "bar")
+"".rstrip()
+
+# OK: Not literals
+foo: str = ""; bar: bytes = b""
+"".strip(foo)
+b"".strip(bar)
+
+# False negative
+foo.rstrip("//")
+bar.lstrip(b"//")
+
+# OK: Not `.[lr]?strip`
+"".mobius_strip("")
diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs
@@ -912,7 +912,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
                 pylint::rules::bad_open_mode(checker, call);
             }
             if checker.enabled(Rule::BadStrStripCall) {
-                pylint::rules::bad_str_strip_call(checker, func, args);
+                pylint::rules::bad_str_strip_call(checker, call);
             }
             if checker.enabled(Rule::ShallowCopyEnviron) {
                 pylint::rules::shallow_copy_environ(checker, call);

diff --git a/crates/ruff_linter/src/rules/pylint/rules/bad_str_strip_call.rs b/crates/ruff_linter/src/rules/pylint/rules/bad_str_strip_call.rs
@@ -47,20 +47,43 @@ use crate::settings::types::PythonVersion;
 /// - [Python documentation: `str.strip`](https://docs.python.org/3/library/stdtypes.html?highlight=strip#str.strip)
 #[derive(ViolationMetadata)]
 pub(crate) struct BadStrStripCall {
+    duplicated: char,
     strip: StripKind,
     removal: Option<RemovalKind>,
 }
 
 impl Violation for BadStrStripCall {
     #[derive_message_formats]
     fn message(&self) -> String {
-        let Self { strip, removal } = self;
+        let Self {
+            duplicated,
+            strip,
+            removal,
+        } = self;
+
         if let Some(removal) = removal {
             format!(
-                "String `{strip}` call contains duplicate characters (did you mean `{removal}`?)",
+                "String `{strip}` call contains duplicate character {duplicated:#?} \
+                (did you mean `{removal}`?)",
             )
         } else {
-            format!("String `{strip}` call contains duplicate characters")
+            format!("String `{strip}` call contains duplicate character {duplicated:#?}")
+        }
+    }
+}
+
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
+pub(crate) enum ValueKind {
+    String,
+    Bytes,
+}
+
+impl ValueKind {
+    fn from(expr: &Expr) -> Option<Self> {
+        match expr {
+            Expr::StringLiteral(_) => Some(Self::String),
+            Expr::BytesLiteral(_) => Some(Self::Bytes),
+            _ => None,
         }
     }
 }
@@ -120,52 +143,78 @@ impl fmt::Display for RemovalKind {
     }
 }
 
-/// Return `true` if a string contains duplicate characters, taking into account
-/// escapes.
-fn has_duplicates(s: &ast::StringLiteralValue) -> bool {
-    let mut escaped = false;
+fn find_duplicate_in_string(string: &ast::StringLiteralValue) -> Option<char> {
+    find_duplicate(string.chars())
+}
+
+fn find_duplicate_in_bytes(bytes: &ast::BytesLiteralValue) -> Option<char> {
+    find_duplicate(bytes.bytes().map(|byte| char::from(byte)))
+}
+
+/// Return `true` if a string or byte sequence contains duplicate characters.
+fn find_duplicate(chars: impl Iterator<Item = char>) -> Option<char> {
     let mut seen = FxHashSet::default();
-    for ch in s.chars() {
-        if escaped {
-            escaped = false;
-            let pair = format!("\\{ch}");
-            if !seen.insert(pair) {
-                return true;
-            }
-        } else if ch == '\\' {
-            escaped = true;
-        } else if !seen.insert(ch.to_string()) {
-            return true;
+
+    for char in chars {
+        if !seen.insert(char) {
+            return Some(char);
         }
     }
-    false
+
+    None
 }
 
 /// PLE1310
-pub(crate) fn bad_str_strip_call(checker: &mut Checker, func: &Expr, args: &[Expr]) {
-    if let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func {
-        if matches!(
-            value.as_ref(),
-            Expr::StringLiteral(_) | Expr::BytesLiteral(_)
-        ) {
-            if let Some(strip) = StripKind::from_str(attr.as_str()) {
-                if let Some(arg) = args.first() {
-                    if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = &arg {
-                        if has_duplicates(value) {
-                            let removal = if checker.settings.target_version >= PythonVersion::Py39
-                            {
-                                RemovalKind::for_strip(strip)
-                            } else {
-                                None
-                            };
-                            checker.diagnostics.push(Diagnostic::new(
-                                BadStrStripCall { strip, removal },
-                                arg.range(),
-                            ));
-                        }
-                    }
-                }
-            }
-        }
+pub(crate) fn bad_str_strip_call(checker: &mut Checker, call: &ast::ExprCall) {
+    if checker.settings.target_version < PythonVersion::Py39 {
+        return;
     }
+
+    let (func, arguments) = (&call.func, &call.arguments);
+
+    let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
+        return;
+    };
+
+    let Some(strip) = StripKind::from_str(attr.as_str()) else {
+        return;
+    };
+
+    if !arguments.keywords.is_empty() {
+        return;
+    }
+
+    let [arg] = arguments.args.as_ref() else {
+        return;
+    };
+
+    let Some(value_kind) = ValueKind::from(value.as_ref()) else {
+        return;
+    };
+
+    let duplicated = match arg {
+        Expr::StringLiteral(string) if value_kind == ValueKind::String => {
+            find_duplicate_in_string(&string.value)
+        }
+        Expr::BytesLiteral(bytes) if value_kind == ValueKind::Bytes => {
+            find_duplicate_in_bytes(&bytes.value)
+        }
+        _ => return,
+    };
+
+    let Some(duplicated) = duplicated else {
+        return;
+    };
+    let removal = RemovalKind::for_strip(strip);
+
+    let diagnostic = Diagnostic::new(
+        BadStrStripCall {
+            duplicated,
+            strip,
+            removal,
+        },
+        arg.range(),
+    );
+
+    checker.diagnostics.push(diagnostic);
 }
diff --git a/...es/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap b/...es/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap
@@ -1,7 +1,7 @@
 ---
 source: crates/ruff_linter/src/rules/pylint/mod.rs
 ---
-bad_str_strip_call.py:2:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:2:21: PLE1310 String `strip` call contains duplicate character 'l'
   |
 1 | # PLE1310
 2 | "Hello World".strip("Hello")
@@ -10,7 +10,7 @@ bad_str_strip_call.py:2:21: PLE1310 String `strip` call contains duplicate chara
 4 | # PLE1310
   |
 
-bad_str_strip_call.py:5:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:5:21: PLE1310 String `strip` call contains duplicate character 'l'
   |
 4 | # PLE1310
 5 | "Hello World".strip("Hello")
@@ -19,7 +19,7 @@ bad_str_strip_call.py:5:21: PLE1310 String `strip` call contains duplicate chara
 7 | # PLE1310
   |
 
-bad_str_strip_call.py:8:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:8:21: PLE1310 String `strip` call contains duplicate character 'l'
    |
  7 | # PLE1310
  8 | "Hello World".strip(u"Hello")
@@ -28,7 +28,7 @@ bad_str_strip_call.py:8:21: PLE1310 String `strip` call contains duplicate chara
 10 | # PLE1310
    |
 
-bad_str_strip_call.py:11:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:11:21: PLE1310 String `strip` call contains duplicate character 'l'
    |
 10 | # PLE1310
 11 | "Hello World".strip(r"Hello")
@@ -37,7 +37,7 @@ bad_str_strip_call.py:11:21: PLE1310 String `strip` call contains duplicate char
 13 | # PLE1310
    |
 
-bad_str_strip_call.py:14:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:14:21: PLE1310 String `strip` call contains duplicate character 'l'
    |
 13 | # PLE1310
 14 | "Hello World".strip("Hello\t")
@@ -46,7 +46,7 @@ bad_str_strip_call.py:14:21: PLE1310 String `strip` call contains duplicate char
 16 | # PLE1310
    |
 
-bad_str_strip_call.py:17:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:17:21: PLE1310 String `strip` call contains duplicate character 'l'
    |
 16 | # PLE1310
 17 | "Hello World".strip(r"Hello\t")
@@ -55,7 +55,7 @@ bad_str_strip_call.py:17:21: PLE1310 String `strip` call contains duplicate char
 19 | # PLE1310
    |
 
-bad_str_strip_call.py:20:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:20:21: PLE1310 String `strip` call contains duplicate character 'l'
    |
 19 | # PLE1310
 20 | "Hello World".strip("Hello\\")
@@ -64,7 +64,7 @@ bad_str_strip_call.py:20:21: PLE1310 String `strip` call contains duplicate char
 22 | # PLE1310
    |
 
-bad_str_strip_call.py:23:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:23:21: PLE1310 String `strip` call contains duplicate character 'l'
    |
 22 | # PLE1310
 23 | "Hello World".strip(r"Hello\\")
@@ -73,7 +73,7 @@ bad_str_strip_call.py:23:21: PLE1310 String `strip` call contains duplicate char
 25 | # PLE1310
    |
 
-bad_str_strip_call.py:26:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:26:21: PLE1310 String `strip` call contains duplicate character '🤣'
    |
 25 | # PLE1310
 26 | "Hello World".strip("🤣🤣🤣🤣🙃👀😀")
@@ -82,7 +82,7 @@ bad_str_strip_call.py:26:21: PLE1310 String `strip` call contains duplicate char
 28 | # PLE1310
    |
 
-bad_str_strip_call.py:30:5: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:30:5: PLE1310 String `strip` call contains duplicate character 'e'
    |
 28 |   # PLE1310
 29 |   "Hello World".strip(
@@ -93,7 +93,7 @@ bad_str_strip_call.py:30:5: PLE1310 String `strip` call contains duplicate chara
 33 |   )
    |
 
-bad_str_strip_call.py:36:21: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:36:21: PLE1310 String `strip` call contains duplicate character ' '
    |
 35 |   # PLE1310
 36 |   "Hello World".strip("can we get a long " \
@@ -105,7 +105,7 @@ bad_str_strip_call.py:36:21: PLE1310 String `strip` call contains duplicate char
 40 |   # PLE1310
    |
 
-bad_str_strip_call.py:42:5: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:42:5: PLE1310 String `strip` call contains duplicate character ' '
    |
 40 |   # PLE1310
 41 |   "Hello World".strip(
@@ -116,7 +116,7 @@ bad_str_strip_call.py:42:5: PLE1310 String `strip` call contains duplicate chara
 45 |   )
    |
 
-bad_str_strip_call.py:49:5: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:49:5: PLE1310 String `strip` call contains duplicate character ' '
    |
 47 |   # PLE1310
 48 |   "Hello World".strip(
@@ -127,7 +127,7 @@ bad_str_strip_call.py:49:5: PLE1310 String `strip` call contains duplicate chara
 52 |   )
    |
 
-bad_str_strip_call.py:61:11: PLE1310 String `strip` call contains duplicate characters
+bad_str_strip_call.py:61:11: PLE1310 String `strip` call contains duplicate character 't'
    |
 60 | # PLE1310
 61 | u''.strip('http://')
@@ -136,7 +136,7 @@ bad_str_strip_call.py:61:11: PLE1310 String `strip` call contains duplicate char
 63 | # PLE1310
    |
 
-bad_str_strip_call.py:64:12: PLE1310 String `lstrip` call contains duplicate characters (did you mean `removeprefix`?)
+bad_str_strip_call.py:64:12: PLE1310 String `lstrip` call contains duplicate character 't' (did you mean `removeprefix`?)
    |
 63 | # PLE1310
 64 | u''.lstrip('http://')
@@ -145,11 +145,39 @@ bad_str_strip_call.py:64:12: PLE1310 String `lstrip` call contains duplicate cha
 66 | # PLE1310
    |
 
-bad_str_strip_call.py:67:12: PLE1310 String `rstrip` call contains duplicate characters (did you mean `removesuffix`?)
+bad_str_strip_call.py:67:12: PLE1310 String `rstrip` call contains duplicate character 't' (did you mean `removesuffix`?)
    |
 66 | # PLE1310
-67 | b''.rstrip('http://')
-   |            ^^^^^^^^^ PLE1310
+67 | b''.rstrip(b'http://')
+   |            ^^^^^^^^^^ PLE1310
 68 |
 69 | # OK
    |
+
+bad_str_strip_call.py:79:10: PLE1310 String `strip` call contains duplicate character '\\'
+   |
+78 | # Errors: Multiple backslashes
+79 | ''.strip('\\b\\x09')
+   |          ^^^^^^^^^^ PLE1310
+80 | ''.strip(r'\b\x09')
+81 | ''.strip('\\\x5C')
+   |
+
+bad_str_strip_call.py:80:10: PLE1310 String `strip` call contains duplicate character '\\'
+   |
+78 | # Errors: Multiple backslashes
+79 | ''.strip('\\b\\x09')
+80 | ''.strip(r'\b\x09')
+   |          ^^^^^^^^^ PLE1310
+81 | ''.strip('\\\x5C')
+   |
+
+bad_str_strip_call.py:81:10: PLE1310 String `strip` call contains duplicate character '\\'
+   |
+79 | ''.strip('\\b\\x09')
+80 | ''.strip(r'\b\x09')
+81 | ''.strip('\\\x5C')
+   |          ^^^^^^^^ PLE1310
+82 |
+83 | # OK: Different types
+   |