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

Add checker for comparing for equality to empty literal #4774

Closed
DanielNoord opened this issue Jul 29, 2021 · 8 comments · Fixed by #5120
Closed

Add checker for comparing for equality to empty literal #4774

DanielNoord opened this issue Jul 29, 2021 · 8 comments · Fixed by #5120
Assignees
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors
Milestone

Comments

@DanielNoord
Copy link
Collaborator

Current problem

Originally posted by @cdce8p in #4769 (comment)

Best not to check for equality by comparing it to an empty literal.
(This should probably be a pylint check as well.)

Desired solution

# good
test_list = []
if not test_list:
   pass

# bad
test_list = []
if test_list == []:
   pass

Additional context

No response

@DanielNoord DanielNoord added Enhancement ✨ Improvement to a component Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jul 29, 2021
@cdce8p
Copy link
Member

cdce8p commented Jul 29, 2021

I already though of a way to implement it, but couldn't yet think of a good name or description. If anyone once to take a stab at it, this here should work:

    def visit_compare(self, node: astroid.Compare) -> None:
        for op, comparator in node.ops:
            if op == "==":
                if (
                    isinstance(comparator, (astroid.List, astroid.Tuple))
                    and not comparator.elts
                    or isinstance(comparator, astroid.Dict)
                    and not comparator.items
                ):
                    self.add_message(..., node=node)

@cdce8p cdce8p added Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jul 29, 2021
@DanielNoord
Copy link
Collaborator Author

Perhaps no-empty-literal-comparison?
And what kind of message would this be? Does this have a performance effect? Or is it just code style?

@cdce8p
Copy link
Member

cdce8p commented Jul 29, 2021

Perhaps no-empty-literal-comparison?

Sounds like a start. I'm not good at naming things though 😅
@Pierre-Sassoulas Do you have a good idea?

And what kind of message would this be? Does this have a performance effect? Or is it just code style?

Maybe a small performance effect and a bit code style. So probably a refactoring checker.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Jul 29, 2021

Maybe use-implicit-booleanness or empty-literal-as-condition (based on the name of len-as-condition which is similar) ? This checker is prone to false positive because for numpy/pandas array the boolean value of an array is ambiguous. Ie you need to do not test_list.size or use all or any. Arguably test_list == [] could be considered better in that case (?). So I think we probably need to do something specifically for numpy/pandas. Something similar was done for len-as-condition where we check that the __bool__ function is not re-implemented and that the class inherit directly from a list, set, or dict (ie: not a ndarray), see #3821

@Pierre-Sassoulas
Copy link
Member

I've been thinking: we could rename len-as-condition to use-implicit-booleanness and add the comparison to empty literal to the checker.

@DanielNoord
Copy link
Collaborator Author

Probably should also implement #3031 when working on this issue.

@jaehoonhwang
Copy link
Contributor

Hi, can I try to resolve this issue?

@Pierre-Sassoulas
Copy link
Member

Hi, @jaehoonhwang , of course, I'll assign you to the issue.

jaehoonhwang added a commit to jaehoonhwang/pylint that referenced this issue Oct 5, 2021
* Create a new visitor on refactoring_checker; `visit_compare`
* Create a new checker; use-implicit-booleanness checker where it looks
  for boolean evaluatiion with collection literals such as `()`, `[]`,
  or `{}`
* Fixed invalid usage of comparison within pylint package

This closes pylint-dev#4774
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.12.0 milestone Oct 5, 2021
@cdce8p cdce8p modified the milestones: 2.13.0, 2.12.0 Oct 5, 2021
jaehoonhwang added a commit to jaehoonhwang/pylint that referenced this issue Oct 8, 2021
* Create a new visitor on refactoring_checker; `visit_compare`
* Create a new checker; use-implicit-booleanness checker where it looks
  for boolean evaluatiion with collection literals such as `()`, `[]`,
  or `{}`
* Fixed invalid usage of comparison within pylint package

This closes pylint-dev#4774
jaehoonhwang added a commit to jaehoonhwang/pylint that referenced this issue Oct 8, 2021
Rename `len-as-condition` to be more general for new checker
`use-implicit-booleaness-not-comparison`

Closes pylint-dev#4774

* Refactor `LenChecker` class -> `ImplicitBooleanessChecker`o
* Rename test files/`len_checker.py`/`__init__.py` to reflect new name.
* Add `len-as-condition` as `old_names` for `use-implicit-booleaness-not-len`
jaehoonhwang added a commit to jaehoonhwang/pylint that referenced this issue Oct 10, 2021
* Create a new visitor on refactoring_checker; `visit_compare`
* Create a new checker; use-implicit-booleanness checker where it looks
  for boolean evaluatiion with collection literals such as `()`, `[]`,
  or `{}`
* Fixed invalid usage of comparison within pylint package

This closes pylint-dev#4774
jaehoonhwang added a commit to jaehoonhwang/pylint that referenced this issue Oct 10, 2021
* Create a new visitor on refactoring_checker; `visit_compare`
* Create a new checker; use-implicit-booleanness checker where it looks
  for boolean evaluatiion with collection literals such as `()`, `[]`,
  or `{}`
* Fixed invalid usage of comparison within pylint package

This closes pylint-dev#4774
jaehoonhwang added a commit to jaehoonhwang/pylint that referenced this issue Oct 11, 2021
* Create a new visitor on refactoring_checker; `visit_compare`
* Create a new checker; use-implicit-booleanness checker where it looks
  for boolean evaluatiion with collection literals such as `()`, `[]`,
  or `{}`
* Fixed invalid usage of comparison within pylint package

This closes pylint-dev#4774
Pierre-Sassoulas added a commit that referenced this issue Oct 17, 2021
…on with collection literals (#5120)

* Create a new checker; use-implicit-booleanness checker where it looks
  for boolean evaluatiion with collection literals such as `()`, `[]`,
  or `{}`

* Fixed invalid usage of comparison within pylint package

This closes #4774

* Ignore tuples when checking for `literal-comparison`

Closes #3031

* Merge len_checker with empty_literal checker

Moving empty literal checker with len_checker to avoid class without
iterators without boolean expressions (false positive on pandas)
Reference: https://github.com/PyCQA/pylint/pull/3821/files

* Update `len_checker` and its class `LenChecker` to `ComparisonChecker`
  to reflect better usage after merging between `len_checker` and
  `empty_literal_checker` and its tests.

* Fixed `consider_using_in` and `consider_iterating_dictionary` tests
  that were failing due to new empty literal checkers.

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
stanislavlevin added a commit to stanislavlevin/fc-admin that referenced this issue Feb 17, 2022
Pylint 2.12.0 introduced new checker:
> Used when Pylint detects that collection literal comparison is being
  used to check for emptiness; Use implicit booleaness insteadof a
  collection classes; empty collections are considered as false

See pylint-dev/pylint#4774

Fixes: fleet-commander#279
Signed-off-by: Stanislav Levin <slev@altlinux.org>
stanislavlevin added a commit to stanislavlevin/fc-admin that referenced this issue Feb 17, 2022
Pylint 2.12.0 introduced new checker:
> Used when Pylint detects that collection literal comparison is being
  used to check for emptiness; Use implicit booleaness insteadof a
  collection classes; empty collections are considered as false

See pylint-dev/pylint#4774

Fixes: fleet-commander#279
Signed-off-by: Stanislav Levin <slev@altlinux.org>
olivergs pushed a commit to fleet-commander/fc-admin that referenced this issue Mar 3, 2022
Pylint 2.12.0 introduced new checker:
> Used when Pylint detects that collection literal comparison is being
  used to check for emptiness; Use implicit booleaness insteadof a
  collection classes; empty collections are considered as false

See pylint-dev/pylint#4774

Fixes: #279
Signed-off-by: Stanislav Levin <slev@altlinux.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Good first issue Friendly and approachable by new contributors Hacktoberfest Help wanted 🙏 Outside help would be appreciated, good for new contributors
Projects
None yet
4 participants