From 1c911c89e718c0a4fcea388333ceaeb3919d0b8b Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Fri, 7 Feb 2025 21:37:02 +0000 Subject: [PATCH 1/2] [`pylint`] Also report when the object isn't a literal (`PLE1310`) --- crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../rules/pylint/rules/bad_str_strip_call.rs | 24 ++- ...review__PLE1310_bad_str_strip_call.py.snap | 201 ++++++++++++++++++ .../src/analyze/typing.rs | 26 +++ 4 files changed, 250 insertions(+), 2 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index fc0ed99f6c5d1..79bb1eafb7ea2 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -441,6 +441,7 @@ mod tests { Path::new("repeated_equality_comparison.py") )] #[test_case(Rule::InvalidEnvvarDefault, Path::new("invalid_envvar_default.py"))] + #[test_case(Rule::BadStrStripCall, Path::new("bad_str_strip_call.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", 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 5f50cb5a62325..d45867355837e 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 @@ -5,6 +5,8 @@ use rustc_hash::FxHashSet; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; +use ruff_python_semantic::analyze::typing; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -72,10 +74,20 @@ pub(crate) enum ValueKind { } impl ValueKind { - fn from(expr: &Expr) -> Option { + fn from(expr: &Expr, semantic: &SemanticModel) -> Option { match expr { Expr::StringLiteral(_) => Some(Self::String), Expr::BytesLiteral(_) => Some(Self::Bytes), + Expr::Name(name) => { + let binding_id = semantic.only_binding(name)?; + let binding = semantic.binding(binding_id); + + match () { + () if typing::is_string(binding, semantic) => Some(Self::String), + () if typing::is_bytes(binding, semantic) => Some(Self::Bytes), + () => None, + } + } _ => None, } } @@ -171,7 +183,15 @@ pub(crate) fn bad_str_strip_call(checker: &Checker, call: &ast::ExprCall) { return; }; - let Some(value_kind) = ValueKind::from(value.as_ref()) else { + let value = value.as_ref(); + + if checker.settings.preview.is_disabled() + && !matches!(value, Expr::StringLiteral(_) | Expr::BytesLiteral(_)) + { + return; + } + + let Some(value_kind) = ValueKind::from(value, checker.semantic()) else { return; }; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap new file mode 100644 index 0000000000000..17822a0d0526c --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap @@ -0,0 +1,201 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +bad_str_strip_call.py:2:21: PLE1310 String `strip` call contains duplicate characters + | +1 | # PLE1310 +2 | "Hello World".strip("Hello") + | ^^^^^^^ PLE1310 +3 | +4 | # PLE1310 + | + +bad_str_strip_call.py:5:21: PLE1310 String `strip` call contains duplicate characters + | +4 | # PLE1310 +5 | "Hello World".strip("Hello") + | ^^^^^^^ PLE1310 +6 | +7 | # PLE1310 + | + +bad_str_strip_call.py:8:21: PLE1310 String `strip` call contains duplicate characters + | + 7 | # PLE1310 + 8 | "Hello World".strip(u"Hello") + | ^^^^^^^^ PLE1310 + 9 | +10 | # PLE1310 + | + +bad_str_strip_call.py:11:21: PLE1310 String `strip` call contains duplicate characters + | +10 | # PLE1310 +11 | "Hello World".strip(r"Hello") + | ^^^^^^^^ PLE1310 +12 | +13 | # PLE1310 + | + +bad_str_strip_call.py:14:21: PLE1310 String `strip` call contains duplicate characters + | +13 | # PLE1310 +14 | "Hello World".strip("Hello\t") + | ^^^^^^^^^ PLE1310 +15 | +16 | # PLE1310 + | + +bad_str_strip_call.py:17:21: PLE1310 String `strip` call contains duplicate characters + | +16 | # PLE1310 +17 | "Hello World".strip(r"Hello\t") + | ^^^^^^^^^^ PLE1310 +18 | +19 | # PLE1310 + | + +bad_str_strip_call.py:20:21: PLE1310 String `strip` call contains duplicate characters + | +19 | # PLE1310 +20 | "Hello World".strip("Hello\\") + | ^^^^^^^^^ PLE1310 +21 | +22 | # PLE1310 + | + +bad_str_strip_call.py:23:21: PLE1310 String `strip` call contains duplicate characters + | +22 | # PLE1310 +23 | "Hello World".strip(r"Hello\\") + | ^^^^^^^^^^ PLE1310 +24 | +25 | # PLE1310 + | + +bad_str_strip_call.py:26:21: PLE1310 String `strip` call contains duplicate characters + | +25 | # PLE1310 +26 | "Hello World".strip("🤣🤣🤣🤣🙃👀😀") + | ^^^^^^^^^^^^^^^^ PLE1310 +27 | +28 | # PLE1310 + | + +bad_str_strip_call.py:30:5: PLE1310 String `strip` call contains duplicate characters + | +28 | # PLE1310 +29 | "Hello World".strip( +30 | / """ +31 | | there are a lot of characters to strip +32 | | """ + | |___^ PLE1310 +33 | ) + | + +bad_str_strip_call.py:36:21: PLE1310 String `strip` call contains duplicate characters + | +35 | # PLE1310 +36 | "Hello World".strip("can we get a long " \ + | _____________________^ +37 | | "string of characters to strip " \ +38 | | "please?") + | |_____________________________^ PLE1310 +39 | +40 | # PLE1310 + | + +bad_str_strip_call.py:42:5: PLE1310 String `strip` call contains duplicate characters + | +40 | # PLE1310 +41 | "Hello World".strip( +42 | / "can we get a long " +43 | | "string of characters to strip " +44 | | "please?" + | |_____________^ PLE1310 +45 | ) + | + +bad_str_strip_call.py:49:5: PLE1310 String `strip` call contains duplicate characters + | +47 | # PLE1310 +48 | "Hello World".strip( +49 | / "can \t we get a long" +50 | | "string \t of characters to strip" +51 | | "please?" + | |_____________^ PLE1310 +52 | ) + | + +bad_str_strip_call.py:61:11: PLE1310 String `strip` call contains duplicate characters + | +60 | # PLE1310 +61 | u''.strip('http://') + | ^^^^^^^^^ PLE1310 +62 | +63 | # PLE1310 + | + +bad_str_strip_call.py:64:12: PLE1310 String `lstrip` call contains duplicate characters (did you mean `removeprefix`?) + | +63 | # PLE1310 +64 | u''.lstrip('http://') + | ^^^^^^^^^ PLE1310 +65 | +66 | # PLE1310 + | + +bad_str_strip_call.py:67:12: PLE1310 String `rstrip` call contains duplicate characters (did you mean `removesuffix`?) + | +66 | # 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 + | + +bad_str_strip_call.py:101:12: PLE1310 String `rstrip` call contains duplicate characters (did you mean `removesuffix`?) + | +100 | # False negative +101 | foo.rstrip("//") + | ^^^^ PLE1310 +102 | bar.lstrip(b"//") + | + +bad_str_strip_call.py:102:12: PLE1310 String `lstrip` call contains duplicate characters (did you mean `removeprefix`?) + | +100 | # False negative +101 | foo.rstrip("//") +102 | bar.lstrip(b"//") + | ^^^^^ PLE1310 +103 | +104 | # OK: Not `.[lr]?strip` + | diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index 2b4d644f3607c..dbe9fad7cb4e0 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -741,6 +741,22 @@ impl BuiltinTypeChecker for SetChecker { const EXPR_TYPE: PythonType = PythonType::Set; } +struct StringChecker; + +impl BuiltinTypeChecker for StringChecker { + const BUILTIN_TYPE_NAME: &'static str = "str"; + const TYPING_NAME: Option<&'static str> = None; + const EXPR_TYPE: PythonType = PythonType::String; +} + +struct BytesChecker; + +impl BuiltinTypeChecker for BytesChecker { + const BUILTIN_TYPE_NAME: &'static str = "bytes"; + const TYPING_NAME: Option<&'static str> = None; + const EXPR_TYPE: PythonType = PythonType::String; +} + struct TupleChecker; impl BuiltinTypeChecker for TupleChecker { @@ -984,6 +1000,16 @@ pub fn is_float(binding: &Binding, semantic: &SemanticModel) -> bool { check_type::(binding, semantic) } +/// Test whether the given binding can be considered an instance of `str`. +pub fn is_string(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) +} + +/// Test whether the given binding can be considered an instance of `bytes`. +pub fn is_bytes(binding: &Binding, semantic: &SemanticModel) -> bool { + check_type::(binding, semantic) +} + /// Test whether the given binding can be considered a set. /// /// For this, we check what value might be associated with it through it's initialization and From ceb958451528c68ca2228ec0a1249ddef96ebbf2 Mon Sep 17 00:00:00 2001 From: InSyncWithFoo Date: Sun, 9 Feb 2025 20:23:26 +0000 Subject: [PATCH 2/2] Per review --- .../fixtures/pylint/bad_str_strip_call.py | 15 ++++-- .../rules/pylint/rules/bad_str_strip_call.rs | 12 +++-- ..._tests__PLE1310_bad_str_strip_call.py.snap | 2 +- ...review__PLE1310_bad_str_strip_call.py.snap | 49 +++++++++++-------- .../src/analyze/typing.rs | 2 +- 5 files changed, 48 insertions(+), 32 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 index feb9fd853132c..9beed03eadb55 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 @@ -80,6 +80,16 @@ ''.strip(r'\b\x09') ''.strip('\\\x5C') +# Errors: Type inference +b = b'' +b.strip(b'//') + +# Errors: Type inference (preview) +foo: str = ""; bar: bytes = b"" +foo.rstrip("//") +bar.lstrip(b"//") + + # OK: Different types b"".strip("//") "".strip(b"//") @@ -93,13 +103,8 @@ "".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/rules/pylint/rules/bad_str_strip_call.rs b/crates/ruff_linter/src/rules/pylint/rules/bad_str_strip_call.rs index d45867355837e..9ed65dc8eb9e2 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 @@ -82,10 +82,12 @@ impl ValueKind { let binding_id = semantic.only_binding(name)?; let binding = semantic.binding(binding_id); - match () { - () if typing::is_string(binding, semantic) => Some(Self::String), - () if typing::is_bytes(binding, semantic) => Some(Self::Bytes), - () => None, + if typing::is_string(binding, semantic) { + Some(Self::String) + } else if typing::is_bytes(binding, semantic) { + Some(Self::Bytes) + } else { + None } } _ => None, @@ -183,7 +185,7 @@ pub(crate) fn bad_str_strip_call(checker: &Checker, call: &ast::ExprCall) { return; }; - let value = value.as_ref(); + let value = &**value; if checker.settings.preview.is_disabled() && !matches!(value, Expr::StringLiteral(_) | Expr::BytesLiteral(_)) 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 c6d17717c50b0..d017b7b062095 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 @@ -179,5 +179,5 @@ bad_str_strip_call.py:81:10: PLE1310 String `strip` call contains duplicate char 81 | ''.strip('\\\x5C') | ^^^^^^^^ PLE1310 82 | -83 | # OK: Different types +83 | # Errors: Type inference | diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap index 17822a0d0526c..a3d6f646c0127 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__preview__PLE1310_bad_str_strip_call.py.snap @@ -179,23 +179,32 @@ bad_str_strip_call.py:81:10: PLE1310 String `strip` call contains duplicate char 81 | ''.strip('\\\x5C') | ^^^^^^^^ PLE1310 82 | -83 | # OK: Different types - | - -bad_str_strip_call.py:101:12: PLE1310 String `rstrip` call contains duplicate characters (did you mean `removesuffix`?) - | -100 | # False negative -101 | foo.rstrip("//") - | ^^^^ PLE1310 -102 | bar.lstrip(b"//") - | - -bad_str_strip_call.py:102:12: PLE1310 String `lstrip` call contains duplicate characters (did you mean `removeprefix`?) - | -100 | # False negative -101 | foo.rstrip("//") -102 | bar.lstrip(b"//") - | ^^^^^ PLE1310 -103 | -104 | # OK: Not `.[lr]?strip` - | +83 | # Errors: Type inference + | + +bad_str_strip_call.py:85:9: PLE1310 String `strip` call contains duplicate characters + | +83 | # Errors: Type inference +84 | b = b'' +85 | b.strip(b'//') + | ^^^^^ PLE1310 +86 | +87 | # Errors: Type inference (preview) + | + +bad_str_strip_call.py:89:12: PLE1310 String `rstrip` call contains duplicate characters (did you mean `removesuffix`?) + | +87 | # Errors: Type inference (preview) +88 | foo: str = ""; bar: bytes = b"" +89 | foo.rstrip("//") + | ^^^^ PLE1310 +90 | bar.lstrip(b"//") + | + +bad_str_strip_call.py:90:12: PLE1310 String `lstrip` call contains duplicate characters (did you mean `removeprefix`?) + | +88 | foo: str = ""; bar: bytes = b"" +89 | foo.rstrip("//") +90 | bar.lstrip(b"//") + | ^^^^^ PLE1310 + | diff --git a/crates/ruff_python_semantic/src/analyze/typing.rs b/crates/ruff_python_semantic/src/analyze/typing.rs index dbe9fad7cb4e0..c9d1858eccac5 100644 --- a/crates/ruff_python_semantic/src/analyze/typing.rs +++ b/crates/ruff_python_semantic/src/analyze/typing.rs @@ -754,7 +754,7 @@ struct BytesChecker; impl BuiltinTypeChecker for BytesChecker { const BUILTIN_TYPE_NAME: &'static str = "bytes"; const TYPING_NAME: Option<&'static str> = None; - const EXPR_TYPE: PythonType = PythonType::String; + const EXPR_TYPE: PythonType = PythonType::Bytes; } struct TupleChecker;