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

SIM401 triggers on dict like objects that aren't proper dictionaries #15814

Open
jogo-openai opened this issue Jan 29, 2025 · 2 comments
Open
Labels
needs-decision Awaiting a decision from a maintainer type-inference Requires more advanced type inference.

Comments

@jogo-openai
Copy link

Description

$ ruff --version
ruff 0.9.3
class Foo:
    def __init__(self):
        self._dict = {}

    def __getitem__(self, key):
        return self._dict[key]

    def __setitem__(self, key, value):
        self._dict[key] = value

    def __iter__(self):
        return self._dict.__iter__()



foo = Foo()
foo['key'] = 'value'


if "key" in foo:
    value = foo["key"]
else:
    value = None

print(value)


# print(foo.get('key', None))
# Triggers `AttributeError: 'Foo' object has no attribute 'get'`

ruff check --fix --unsafe-fixes --diff --select=SIM401 foo.py

--- foo.py
+++ foo.py
@@ -17,10 +17,7 @@
 foo['key'] = 'value'


-if "key" in foo:
-    value = foo["key"]
-else:
-    value = None
+value = foo.get('key', None)

 print(value)


Would fix 1 error.
@dylwil3 dylwil3 added the type-inference Requires more advanced type inference. label Jan 29, 2025
@dylwil3
Copy link
Collaborator

dylwil3 commented Jan 29, 2025

I'm slightly in favor of just updating the documentation (which is part of #15584) for this unsafe fix. An alternative is to restrict to instances where our limited type inference can tell that we have an honest dict object, but that would be a significant restriction in applicability. That said, false positives are more annoying than false negatives for this sort of rule, so I could be convinced of the second route.

@dylwil3 dylwil3 added the needs-decision Awaiting a decision from a maintainer label Jan 29, 2025
@jogo-openai
Copy link
Author

I have a slight preference for removing false positives and making sure things are actually dicts. That being said, just updating the documentation for this unsafe fix also seems good to me (I suspect this means llarge code bases would opt to not use this rule).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer type-inference Requires more advanced type inference.
Projects
None yet
Development

No branches or pull requests

2 participants