Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PIE810 gives wrong suggestions #8906

Closed
deliro opened this issue Nov 29, 2023 · 5 comments
Closed

PIE810 gives wrong suggestions #8906

deliro opened this issue Nov 29, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@deliro
Copy link

deliro commented Nov 29, 2023

Hey! Given the code:

import sys

if __name__ == "__main__":
    x = ("hello", "world")
    y = "foo"

    msg = "abracadabra"

    if msg.startswith(x) or msg.startswith(y):
        sys.exit(1)

Ruff suggests (ruff check x.py):

x.py:9:8: PIE810 Call `startswith` once with a `tuple`

The problem is x is a tuple and the expression cannot be combined to msg.startswith((x, y))

Ruff settings
[tool.ruff]
line-length = 120
target-version = "py311"
exclude = ["*migrations*"]
select = [
    "E", # pyflakes
    "F", # pycodestyle errors
    "W", # pycodestyle warnings
    "UP", # pyupgrade
    "I", # isort
    "C4", # flake8-comprehensions
    # pytest
    "PT018", # Assertion should be broken down into multiple parts
    "PT022", # No teardown in fixture {name}, use return instead of yield

    "T10", # flake8-debugger
    "SIM", # flake8-simplify
    "ISC", # flake8-implicit-str-concat
    "FA", # flake8-future-annotations
    "PIE", # flake8-pie
    "T20", # flake8-print
    "RET", # flake8-return
    "FLY", # flynt
]
ignore = [
    "E741", # Ambiguous variable name
    "SIM115", # Use context handler for opening files
    "SIM116", # Use a dictionary instead of consecutive `if` statements
    "RET501", # Do not explicitly return None in function if it is the only possible return value
    # broken ones in RET:
    "RET505", # Unnecessary `elif` after `return` statement
    "RET506", # Unnecessary `elif` after `raise` statement
    "RET507", # Unnecessary `elif` after `continue` statement
    "RET508", # Unnecessary {branch} after break statement
]
Ruff version

ruff 0.1.6 (f460f9c 2023-11-17)

@prokie
Copy link

prokie commented Nov 29, 2023

I guess you can do like this?

if msg.startswith(x + (y,)):
    sys.exit(1)

@deliro
Copy link
Author

deliro commented Nov 29, 2023

I can, but the point is ruff suggests invalid code that will throw

@charliermarsh charliermarsh added the bug Something isn't working label Nov 29, 2023
@charliermarsh
Copy link
Member

Makes sense, thanks!

@dhruvmanila
Copy link
Member

I guess one solution would be to avoid raising the violation if we're sure that it's a collection type like the mentioned code:

x = ["a", "b"]
y = "c"

msg.startswith(x) or msg.startswith(y)

But, if there are more than 1 startswith calls which has non-collection type or an unknown type, then we can raise an error:

x = ["a", "b"]
y = "c"
z = "d"

msg.startswith(x) or msg.startswith(y) or msg.startswith(z)
#                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#                    Raise here

@charliermarsh
Copy link
Member

Closed by #9661.

charliermarsh pushed a commit that referenced this issue Jan 28, 2024
…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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants