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

Incorrect suggestion to fix string comparison #378

Closed
alphatownsman opened this issue Aug 10, 2023 · 2 comments · Fixed by #391
Closed

Incorrect suggestion to fix string comparison #378

alphatownsman opened this issue Aug 10, 2023 · 2 comments · Fixed by #391
Labels
bug Something isn't working

Comments

@alphatownsman
Copy link

alphatownsman commented Aug 10, 2023

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
Copy link
Member

Seems related to #375

@amyreese amyreese added the bug Something isn't working label Aug 22, 2023
@llllvvuu
Copy link
Contributor

llllvvuu commented Sep 7, 2023

EDIT: Fix submitted: #391

This is due to Instagram/LibCST#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.

llllvvuu added a commit to llllvvuu/Fixit that referenced this issue Sep 7, 2023
…d of QualifiedName

fixes Instagram#378 and Instagram#375 which are caused by Instagram/LibCST#389.

Qualifying a name will never make some thing that wasn't already
True/False/None into a True/False/None:

```python
import libcst as cst
from libcst.metadata.name_provider import QualifiedNameProvider
from textwrap import dedent

wrapper = cst.MetadataWrapper(
    cst.parse_module(dedent(
    '''
        x = None
        x()
    '''
    ))
)
x_ref = wrapper.module.body[1].body[0].value
print(wrapper.resolve(QualifiedNameProvider)[x_ref]())
```
prints:
```python
{QualifiedName(name='x', source=<QualifiedNameSource.LOCAL: 3>)}
```
amyreese added a commit to llllvvuu/Fixit that referenced this issue Oct 17, 2023
…d of QualifiedName

fixes Instagram#378 and Instagram#375 which are caused by Instagram/LibCST#389.

Qualifying a name will never make some thing that wasn't already
True/False/None into a True/False/None:

```python
import libcst as cst
from libcst.metadata.name_provider import QualifiedNameProvider
from textwrap import dedent

wrapper = cst.MetadataWrapper(
    cst.parse_module(dedent(
    '''
        x = None
        x()
    '''
    ))
)
x_ref = wrapper.module.body[1].body[0].value
print(wrapper.resolve(QualifiedNameProvider)[x_ref]())
```
prints:
```python
{QualifiedName(name='x', source=<QualifiedNameSource.LOCAL: 3>)}
```

Co-authored-by: Amethyst Reese <[email protected]>
amyreese added a commit that referenced this issue Oct 17, 2023
…d of QualifiedName (#391)

fixes #378 and #375 which are caused by Instagram/LibCST#389.

Qualifying a name will never make some thing that wasn't already
True/False/None into a True/False/None:

```python
import libcst as cst
from libcst.metadata.name_provider import QualifiedNameProvider
from textwrap import dedent

wrapper = cst.MetadataWrapper(
    cst.parse_module(dedent(
    '''
        x = None
        x()
    '''
    ))
)
x_ref = wrapper.module.body[1].body[0].value
print(wrapper.resolve(QualifiedNameProvider)[x_ref]())
```
prints:
```python
{QualifiedName(name='x', source=<QualifiedNameSource.LOCAL: 3>)}
```

Co-authored-by: Amethyst Reese <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants