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 index b7344b26ad7418..feb9fd853132c5 100644 --- 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,42 @@ 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: Escapes +'\\test'.strip('\\') + +# 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 index 89aee1e4cde5de..b4a594ead1b28b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -906,7 +906,7 @@ pub(crate) fn expression(expr: &Expr, checker: &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 index 656b74bff48d2b..5f50cb5a623256 100644 --- 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 @@ -65,6 +65,22 @@ impl Violation for BadStrStripCall { } } +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +pub(crate) enum ValueKind { + String, + Bytes, +} + +impl ValueKind { + fn from(expr: &Expr) -> Option { + match expr { + Expr::StringLiteral(_) => Some(Self::String), + Expr::BytesLiteral(_) => Some(Self::Bytes), + _ => None, + } + } +} + #[derive(Debug, PartialEq, Eq, Copy, Clone)] pub(crate) enum StripKind { Strip, @@ -120,52 +136,66 @@ 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 string_has_duplicate_char(string: &ast::StringLiteralValue) -> bool { + has_duplicate(string.chars()) +} + +fn bytes_has_duplicate_char(bytes: &ast::BytesLiteralValue) -> bool { + has_duplicate(bytes.bytes().map(char::from)) +} + +/// Return true if a string or byte sequence has a duplicate. +fn has_duplicate(mut chars: impl Iterator) -> bool { 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; - } - } - false + + chars.any(|char| !seen.insert(char)) } /// PLE1310 -pub(crate) fn bad_str_strip_call(checker: &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.report_diagnostic(Diagnostic::new( - BadStrStripCall { strip, removal }, - arg.range(), - )); - } - } - } - } +pub(crate) fn bad_str_strip_call(checker: &Checker, call: &ast::ExprCall) { + 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 => { + string_has_duplicate_char(&string.value) + } + Expr::BytesLiteral(bytes) if value_kind == ValueKind::Bytes => { + bytes_has_duplicate_char(&bytes.value) } + _ => return, + }; + + if !duplicated { + return; } + + let removal = if checker.settings.target_version >= PythonVersion::Py39 { + RemovalKind::for_strip(strip) + } else { + None + }; + + let diagnostic = Diagnostic::new(BadStrStripCall { strip, removal }, arg.range()); + + checker.report_diagnostic(diagnostic); } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap index 889fca29d760d5..c6d17717c50b0d 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap @@ -148,8 +148,36 @@ bad_str_strip_call.py:64:12: PLE1310 String `lstrip` call contains duplicate cha bad_str_strip_call.py:67:12: PLE1310 String `rstrip` call contains duplicate characters (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 characters + | +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 characters + | +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 characters + | +79 | ''.strip('\\b\\x09') +80 | ''.strip(r'\b\x09') +81 | ''.strip('\\\x5C') + | ^^^^^^^^ PLE1310 +82 | +83 | # OK: Different types + |