Skip to content

Commit

Permalink
[flake8-pie] Omit bound tuples passed to .startswith or `.endswit…
Browse files Browse the repository at this point in the history
…h` (#9661)

## Summary

This review contains a fix for
[PIE810](https://docs.astral.sh/ruff/rules/multiple-starts-ends-with/)
(multiple-starts-ends-with)

The problem is that ruff suggests combining multiple startswith/endswith
calls into a single call even though there might be a call with tuple of
strs. This leads to calling startswith/endswith with tuple of tuple of
strs which is incorrect and violates startswith/endswith conctract and
results in runtime failure.

However the following will be valid and fixed correctly => 
```python
x = ("hello", "world")
y = "h"
z = "w"
msg = "hello world"

if msg.startswith(x) or msg.startswith(y) or msg.startswith(z) :
      sys.exit(1)
```
```
ruff --fix --select PIE810 --unsafe-fixes
```
=> 
```python
if msg.startswith(x) or msg.startswith((y,z)):
      sys.exit(1)
```

See: #8906

## Test Plan

```bash
cargo test
```
  • Loading branch information
mikeleppane committed Jan 28, 2024
1 parent 7329bf4 commit b9139a3
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 5 deletions.
59 changes: 59 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pie/PIE810.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,22 @@
# error
obj.endswith(foo) or obj.startswith(foo) or obj.startswith("foo")

def func():
msg = "hello world"

x = "h"
y = ("h", "e", "l", "l", "o")
z = "w"

if msg.startswith(x) or msg.startswith(y) or msg.startswith(z): # Error
print("yes")

def func():
msg = "hello world"

if msg.startswith(("h", "e", "l", "l", "o")) or msg.startswith("h") or msg.startswith("w"): # Error
print("yes")

# ok
obj.startswith(("foo", "bar"))
# ok
Expand All @@ -17,3 +33,46 @@
obj.startswith("foo") or obj.endswith("bar")
# ok
obj.startswith("foo") or abc.startswith("bar")

def func():
msg = "hello world"

x = "h"
y = ("h", "e", "l", "l", "o")

if msg.startswith(x) or msg.startswith(y): # OK
print("yes")

def func():
msg = "hello world"

y = ("h", "e", "l", "l", "o")

if msg.startswith(y): # OK
print("yes")

def func():
msg = "hello world"

y = ("h", "e", "l", "l", "o")

if msg.startswith(y) or msg.startswith(y): # OK
print("yes")

def func():
msg = "hello world"

y = ("h", "e", "l", "l", "o")
x = ("w", "o", "r", "l", "d")

if msg.startswith(y) or msg.startswith(x) or msg.startswith("h"): # OK
print("yes")

def func():
msg = "hello world"

y = ("h", "e", "l", "l", "o")
x = ("w", "o", "r", "l", "d")

if msg.startswith(y) or msg.endswith(x) or msg.startswith("h"): # OK
print("yes")
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::iter;

use itertools::Either::{Left, Right};

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 @@ -36,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 @@ -84,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 @@ -99,6 +112,13 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) {
continue;
};

// 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;
}

duplicates
.entry((attr.as_str(), arg_name.as_str()))
.or_insert_with(Vec::new)
Expand Down Expand Up @@ -149,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 @@ -202,3 +222,18 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) {
}
}
}

/// 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;
};

let Some(binding_id) = semantic.lookup_symbol(id.as_str()) else {
return false;
};

let binding = semantic.binding(binding_id);

analyze::typing::is_tuple(binding, semantic)
}
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ PIE810.py:10:1: PIE810 [*] Call `startswith` once with a `tuple`
10 | obj.endswith(foo) or obj.startswith(foo) or obj.startswith("foo")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810
11 |
12 | # ok
12 | def func():
|
= help: Merge into a single `startswith` call

Expand All @@ -100,7 +100,47 @@ PIE810.py:10:1: PIE810 [*] Call `startswith` once with a `tuple`
10 |-obj.endswith(foo) or obj.startswith(foo) or obj.startswith("foo")
10 |+obj.endswith(foo) or obj.startswith((foo, "foo"))
11 11 |
12 12 | # ok
13 13 | obj.startswith(("foo", "bar"))
12 12 | def func():
13 13 | msg = "hello world"

PIE810.py:19:8: PIE810 [*] Call `startswith` once with a `tuple`
|
17 | z = "w"
18 |
19 | if msg.startswith(x) or msg.startswith(y) or msg.startswith(z): # Error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810
20 | print("yes")
|
= help: Merge into a single `startswith` call

Unsafe fix
16 16 | y = ("h", "e", "l", "l", "o")
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
20 20 | print("yes")
21 21 |
22 22 | def func():

PIE810.py:25:8: PIE810 [*] Call `startswith` once with a `tuple`
|
23 | msg = "hello world"
24 |
25 | if msg.startswith(("h", "e", "l", "l", "o")) or msg.startswith("h") or msg.startswith("w"): # Error
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PIE810
26 | print("yes")
|
= help: Merge into a single `startswith` call

Unsafe fix
22 22 | def func():
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", "h", "w")): # Error
26 26 | print("yes")
27 27 |
28 28 | # ok


0 comments on commit b9139a3

Please sign in to comment.