From 15307e4e83b8c42d85c21af500721a8555e8c14d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 28 Jan 2024 14:17:47 -0500 Subject: [PATCH] Still fix, but splat --- .../rules/multiple_starts_ends_with.rs | 75 ++++++++--------- ...__flake8_pie__tests__PIE810_PIE810.py.snap | 82 ++++++++++++++++++- 2 files changed, 115 insertions(+), 42 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pie/rules/multiple_starts_ends_with.rs b/crates/ruff_linter/src/rules/flake8_pie/rules/multiple_starts_ends_with.rs index 8b3e0a321504fb..b45f6be8eb00ba 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/rules/multiple_starts_ends_with.rs +++ b/crates/ruff_linter/src/rules/flake8_pie/rules/multiple_starts_ends_with.rs @@ -3,7 +3,7 @@ use std::iter; use itertools::Either::{Left, Right}; -use ruff_python_semantic::SemanticModel; +use ruff_python_semantic::{analyze, SemanticModel}; use ruff_text_size::{Ranged, TextRange}; use ruff_python_ast::{self as ast, Arguments, BoolOp, Expr, ExprContext, Identifier}; @@ -37,6 +37,14 @@ use crate::checkers::ast::Checker; /// print("Greetings!") /// ``` /// +/// ## Fix safety +/// This rule's fix is unsafe, as in some cases, it will be unable to determine +/// whether the argument to an existing `.startswith` or `.endswith` call is a +/// tuple. For example, given `msg.startswith(x) or msg.startswith(y)`, if `x` +/// or `y` is a tuple, and the semantic model is unable to detect it as such, +/// the rule will suggest `msg.startswith((x, y))`, which will error at +/// runtime. +/// /// ## References /// - [Python documentation: `str.startswith`](https://docs.python.org/3/library/stdtypes.html#str.startswith) /// - [Python documentation: `str.endswith`](https://docs.python.org/3/library/stdtypes.html#str.endswith) @@ -85,7 +93,11 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) { continue; }; - if !(args.len() == 1 && keywords.is_empty()) { + if !keywords.is_empty() { + continue; + } + + if args.len() != 1 { continue; } @@ -100,17 +112,6 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) { continue; }; - let Some(arg) = args.first() else { - continue; - }; - - // Check if the argument is a tuple of strings. If so, we will exclude it from the check. - // This is because we don't want to suggest merging a tuple of strings into a tuple. - // Which violates the conctract of startswith and endswith. - if matches_to_tuple_of_strs(arg, checker.semantic()) { - continue; - } - duplicates .entry((attr.as_str(), arg_name.as_str())) .or_insert_with(Vec::new) @@ -126,7 +127,7 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) { }, expr.range(), ); - let words: Vec<&Expr> = indices + let words: Vec = indices .iter() .map(|index| &values[*index]) .map(|expr| { @@ -146,22 +147,32 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) { format!("Indices should only contain `{attr_name}` calls") ) }; - args.first() - .unwrap_or_else(|| panic!("`{attr_name}` should have one argument")) + let arg = args + .first() + .unwrap_or_else(|| panic!("`{attr_name}` should have one argument")); + + if is_bound_to_tuple(arg, checker.semantic()) { + Expr::Starred(ast::ExprStarred { + value: Box::new(arg.clone()), + ctx: ExprContext::Load, + range: TextRange::default(), + }) + } else { + arg.clone() + } }) .collect(); let node = Expr::Tuple(ast::ExprTuple { elts: words - .iter() + .into_iter() .flat_map(|value| { if let Expr::Tuple(ast::ExprTuple { elts, .. }) = value { - Left(elts.iter()) + Left(elts.into_iter()) } else { - Right(iter::once(*value)) + Right(iter::once(value)) } }) - .map(Clone::clone) .collect(), ctx: ExprContext::Load, range: TextRange::default(), @@ -215,11 +226,8 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) { } } -fn matches_to_tuple_of_strs(arg: &Expr, semantic: &SemanticModel) -> bool { - if is_tuple_of_strs(arg) { - return true; - } - +/// Returns `true` if the expression definitively resolves to a tuple (e.g., `x` in `x = (1, 2)`). +fn is_bound_to_tuple(arg: &Expr, semantic: &SemanticModel) -> bool { let Expr::Name(ast::ExprName { id, .. }) = arg else { return false; }; @@ -228,20 +236,7 @@ fn matches_to_tuple_of_strs(arg: &Expr, semantic: &SemanticModel) -> bool { return false; }; - if let Some(statement_id) = semantic.binding(binding_id).source { - let stmt = semantic.statement(statement_id); - if let ast::Stmt::Assign(ast::StmtAssign { targets, value, .. }) = stmt { - if targets.len() == 1 { - return is_tuple_of_strs(value); - } - } - } - false -} + let binding = semantic.binding(binding_id); -fn is_tuple_of_strs(arg: &Expr) -> bool { - if let Expr::Tuple(ast::ExprTuple { elts, .. }) = arg { - return elts.iter().all(|elt| matches!(elt, Expr::StringLiteral(_))); - } - false + analyze::typing::is_tuple(binding, semantic) } diff --git a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE810_PIE810.py.snap b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE810_PIE810.py.snap index d94daa662bd8bb..ea6fddc81091c9 100644 --- a/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE810_PIE810.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE810_PIE810.py.snap @@ -118,7 +118,7 @@ PIE810.py:19:8: PIE810 [*] Call `startswith` once with a `tuple` 17 17 | z = "w" 18 18 | 19 |- if msg.startswith(x) or msg.startswith(y) or msg.startswith(z): # Error - 19 |+ if msg.startswith((x, z)) or msg.startswith(y): # Error + 19 |+ if msg.startswith((x, *y, z)): # Error 20 20 | print("yes") 21 21 | 22 22 | def func(): @@ -138,9 +138,87 @@ PIE810.py:25:8: PIE810 [*] Call `startswith` once with a `tuple` 23 23 | msg = "hello world" 24 24 | 25 |- if msg.startswith(("h", "e", "l", "l", "o")) or msg.startswith("h") or msg.startswith("w"): # Error - 25 |+ if msg.startswith(("h", "e", "l", "l", "o")) or msg.startswith(("h", "w")): # Error + 25 |+ if msg.startswith(("h", "e", "l", "l", "o", "h", "w")): # Error 26 26 | print("yes") 27 27 | 28 28 | # ok +PIE810.py:43:8: PIE810 [*] Call `startswith` once with a `tuple` + | +41 | y = ("h", "e", "l", "l", "o") +42 | +43 | if msg.startswith(x) or msg.startswith(y): # OK + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810 +44 | print("yes") + | + = help: Merge into a single `startswith` call + +ℹ Unsafe fix +40 40 | x = "h" +41 41 | y = ("h", "e", "l", "l", "o") +42 42 | +43 |- if msg.startswith(x) or msg.startswith(y): # OK + 43 |+ if msg.startswith((x, *y)): # OK +44 44 | print("yes") +45 45 | +46 46 | def func(): + +PIE810.py:59:8: PIE810 [*] Call `startswith` once with a `tuple` + | +57 | y = ("h", "e", "l", "l", "o") +58 | +59 | if msg.startswith(y) or msg.startswith(y): # OK + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810 +60 | print("yes") + | + = help: Merge into a single `startswith` call + +ℹ Unsafe fix +56 56 | +57 57 | y = ("h", "e", "l", "l", "o") +58 58 | +59 |- if msg.startswith(y) or msg.startswith(y): # OK + 59 |+ if msg.startswith((*y, *y)): # OK +60 60 | print("yes") +61 61 | +62 62 | def func(): + +PIE810.py:68:8: PIE810 [*] Call `startswith` once with a `tuple` + | +66 | x = ("w", "o", "r", "l", "d") +67 | +68 | if msg.startswith(y) or msg.startswith(x) or msg.startswith("h"): # OK + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810 +69 | print("yes") + | + = help: Merge into a single `startswith` call + +ℹ Unsafe fix +65 65 | y = ("h", "e", "l", "l", "o") +66 66 | x = ("w", "o", "r", "l", "d") +67 67 | +68 |- if msg.startswith(y) or msg.startswith(x) or msg.startswith("h"): # OK + 68 |+ if msg.startswith((*y, *x, "h")): # OK +69 69 | print("yes") +70 70 | +71 71 | def func(): + +PIE810.py:77:8: PIE810 [*] Call `startswith` once with a `tuple` + | +75 | x = ("w", "o", "r", "l", "d") +76 | +77 | if msg.startswith(y) or msg.endswith(x) or msg.startswith("h"): # OK + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810 +78 | print("yes") + | + = help: Merge into a single `startswith` call + +ℹ Unsafe fix +74 74 | y = ("h", "e", "l", "l", "o") +75 75 | x = ("w", "o", "r", "l", "d") +76 76 | +77 |- if msg.startswith(y) or msg.endswith(x) or msg.startswith("h"): # OK + 77 |+ if msg.startswith((*y, "h")) or msg.endswith(x): # OK +78 78 | print("yes") +