[`flake8-simplify`] Detect implicit `else` cases in `needless-bool` (`SIM103`) #10414

ottaviohartman · 2024-03-15T00:11:30Z

Summary

For SIM103, detect and simplify the following case:

def main():
    if foo > 5:
        return True
    return False

Test Plan

Unit tested only.

.../flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap

github-actions · 2024-03-15T00:30:16Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+156 -0 violations, +0 -0 fixes in 10 projects; 33 projects unchanged)

apache/airflow (+32 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ airflow/configuration.py:1313:13: SIM103 Return the condition `value is None` directly
+ airflow/dag_processing/manager.py:1247:9: SIM103 Return the condition `self._num_run < self._max_runs` directly
+ airflow/models/operator.py:45:5: SIM103 Return the condition `task.get_closest_mapped_task_group() is not None` directly
+ airflow/models/taskinstance.py:372:5: SIM103 Return the condition `isinstance(value, (bytearray, bytes, str))` directly
+ airflow/operators/python.py:72:5: SIM103 Return the condition directly
+ airflow/providers/airbyte/triggers/airbyte.py:119:9: SIM103 Return the condition directly
+ airflow/providers/amazon/aws/hooks/s3.py:648:13: SIM103 Return the condition directly
+ airflow/providers/amazon/aws/sensors/athena.py:97:9: SIM103 Return the condition `state in self.INTERMEDIATE_STATES` directly
+ airflow/providers/amazon/aws/sensors/emr.py:331:9: SIM103 Return the condition `state in self.INTERMEDIATE_STATES` directly
+ airflow/providers/dbt/cloud/triggers/dbt.py:113:9: SIM103 Return the condition directly
... 22 additional changes omitted for project

bokeh/bokeh (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ examples/server/app/server_auth/auth.py:40:9: SIM103 Return the condition `username == "bokeh" and password == "bokeh"` directly

demisto/content (+46 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:763:9: SIM103 Return the condition `RDL is None` directly
+ Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:778:9: SIM103 Return the condition `not self._valid(self.rcs)` directly
+ Packs/Active_Directory_Query/Integrations/Active_Directory_Query/Active_Directory_Query.py:347:5: SIM103 Return the condition `entries.get('flat')` directly
+ Packs/Base/Scripts/DBotTrainClustering/DBotTrainClustering.py:540:5: SIM103 Return the condition `not 1 < n_labels < n_samples` directly
+ Packs/BreachRx/Integrations/BreachRx/BreachRx_test.py:28:5: SIM103 Return the condition directly
+ Packs/BreachRx/Integrations/BreachRx/BreachRx_test.py:34:5: SIM103 Return the condition directly
+ Packs/BreachRx/Integrations/BreachRx/BreachRx_test.py:40:5: SIM103 Return the condition directly
+ Packs/BreachRx/Integrations/BreachRx/BreachRx_test.py:46:5: SIM103 Return the condition directly
+ Packs/BreachRx/Integrations/BreachRx/BreachRx_test.py:52:5: SIM103 Return the condition directly
+ Packs/BreachRx/Integrations/BreachRx/BreachRx_test.py:58:5: SIM103 Return the condition directly
+ Packs/CheckPhish/Integrations/CheckPhish/CheckPhish.py:144:5: SIM103 Return the condition `res and res['status'] == DONE_STATUS` directly
+ Packs/CommonScripts/Scripts/DisableUserWrapper/DisableUserWrapper_test.py:10:5: SIM103 Return the condition `not command1.args_lst == command2.args_lst` directly
+ Packs/CommonScripts/Scripts/DomainReputation/DomainReputation.py:22:5: SIM103 Return the condition `item['Contents'] and 'Offset' in item['Contents']` directly
+ Packs/CommonScripts/Scripts/FileReputation/FileReputation.py:22:5: SIM103 Return the condition `item['Contents'] and 'Offset' in item['Contents']` directly
... 32 additional changes omitted for project

freedomofpress/securedrop (+8 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ admin/bootstrap.py:120:9: SIM103 Return the condition directly
+ journalist_gui/journalist_gui/SecureDropUpdater.py:23:5: SIM103 Return the condition `pwd_flag == "NP"` directly
+ securedrop/models.py:257:9: SIM103 Return the condition directly
+ securedrop/pretty_bad_protocol/_parsers.py:1008:9: SIM103 Return the condition `self.fingerprint` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1321:9: SIM103 Return the condition `len(self.fingerprints) == 0` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1425:9: SIM103 Return the condition `len(self.fingerprints) == 0` directly
+ securedrop/pretty_bad_protocol/_parsers.py:1788:9: SIM103 Return the condition `self.ok` directly
+ securedrop/pretty_bad_protocol/_parsers.py:234:5: SIM103 Return the condition `HEXADECIMAL.match(string)` directly

milvus-io/pymilvus (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pymilvus/client/check.py:166:5: SIM103 Return the condition `(end_date - start_date).days < 0` directly
+ pymilvus/client/check.py:20:5: SIM103 Return the condition `not is_legal_host(a[0]) or not is_legal_port(a[1])` directly
+ pymilvus/client/check.py:27:5: SIM103 Return the condition directly
+ pymilvus/orm/collection.py:1438:9: SIM103 Return the condition directly
+ pymilvus/orm/iterator.py:458:9: SIM103 Return the condition `cached_page is None or len(cached_page) < count` directly

mlflow/mlflow (+11 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ mlflow/sklearn/utils.py:928:9: SIM103 Return the condition `not len(c.__abstractmethods__)` directly
+ mlflow/tracking/_tracking_service/utils.py:25:5: SIM103 Return the condition `_tracking_uri or MLFLOW_TRACKING_URI.get()` directly
+ mlflow/transformers/__init__.py:1418:5: SIM103 Return the condition directly
+ mlflow/utils/autologging_utils/__init__.py:254:9: SIM103 Return the condition directly
+ mlflow/utils/logging_utils.py:101:9: SIM103 Return the condition directly
+ mlflow/utils/search_utils.py:1199:9: SIM103 Return the condition directly
+ mlflow/utils/search_utils.py:1396:9: SIM103 Return the condition directly
+ mlflow/utils/search_utils.py:439:9: SIM103 Return the condition directly
+ mlflow/utils/search_utils.py:872:9: SIM103 Return the condition directly
+ mlflow/utils/uri.py:65:5: SIM103 Return the condition directly
... 1 additional changes omitted for project

pypa/cibuildwheel (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ cibuildwheel/projectfiles.py:42:5: SIM103 Return the condition `len(consts) != 1` directly

rotki/rotki (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ rotkehlchen/chain/bitcoin/bch/utils.py:63:5: SIM103 Return the condition `_polymod(_prefix_expand(prefix) + decoded) != 0` directly
+ rotkehlchen/chain/ethereum/defi/zerionsdk.py:166:5: SIM103 Return the condition directly
+ rotkehlchen/premium/sync.py:161:9: SIM103 Return the condition `diff < 3600 and not force_upload` directly
+ rotkehlchen/tests/fixtures/blockchain.py:260:5: SIM103 Return the condition `should_mock_web3` directly
+ rotkehlchen/utils/misc.py:404:5: SIM103 Return the condition `version.dev is None` directly

scikit-build/scikit-build (+2 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ skbuild/setuptools_wrap.py:323:5: SIM103 Return the condition directly
+ skbuild/setuptools_wrap.py:348:5: SIM103 Return the condition `"sdist" in given_commands and cmake_with_sdist` directly

zulip/zulip (+45 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ corporate/lib/stripe.py:1027:9: SIM103 Return the condition directly
+ corporate/lib/stripe.py:3719:9: SIM103 Return the condition `plan_tier in implemented_plan_tiers` directly
+ corporate/lib/stripe.py:4125:9: SIM103 Return the condition `plan_tier in implemented_plan_tiers` directly
+ corporate/lib/stripe.py:4569:9: SIM103 Return the condition `plan_tier in implemented_plan_tiers` directly
+ scripts/lib/zulip_tools.py:150:5: SIM103 Return the condition directly
+ scripts/lib/zulip_tools.py:549:5: SIM103 Return the condition `"posix" in os.name and os.geteuid() == 0` directly
+ zerver/actions/user_groups.py:132:9: SIM103 Return the condition `diff < realm.waiting_period_threshold` directly
+ zerver/decorator.py:1025:9: SIM103 Return the condition `not user_has_device(user)` directly
+ zerver/lib/avatar.py:144:5: SIM103 Return the condition directly
+ zerver/lib/compatibility.py:157:5: SIM103 Return the condition directly
+ zerver/lib/compatibility.py:40:5: SIM103 Return the condition `timezone_now() > deadline` directly
+ zerver/lib/markdown/__init__.py:2057:9: SIM103 Return the condition directly
+ zerver/lib/markdown/__init__.py:2062:9: SIM103 Return the condition directly
+ zerver/lib/message.py:1381:5: SIM103 Return the condition directly
... 31 additional changes omitted for project

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
SIM103	156	156	0	0	0

charliermarsh · 2024-03-15T00:33:04Z

Thanks! Do you mind moving this to preview-only?

Is there a preferred way to get the next statement or expression...

Yes, the best prior art here would be... convert_for_loop_to_any_all. It will look like:

let parent = checker.semantic().current_statement_parent()?;
let suite = traversal::suite(stmt, parent)?;
let sibling = traversal::next_sibling(stmt, suite)?;

So, ideally, we check if the statement is an if that returns a boolean constant, and then grab the next statement if so.

ottaviohartman · 2024-03-15T00:49:55Z

Thanks! Do you mind moving this to preview-only?

Yep! Is there a way to partially move SIM103 to preview? Or shall I create a new rule? (if so, SIM???)

        (Flake8Simplify, "103") => (RuleGroup::Stable, rules::flake8_simplify::rules::NeedlessBool),

Is there a preferred way to get the next statement or expression...

Yes, the best prior art here would be... convert_for_loop_to_any_all. It will look like:
let parent = checker.semantic().current_statement_parent()?;
let suite = traversal::suite(stmt, parent)?;
let sibling = traversal::next_sibling(stmt, suite)?;
So, ideally, we check if the statement is an if that returns a boolean constant, and then grab the next statement if so.

Ah, I'll refactor to use that instead!

charliermarsh · 2024-03-15T00:50:44Z

Yep! Is there a way to partially move SIM103 to preview?

So typically we'd just add a check for the new codepath on checker.settings.preview.is_enabled().

codspeed-hq · 2024-03-15T01:52:23Z

CodSpeed Performance Report

Merging #10414 will not alter performance

_{Comparing ottaviohartman:thartman/sim103-detect-implicit-else (7ecc53d) with main (8619986)}

Summary

✅ 30 untouched benchmarks

ottaviohartman · 2024-03-15T01:55:36Z

Ready for another review @charliermarsh . Thanks for the help!

Used (much simpler) sibling logic.
Added if preview to guard this new change.
Created preview insta snap.

ottaviohartman · 2024-03-15T02:07:41Z

Looks like my clone() is slowing down perf tests. I'll look into that.

crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs

charliermarsh

Thanks!

charliermarsh · 2024-03-18T00:10:01Z

crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs

+            (
+                if_test.as_ref(),
+                if_body,
+                std::slice::from_ref(next_stmt),


@ottaviohartman -- I was able to remove the vector allocation here by making every branch return &[Stmt] instead of &Vec<Stmt>. Then, you can do a nice trick whereby if you have an &T, you can create &[T] via std::slice::from_ref.

Awesome - I'll use that in the future.

…dless-bool` (`SIM103`) (#12048) ## Summary See: #10414. This is a good and intuitive change; we just put it in preview because it expanded scope a bit.

ottaviohartman added 8 commits March 14, 2024 18:14

this is certainly some code

f1c3626

broke a test

f663b85

fixed test mostly

cf62508

is that right?

683be70

working

29b4c84

more tests

9755bb2

some cleanup

5f7a51a

rm

c66caca

ottaviohartman commented Mar 15, 2024

View reviewed changes

.../flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap Outdated Show resolved Hide resolved

comment and small change

db10fe7

ottaviohartman added 3 commits March 14, 2024 21:41

working

f92dad8

move snap test

86e6af3

clean up diff

915371f

add preview snap

66f6ee4

charliermarsh reviewed Mar 15, 2024

View reviewed changes

crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs Outdated Show resolved Hide resolved

ottaviohartman added 5 commits March 14, 2024 22:26

get rid of clone()

93dbba3

forward declare vec option

13a73c1

cleanup

482aa69

clippy

b85a8db

small change to reduce blocks

03170c1

ottaviohartman requested a review from charliermarsh March 15, 2024 12:45

charliermarsh added 2 commits March 17, 2024 19:57

Merge branch 'main' into thartman/sim103-detect-implicit-else

1b7d160

Defer fetch

7ecc53d

charliermarsh approved these changes Mar 18, 2024

View reviewed changes

charliermarsh reviewed Mar 18, 2024

View reviewed changes

charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Mar 18, 2024

charliermarsh changed the title ~~SIM103 detect implicit else~~ [flake8-simplify] Detect implicit else cases in needless-bool (SIM103) Mar 18, 2024

charliermarsh enabled auto-merge (squash) March 18, 2024 00:10

charliermarsh merged commit 526abeb into astral-sh:main Mar 18, 2024
17 checks passed

charliermarsh mentioned this pull request Jun 26, 2024

[flake8-simplify] Stabilize implicit-else simplifications in needless-bool (SIM103) #12048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`flake8-simplify`] Detect implicit `else` cases in `needless-bool` (`SIM103`) #10414

[`flake8-simplify`] Detect implicit `else` cases in `needless-bool` (`SIM103`) #10414

ottaviohartman commented Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 15, 2024 •

edited

Loading

charliermarsh commented Mar 15, 2024

ottaviohartman commented Mar 15, 2024

charliermarsh commented Mar 15, 2024

codspeed-hq bot commented Mar 15, 2024 •

edited

Loading

ottaviohartman commented Mar 15, 2024 •

edited

Loading

ottaviohartman commented Mar 15, 2024

charliermarsh left a comment

charliermarsh Mar 18, 2024

ottaviohartman Mar 18, 2024

[flake8-simplify] Detect implicit else cases in needless-bool (SIM103) #10414

[flake8-simplify] Detect implicit else cases in needless-bool (SIM103) #10414

Conversation

ottaviohartman commented Mar 15, 2024 • edited Loading

Summary

Test Plan

github-actions bot commented Mar 15, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

charliermarsh commented Mar 15, 2024

ottaviohartman commented Mar 15, 2024

charliermarsh commented Mar 15, 2024

codspeed-hq bot commented Mar 15, 2024 • edited Loading

CodSpeed Performance Report

Merging #10414 will not alter performance

Summary

ottaviohartman commented Mar 15, 2024 • edited Loading

ottaviohartman commented Mar 15, 2024

charliermarsh left a comment

Choose a reason for hiding this comment

charliermarsh Mar 18, 2024

Choose a reason for hiding this comment

ottaviohartman Mar 18, 2024

Choose a reason for hiding this comment

[`flake8-simplify`] Detect implicit `else` cases in `needless-bool` (`SIM103`) #10414

[`flake8-simplify`] Detect implicit `else` cases in `needless-bool` (`SIM103`) #10414

ottaviohartman commented Mar 15, 2024 •

edited

Loading

github-actions bot commented Mar 15, 2024 •

edited

Loading

`ruff-ecosystem` results

codspeed-hq bot commented Mar 15, 2024 •

edited

Loading

ottaviohartman commented Mar 15, 2024 •

edited

Loading