From 610f150dd154167fb6b0133ccc8eaec3f236385a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 6 Feb 2023 16:25:20 -0500 Subject: [PATCH] Remove autofix from bad-str-strip-call; add suggestions instead (#2610) --- README.md | 2 +- .../rules/pylint/rules/bad_str_strip_call.rs | 153 ++++++------------ ..._tests__PLE1310_bad_str_strip_call.py.snap | 136 ++++------------ 3 files changed, 81 insertions(+), 210 deletions(-) diff --git a/README.md b/README.md index 0bbb6d8437..cb79d78511 100644 --- a/README.md +++ b/README.md @@ -1336,7 +1336,7 @@ For more, see [Pylint](https://pypi.org/project/pylint/) on PyPI. | PLE0604 | invalid-all-object | Invalid object in `__all__`, must contain only strings | | | PLE0605 | invalid-all-format | Invalid format for `__all__`, must be `tuple` or `list` | | | PLE1142 | await-outside-async | `await` should be used within an async function | | -| PLE1310 | bad-str-strip-call | String `{kind}` call contains duplicate characters | 🛠 | +| PLE1310 | bad-str-strip-call | String `{strip}` call contains duplicate characters (did you mean `{removal}`?) | | #### Refactor (PLR) diff --git a/crates/ruff/src/rules/pylint/rules/bad_str_strip_call.rs b/crates/ruff/src/rules/pylint/rules/bad_str_strip_call.rs index c3ef0b47f9..685fb2091c 100644 --- a/crates/ruff/src/rules/pylint/rules/bad_str_strip_call.rs +++ b/crates/ruff/src/rules/pylint/rules/bad_str_strip_call.rs @@ -2,8 +2,6 @@ use std::fmt; use rustc_hash::FxHashSet; use rustpython_ast::{Constant, Expr, ExprKind}; -use rustpython_parser::lexer; -use rustpython_parser::lexer::Tok; use serde::{Deserialize, Serialize}; use ruff_macros::derive_message_formats; @@ -11,10 +9,29 @@ use ruff_macros::derive_message_formats; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::define_violation; -use crate::fix::Fix; use crate::registry::Diagnostic; -use crate::rules::pydocstyle::helpers::{leading_quote, trailing_quote}; -use crate::violation::AlwaysAutofixableViolation; +use crate::settings::types::PythonVersion; +use crate::violation::Violation; + +define_violation!( + pub struct BadStrStripCall { + strip: StripKind, + removal: Option, + } +); +impl Violation for BadStrStripCall { + #[derive_message_formats] + fn message(&self) -> String { + let Self { strip, removal } = self; + if let Some(removal) = removal { + format!( + "String `{strip}` call contains duplicate characters (did you mean `{removal}`?)", + ) + } else { + format!("String `{strip}` call contains duplicate characters") + } + } +} #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] pub enum StripKind { @@ -45,56 +62,30 @@ impl fmt::Display for StripKind { } } -define_violation!( - pub struct BadStrStripCall { - kind: StripKind, - } -); -impl AlwaysAutofixableViolation for BadStrStripCall { - #[derive_message_formats] - fn message(&self) -> String { - let Self { kind } = self; - format!("String `{kind}` call contains duplicate characters") - } +#[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] +pub enum RemovalKind { + RemovePrefix, + RemoveSuffix, +} - fn autofix_title(&self) -> String { - "Remove duplicate characters".to_string() +impl RemovalKind { + pub fn for_strip(s: &StripKind) -> Option { + match s { + StripKind::Strip => None, + StripKind::LStrip => Some(Self::RemovePrefix), + StripKind::RStrip => Some(Self::RemoveSuffix), + } } } -/// Remove duplicate characters from an escaped string. -fn deduplicate_escaped(s: &str) -> String { - let mut result = String::new(); - let mut escaped = false; - let mut seen = FxHashSet::default(); - for ch in s.chars() { - if escaped { - escaped = false; - let pair = format!("\\{}", ch); - if !seen.insert(pair) { - continue; - } - } else if ch == '\\' { - escaped = true; - } else if !seen.insert(ch.to_string()) { - continue; - } - result.push(ch); +impl fmt::Display for RemovalKind { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + let representation = match self { + Self::RemovePrefix => "removeprefix", + Self::RemoveSuffix => "removesuffix", + }; + write!(f, "{representation}") } - result -} - -/// Remove duplicate characters from a raw string. -fn deduplicate_raw(s: &str) -> String { - let mut result = String::new(); - let mut seen = FxHashSet::default(); - for ch in s.chars() { - if !seen.insert(ch) { - continue; - } - result.push(ch); - } - result } /// Return `true` if a string contains duplicate characters, taking into account escapes. @@ -127,64 +118,24 @@ pub fn bad_str_strip_call(checker: &mut Checker, func: &Expr, args: &[Expr]) { .. } ) { - if let Some(kind) = StripKind::from_str(attr.as_str()) { + if let Some(strip) = StripKind::from_str(attr.as_str()) { if let Some(arg) = args.get(0) { if let ExprKind::Constant { value: Constant::Str(value), .. } = &arg.node { - let is_multiline = arg.location.row() != arg.end_location.unwrap().row(); - - let module_text = checker - .locator - .slice_source_code_range(&Range::from_located(arg)); - - if !is_multiline - && lexer::make_tokenizer_located(module_text, arg.location) - .flatten() - .filter(|(_, tok, _)| matches!(tok, Tok::String { .. })) - .nth(1) - .is_none() - { - // If we have a single string (no implicit concatenation), fix it. - let Some(leading_quote) = leading_quote(module_text) else { - return; + if has_duplicates(value) { + let removal = if checker.settings.target_version >= PythonVersion::Py39 + { + RemovalKind::for_strip(&strip) + } else { + None }; - let Some(trailing_quote) = trailing_quote(module_text) else { - return; - }; - let content = &module_text - [leading_quote.len()..module_text.len() - trailing_quote.len()]; - - let deduplicated = - if leading_quote.contains('r') || leading_quote.contains('R') { - deduplicate_raw(content) - } else { - deduplicate_escaped(content) - }; - if content != deduplicated { - let mut diagnostic = Diagnostic::new( - BadStrStripCall { kind }, - Range::from_located(arg), - ); - if checker.patch(diagnostic.kind.rule()) { - diagnostic.amend(Fix::replacement( - format!("{leading_quote}{deduplicated}{trailing_quote}"), - arg.location, - arg.end_location.unwrap(), - )); - }; - checker.diagnostics.push(diagnostic); - } - } else { - // Otherwise, let's just look for duplicates. - if has_duplicates(value) { - checker.diagnostics.push(Diagnostic::new( - BadStrStripCall { kind }, - Range::from_located(arg), - )); - } + checker.diagnostics.push(Diagnostic::new( + BadStrStripCall { strip, removal }, + Range::from_located(arg), + )); } } } diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap index a0c3e2f1cb..0aaab144b6 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE1310_bad_str_strip_call.py.snap @@ -5,177 +5,115 @@ expression: diagnostics - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 2 column: 20 end_location: row: 2 column: 27 - fix: - content: - - "\"Helo\"" - location: - row: 2 - column: 20 - end_location: - row: 2 - column: 27 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 5 column: 20 end_location: row: 5 column: 27 - fix: - content: - - "\"Helo\"" - location: - row: 5 - column: 20 - end_location: - row: 5 - column: 27 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 8 column: 20 end_location: row: 8 column: 28 - fix: - content: - - "u\"Helo\"" - location: - row: 8 - column: 20 - end_location: - row: 8 - column: 28 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 11 column: 20 end_location: row: 11 column: 28 - fix: - content: - - "r\"Helo\"" - location: - row: 11 - column: 20 - end_location: - row: 11 - column: 28 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 14 column: 20 end_location: row: 14 column: 29 - fix: - content: - - "\"Helo\\t\"" - location: - row: 14 - column: 20 - end_location: - row: 14 - column: 29 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 17 column: 20 end_location: row: 17 column: 30 - fix: - content: - - "r\"Helo\\t\"" - location: - row: 17 - column: 20 - end_location: - row: 17 - column: 30 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 20 column: 20 end_location: row: 20 column: 29 - fix: - content: - - "\"Helo\\\\\"" - location: - row: 20 - column: 20 - end_location: - row: 20 - column: 29 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 23 column: 20 end_location: row: 23 column: 30 - fix: - content: - - "r\"Helo\\\"" - location: - row: 23 - column: 20 - end_location: - row: 23 - column: 30 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 26 column: 20 end_location: row: 26 column: 29 - fix: - content: - - "\"🤣🙃👀😀\"" - location: - row: 26 - column: 20 - end_location: - row: 26 - column: 29 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 30 column: 4 @@ -187,6 +125,7 @@ expression: diagnostics - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 36 column: 20 @@ -198,6 +137,7 @@ expression: diagnostics - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 42 column: 4 @@ -209,6 +149,7 @@ expression: diagnostics - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 49 column: 4 @@ -220,58 +161,37 @@ expression: diagnostics - kind: BadStrStripCall: kind: Strip + suggestion: ~ location: row: 61 column: 10 end_location: row: 61 column: 19 - fix: - content: - - "'htp:/'" - location: - row: 61 - column: 10 - end_location: - row: 61 - column: 19 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: LStrip + suggestion: RemovePrefix location: row: 64 column: 11 end_location: row: 64 column: 20 - fix: - content: - - "'htp:/'" - location: - row: 64 - column: 11 - end_location: - row: 64 - column: 20 + fix: ~ parent: ~ - kind: BadStrStripCall: kind: RStrip + suggestion: RemoveSuffix location: row: 67 column: 11 end_location: row: 67 column: 20 - fix: - content: - - "'htp:/'" - location: - row: 67 - column: 11 - end_location: - row: 67 - column: 20 + fix: ~ parent: ~