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

Fix false positives for use-implicit-booleaness-not-comparison checks #10246

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zenlyj
Copy link
Contributor

@zenlyj zenlyj commented Feb 24, 2025

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Currently, the use-implicit-booleaness-not-comparison check is applied to expressions with chained comparison. The emitted message and suggested code correction seem inaccurate, and I can't seem to find existing functional tests indicating that this behavior is intended. Therefore, these changes will skip the check when chained comparison nodes are visited.

Take for example:

foo, bar = [1], []

assert foo != bar == []  # "foo == []" can be simplified to "not foo", if it is strictly a sequence, as an empty list is falsey

foo != bar == [] is equivalent to foo != bar and bar == [], which is not the same as not foo.

Similar for use-implicit-booleaness-not-comparison-to-zero and use-implicit-booleaness-not-comparison-to-string.

Closes #10065

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.86%. Comparing base (d17bb06) to head (187e34f).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10246   +/-   ##
=======================================
  Coverage   95.85%   95.86%           
=======================================
  Files         175      175           
  Lines       19056    19058    +2     
=======================================
+ Hits        18267    18269    +2     
  Misses        789      789           
Files with missing lines Coverage Ξ”
...heckers/refactoring/implicit_booleaness_checker.py 100.00% <100.00%> (ΓΈ)

This comment has been minimized.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a changelog? Rest of the changes LGTM!

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/3.3.x labels Feb 24, 2025
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.3.5 milestone Feb 24, 2025
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks great, the primer looks amazing (I realized that we had a bug in the message for some chained comparisons they were not displayed in full but now it's gone, so.. good)

Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on django:
The following messages are no longer emitted:

  1. use-implicit-booleaness-not-comparison-to-string:
    "y == m == d == ''" can be simplified to "not d", if it is strictly a string, as an empty string is falsey
    https://github.com/django/django/blob/240421c7c4c81fe5df26274b807266bd4ca73d7f/django/forms/widgets.py#L1256

Effect on pandas:
The following messages are no longer emitted:

  1. use-implicit-booleaness-not-comparison-to-zero:
    "0 != lplane_indexer" can be simplified to "lplane_indexer", if it is strictly an int, as 0 is falsey
    https://github.com/pandas-dev/pandas/blob/6bd74fab5f2dfe3512ecbd2797808399411bd452/pandas/core/indexing.py#L1984
  2. use-implicit-booleaness-not-comparison-to-zero:
    "nobs == 0" can be simplified to "not nobs", if it is strictly an int, as 0 is falsey
    https://github.com/pandas-dev/pandas/blob/6bd74fab5f2dfe3512ecbd2797808399411bd452/pandas/core/_numba/kernels/sum_.py#L139
  3. use-implicit-booleaness-not-comparison-to-zero:
    "0 == min_periods" can be simplified to "not min_periods", if it is strictly an int, as 0 is falsey
    https://github.com/pandas-dev/pandas/blob/6bd74fab5f2dfe3512ecbd2797808399411bd452/pandas/core/_numba/kernels/sum_.py#L139
  4. use-implicit-booleaness-not-comparison-to-zero:
    "values.shape[0] == 0" can be simplified to "not values.shape[0]", if it is strictly an int, as 0 is falsey
    https://github.com/pandas-dev/pandas/blob/6bd74fab5f2dfe3512ecbd2797808399411bd452/pandas/core/internals/construction.py#L341

Effect on sentry:
The following messages are no longer emitted:

  1. use-implicit-booleaness-not-comparison-to-zero:
    "p75 == 0" can be simplified to "not p75", if it is strictly an int, as 0 is falsey
    https://github.com/getsentry/sentry/blob/5dd543a208decc6fb5000a6d1c75a92ad90107c8/src/sentry/relay/config/metric_extraction.py#L1448
  2. use-implicit-booleaness-not-comparison-to-zero:
    "duration % 3600 == 0" can be simplified to "not duration % 3600", if it is strictly an int, as 0 is falsey
    https://github.com/getsentry/sentry/blob/5dd543a208decc6fb5000a6d1c75a92ad90107c8/src/sentry/search/events/builder/utils.py#L58
  3. use-implicit-booleaness-not-comparison-to-zero:
    "duration % 86400 == 0" can be simplified to "not duration % 86400", if it is strictly an int, as 0 is falsey
    https://github.com/getsentry/sentry/blob/5dd543a208decc6fb5000a6d1c75a92ad90107c8/src/sentry/search/events/builder/utils.py#L66

This comment was generated for commit 187e34f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport maintenance/3.3.x False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use-implicit-booleaness-not-comparison should not be offered in N-way comparisons
3 participants