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

False positive for PLR1706 #9007

Closed
UnknownPlatypus opened this issue Dec 5, 2023 · 6 comments · Fixed by #9680
Closed

False positive for PLR1706 #9007

UnknownPlatypus opened this issue Dec 5, 2023 · 6 comments · Fixed by #9680
Assignees
Labels
bug Something isn't working
Milestone

Comments

@UnknownPlatypus
Copy link

Here is a minimal example for the issue I had with PLR1706 and the autofix using ruff v0.1.7.

-print(True and False or 2) # This print '2'
+print(False if True else 2) # This print 'False'

I used this command:

echo "print(True and False or 2)" > t.py
ruff check --select PLR1706 --isolated --preview --fix --unsafe-fixes t.py

Generally speaking, A and B or C is transformed into B if A else C but this is not equivalent when A is truphy and B is falsy because the first one returns C and the second returns B.

Let me know if I'm missing something.

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

Does look like a bug, thanks for filing.

@charliermarsh
Copy link
Member

It turns out that this rule relies on type inference. If B is false-y, we shouldn't be triggering it.

@charliermarsh
Copy link
Member

Specifically, you have to be able to resolve B to a specific value.

Even x >= y and x or y is not safe to convert when x and y are both integers -- it fails for x, y = 0, -1. (Pylint correctly does not flag this example, but I'm more pointing out that it's a very limited rule.)

We may want to deprecate.

@UnknownPlatypus
Copy link
Author

Ye I feel like with the current infrastructure it might be hard to get this right.
This rule is targeting a very limited use case anyway since it's supposed to be a pattern for python <2.5.

I would probably deprecate it too, and remove the autofix altogether (it only introduced behavior changes when I tried it myself so I'd rather let people do manual changes if they really want the rule).

Anyway thanks for the quick reply and it's a pleasure to use ruff.

@charliermarsh
Copy link
Member

👍 Makes sense. I'll mark it in the 0.2.0 issue. Thanks for the kind words, I appreciate it.

@zanieb
Copy link
Member

zanieb commented Jan 30, 2024

See #9691 which removes this rule

zanieb added a commit that referenced this issue Jan 30, 2024
Similar to #9689 — retains removed
rules for better error messages and documentation but removed rules
_cannot_ be used in any context.

Removes PLR1706 as a useful test case and something we want to
accomplish in #9680 anyway. The rule was in preview so we do not need to
deprecate it first.

Closes #9007

## Test plan

<img width="1110" alt="Rules table"
src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355">

<img width="1110" alt="Rule page"
src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
zanieb added a commit that referenced this issue Jan 30, 2024
Similar to #9689 — retains removed
rules for better error messages and documentation but removed rules
_cannot_ be used in any context.

Removes PLR1706 as a useful test case and something we want to
accomplish in #9680 anyway. The rule was in preview so we do not need to
deprecate it first.

Closes #9007

## Test plan

<img width="1110" alt="Rules table"
src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355">

<img width="1110" alt="Rule page"
src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
@zanieb zanieb mentioned this issue Jan 30, 2024
zanieb added a commit that referenced this issue Feb 1, 2024
Similar to #9689 — retains removed
rules for better error messages and documentation but removed rules
_cannot_ be used in any context.

Removes PLR1706 as a useful test case and something we want to
accomplish in #9680 anyway. The rule was in preview so we do not need to
deprecate it first.

Closes #9007

## Test plan

<img width="1110" alt="Rules table"
src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355">

<img width="1110" alt="Rule page"
src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
zanieb added a commit that referenced this issue Feb 1, 2024
Similar to #9689 — retains removed
rules for better error messages and documentation but removed rules
_cannot_ be used in any context.

Removes PLR1706 as a useful test case and something we want to
accomplish in #9680 anyway. The rule was in preview so we do not need to
deprecate it first.

Closes #9007

## Test plan

<img width="1110" alt="Rules table"
src="https://github.com/astral-sh/ruff/assets/2586601/ac9fa682-623c-44aa-8e51-d8ab0d308355">

<img width="1110" alt="Rule page"
src="https://github.com/astral-sh/ruff/assets/2586601/05850b2d-7ca5-49bb-8df8-bb931bab25cd">
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

Successfully merging a pull request may close this issue.

4 participants