Skip to content

Commit

Permalink
Still fix, but splat
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Jan 28, 2024
1 parent f78f324 commit 15307e4
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 42 deletions.
75 changes: 35 additions & 40 deletions crates/ruff_linter/src/rules/flake8_pie/rules/multiple_starts_ends_with.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}

Expand All @@ -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)
Expand All @@ -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<Expr> = indices
.iter()
.map(|index| &values[*index])
.map(|expr| {
Expand All @@ -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(),
Expand Down Expand Up @@ -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;
};
Expand All @@ -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)
}
82 changes: 80 additions & 2 deletions ...c/rules/flake8_pie/snapshots/ruff_linter__rules__flake8_pie__tests__PIE810_PIE810.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand All @@ -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")


0 comments on commit 15307e4

Please sign in to comment.