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

[refurb] Add coverage for using set(...) in single-item-membership-test (FURB171) #15793

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

naslundx
Copy link
Contributor

Summary

Adds coverage of using set(...) in addition to `{...} in SingleItemMembershipTest.

Fixes #15792

Test Plan

Updated unit test and snapshot.
Steps to reproduce are in the issue linked above.

@naslundx naslundx force-pushed the single-item-membership-test-missing-set-constructor branch from 77168c6 to 781af24 Compare January 28, 2025 20:17
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+5 -0 violations, +0 -0 fixes in 4 projects; 51 projects unchanged)

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

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

+ airflow/api_fastapi/execution_api/datamodels/taskinstance.py:165:10: FURB171 [*] Membership test against single-item container
+ dev/breeze/src/airflow_breeze/utils/exclude_from_matrix.py:46:44: FURB171 [*] Membership test against single-item container

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

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

+ src/bokeh/settings.py:265:12: FURB171 [*] Membership test against single-item container

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

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

+ rotkehlchen/exchanges/bitfinex.py:638:24: FURB171 [*] Membership test against single-item container

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

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

+ astropy/units/tests/test_equivalencies.py:787:16: FURB171 [*] Membership test against single-item container

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB171 5 5 0 0 0

@naslundx
Copy link
Contributor Author

naslundx commented Jan 28, 2025

Ah, I see, it doesn't work if the single argument to set() is an enum, for example. :(

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! However, I think it would be incorrect for this rule to trigger on set(1), since that doesn't create a set with a single member at runtime. It raises an exception.

Comment on lines +12 to +13
if 1 in set(1):
print("Single-element set")
Copy link
Member

Choose a reason for hiding this comment

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

hmm, this fails at runtime:

>>> set(1)
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    set(1)
    ~~~^^^
TypeError: 'int' object is not iterable

Comment on lines +35 to +36
if 1 in set(1, 2):
pass
Copy link
Member

Choose a reason for hiding this comment

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

this also fails at runtime:

>>> set(1, 2)
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    set(1, 2)
    ~~~^^^^^^
TypeError: set expected at most 1 argument, got 2

@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 29, 2025

To clarify, @naslundx, all of these succeed at runtime, and I'd be okay with adding support for these to FURB171!

set([1])
set((1,))
set({1})

You'd need to check that the object being passed to the set() call is an Expr::List, Expr::Tuple or Expr::Set that has exactly one member in its elts field.

@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Jan 29, 2025

You'd need to check that the object [...] has exactly one member in its elts field.

Additionally, that element should be literal/hashable, so that errors like this one are not masked:

if a in set([{}]): ...  # TypeError: unhashable type: 'dict'
if a == {}: ...         # No errors

@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 30, 2025

You'd need to check that the object [...] has exactly one member in its elts field.

Additionally, that element should be literal/hashable, so that errors like this one are not masked:

if a in set([{}]): ...  # TypeError: unhashable type: 'dict'
if a == {}: ...         # No errors

I think we should open a separate issue about this because that behavior is already present in the current scope of the rule:

1 in {set()} # TypeError: unhashable type: 'set'
1 == set() # No errors

@dylwil3 dylwil3 added rule Implementing or modifying a lint rule preview Related to preview mode features labels Jan 30, 2025
@dylwil3 dylwil3 changed the title (FURB171) Add coverage of using set(...) in SingleItemMembershipTest [refurb] Add coverage for using set(...) in single-item-membership-test (FURB171) Jan 30, 2025
@tdulcet
Copy link

tdulcet commented Jan 30, 2025

Maybe check for frozenset as well:

frozenset([1])
frozenset((1,))
frozenset({1})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FURB171 Does not trigger when calling set(...)
5 participants