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

E712 fix results in unsafe behavior #9063

Closed
DavidSlayback opened this issue Dec 8, 2023 · 5 comments
Closed

E712 fix results in unsafe behavior #9063

DavidSlayback opened this issue Dec 8, 2023 · 5 comments

Comments

@DavidSlayback
Copy link

If Ruff is allowed to check for and fix E712 errors, this can result in unexpected behavior with things like pandas DataFrames. Example:

mask = (df["bool_col"] == True) & (df["other_col"].isna())
# If we add noqa, we're ok
mask = (df["bool_col"] == True) & (df["other_col"].isna())  & (df["other_col"].isna()) # noqa: E712
# But if we let ruff fix as part of a pre-commit...
# ruff check . --fix
mask = (df["bool_col"] is True) & (df["other_col"].isna()) 

The original behavior produces a mask for each operand and combines them. The "fix" makes the first operand always return False, which means the whole mask is False.

Ruff version: v0.1.6

I think the desired behavior would be to either consider the fix unsafe or to favor removing == True and != True, but the latter might be difficult (because the correct equivalent to != in Pandas is ~df, not !df or not df

@zanieb
Copy link
Member

zanieb commented Dec 9, 2023

Please see #4560

This fix should already be marked as unsafe and not applied unless you've opted into unsafe fixes e.g.

❯ ruff check example.py --isolated --no-cache --select E712 --fix
example.py:1:27: E712 Comparison to `True` should be `cond is True` or `if cond:`
Found 1 error.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
❯ ruff check example.py --isolated --no-cache --select E712 --fix --unsafe-fixes
Found 1 error (1 fixed, 0 remaining).

@zanieb zanieb closed this as completed Dec 9, 2023
@DavidSlayback
Copy link
Author

Hmm...sorry about that, I'll have to figure out how the auto fix got triggered, our pre-commit doesn't specify --unsafe-fixes

@charliermarsh
Copy link
Member

Hmm, could you have unsafe-fixes = true set in your pyproject.toml or ruff.toml file?

@DavidSlayback
Copy link
Author

DavidSlayback commented Dec 11, 2023

No ruff.toml, pyproject.toml doesn't specify anything about the fixes, and .pre-commit-config.yaml section is:

  - repo: https://github.com/astral-sh/ruff-pre-commit
    # Ruff version.
    rev: v0.1.6
    hooks:
      - id: ruff
        name: "Lint and fix code using Ruff"
        args: [ --fix, --exit-non-zero-on-fix ]
        stages: [commit]

Going to assume maybe someone triggered it manually to get it to stop complaining

zanieb added a commit that referenced this issue Dec 11, 2023
Hides hints about unsafe fixes when they are disabled e.g. with
`--no-unsafe-fixes` or `unsafe-fixes = false`. By default, unsafe fix
hints are still displayed. This seems like a nice way to remove the nag
for users who have chosen not to apply unsafe fixes.

Inspired by comment at
#9063 (comment)
@zanieb
Copy link
Member

zanieb commented Dec 11, 2023

@DavidSlayback #9095 will be in our next release so you can disable unsafe fixes entirely to hide that complaint :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants