Instagram / Fixit

Advanced Python linting framework with auto-fixes and hierarchical configuration that makes it easy to write custom in-repo lint rules.
https://fixit.rtfd.io/en/latest/
Other
666 stars 62 forks source link

Incorrect suggestion to fix string comparison #378

Closed alphatownsman closed 10 months ago

alphatownsman commented 1 year ago

It seems this autofix is changing the logic for no good reason?

users/account.py@370:15 CompareSingletonPrimitivesByIs: Comparisons to singleton primitives should not be done with == or !=, as they check equality rather than identiy. Use `is` or `is not` instead. (has autofix)
--- a/account.py
+++ b/account.py
@@ -369,3 +369,3 @@
         if form.cleaned_data["email"]:
-            if form.cleaned_data["email"].lower() != (request.user.email or "").lower():
+            if form.cleaned_data["email"].lower() is not (request.user.email or "").lower():
                 if User.objects.filter(
fixit -V
fixit, version 2.0.0.post1
amyreese commented 1 year ago

Seems related to #375

llllvvuu commented 1 year ago

EDIT: Fix submitted: #391

This is due to https://github.com/Instagram/LibCST/issues/389 .

Simplified repro:

    VALID = [
        ...
        Valid('"True" == "True"'),  # passes
        Valid('"True" != "False".lower()'),  # fails
        Valid('lower = lambda x: x.lower()\n"True" != lower("False")'),  # passes
        Valid('"True" != lower("False")'),  # passes
    ]
dap> right_comp
...
  func: Attribute(\n    value=SimpleString(\n        value='"False"',\n        lpar=[],\n        rpar=[],\n    ),\n    attr=Name(\n        value='lower',\n        lpar=[]
...
dap> self.metadata[QualifiedNameProvider][right_comp]()
{QualifiedName(name='...ILTIN: 2>)}
  4354005056: QualifiedName(name='builtins.None', source=<QualifiedNameSource.BUILTIN: 2>)
  function variables:
  len(): 1
  special variables:
dap>

I think it should either return {String.lower} or {}, not {None}?

That being said, I think the lint issue could be fixed by just not using QualifiedNameProvider. You can just look at the node directly.