Skip to content

Commit

Permalink
Still fix, but splat
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jan 28, 2024
1 parent f78f324 commit d02dc26
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 32 deletions.
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,10 +93,14 @@ 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;
}

let [arg] = args.as_slice() else {
continue;
};

let Expr::Attribute(ast::ExprAttribute { value, attr, .. }) = func.as_ref() else {
continue;
};
Expand All @@ -100,14 +112,10 @@ 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()) {
// If the argument is bound to a tuple, skip it, since we don't want to suggest
// `startswith((x, y))` where `x` or `y` are tuples. (Tuple literals are okay, since we
// inline them below.)
if is_bound_to_tuple(arg, checker.semantic()) {
continue;
}

Expand Down Expand Up @@ -161,7 +169,7 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) {
Right(iter::once(*value))
}
})
.map(Clone::clone)
.cloned()
.collect(),
ctx: ExprContext::Load,
range: TextRange::default(),
Expand Down Expand Up @@ -215,11 +223,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 +233,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)
}
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ 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
Expand Down

0 comments on commit d02dc26

Please sign in to comment.