Skip to content

Fix crash on properties & inherited methods in implicit-booleaness #5652

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

Merged
merged 4 commits into from
Jan 13, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ Release date: TBA

Closes #5483

* Fixed crash on properties and inherited class methods when comparing them for
equality against an empty dict.

Closes #5646

* Fixed a false positive for ``assigning-non-slot`` when the slotted class
defined ``__setattr__``.

Expand Down
5 changes: 5 additions & 0 deletions doc/whatsnew/2.13.rst
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ Other Changes
* When run in parallel mode ``pylint`` now pickles the data passed to subprocesses with
the ``dill`` package. The ``dill`` package has therefore been added as a dependency.

* Fixed crash on properties and inherited class methods when comparing them for
equality against an empty dict.

Closes #5646

* By default, pylint does no longer take files starting with ``.#`` into account. Those are
considered `emacs file locks`_. This behavior can be reverted by redefining the
``ignore-patterns`` option.
Expand Down
28 changes: 18 additions & 10 deletions pylint/checkers/refactoring/implicit_booleaness_checker.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
# For details: https://github.com/PyCQA/pylint/blob/main/LICENSE
from typing import List
from typing import List, Union

import astroid
from astroid import nodes
from astroid import bases, nodes

from pylint import checkers, interfaces
from pylint.checkers import utils
Expand Down Expand Up @@ -107,7 +107,7 @@ def visit_call(self, node: nodes.Call) -> None:
except astroid.InferenceError:
# Probably undefined-variable, abort check
return
mother_classes = self.base_classes_of_node(instance)
mother_classes = self.base_classes_and_return_names(instance)
affected_by_pep8 = any(
t in mother_classes for t in ("str", "tuple", "list", "set")
)
Expand Down Expand Up @@ -165,7 +165,7 @@ def _check_use_implicit_booleaness_not_comparison(
target_instance = utils.safe_infer(target_node)
if target_instance is None:
continue
mother_classes = self.base_classes_of_node(target_instance)
mother_classes = self.base_classes_and_return_names(target_instance)
is_base_comprehension_type = any(
t in mother_classes for t in ("tuple", "list", "dict", "set")
)
Expand Down Expand Up @@ -209,9 +209,17 @@ def _check_use_implicit_booleaness_not_comparison(
)

@staticmethod
def base_classes_of_node(instance: nodes.ClassDef) -> List[str]:
"""Return all the classes names that a ClassDef inherit from including 'object'."""
try:
return [instance.name] + [x.name for x in instance.ancestors()]
except TypeError:
return [instance.name]
def base_classes_and_return_names(
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should have two functions, one for unbound method (base_classes_of_returned_instance) and one for instances (base_classes_of_instance). This new function is handling two cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure.. How would we then call the function that calls base_classes_of_returned_instance and base_classes_of_instance?

Copy link
Member

Choose a reason for hiding this comment

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

Do we need one ? It seems the code before was not handling returned instances ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's what the crash is about. We don't only get Instances here but also UnboundMethod whenever one of the compared values is a property.

This is basically three changes in one: fix method typing to actually reflect that is handling Instances + update method to also handle UnboundMethod + update method to also handle Uninsurable (and everything else that is not Instance or UnboundMethod. Perhaps I should have split this, but felt that this made more sense.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I understand now. Maybe the original name, "base class of node" is generic enough (and future proof if we encounter exotic codebase that will have other nodes) ?

Copy link
Collaborator Author

@DanielNoord DanielNoord Jan 13, 2022

Choose a reason for hiding this comment

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

Renamed to base_names_of_base_node, let me know what you think!

Copy link
Member

Choose a reason for hiding this comment

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

Neat !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comment was outdated when you did your review. Are you also okay with the new new name?

Copy link
Member

Choose a reason for hiding this comment

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

Yes :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then I'll merge 🎉

node: Union[bases.UnboundMethod, bases.Instance]
) -> List[str]:
"""Return all names inherited by a class instance or those returned by a function.

The inherited names include 'object'.
"""
if isinstance(node, bases.UnboundMethod):
return [i.name for i in node.infer_call_result(node)]
if isinstance(node, bases.Instance):
return [node.name] + [x.name for x in node.ancestors()]

# For example for astroid.nodes.Uninferable
return []
12 changes: 8 additions & 4 deletions tests/checkers/unittest_refactoring.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,27 @@ class ChildClassWithoutBool(ClassWithoutBool):
"""
)
with_bool, without_bool, child_with_bool, child_without_bool = module.body
assert ImplicitBooleanessChecker().base_classes_of_node(with_bool) == [
assert ImplicitBooleanessChecker().base_classes_and_return_names(with_bool) == [
"ClassWithBool",
"list",
"object",
]
assert ImplicitBooleanessChecker().base_classes_of_node(without_bool) == [
assert ImplicitBooleanessChecker().base_classes_and_return_names(without_bool) == [
"ClassWithoutBool",
"dict",
"object",
]
assert ImplicitBooleanessChecker().base_classes_of_node(child_with_bool) == [
assert ImplicitBooleanessChecker().base_classes_and_return_names(
child_with_bool
) == [
"ChildClassWithBool",
"ClassWithBool",
"list",
"object",
]
assert ImplicitBooleanessChecker().base_classes_of_node(child_without_bool) == [
assert ImplicitBooleanessChecker().base_classes_and_return_names(
child_without_bool
) == [
"ChildClassWithoutBool",
"ClassWithoutBool",
"dict",
Expand Down
43 changes: 43 additions & 0 deletions tests/functional/u/use/use_implicit_booleaness_not_comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,3 +194,46 @@ def test_function():
long_test = {}
if long_test == { }: # [use-implicit-booleaness-not-comparison]
pass


# Check for properties and uninferable class methods
# See https://github.com/PyCQA/pylint/issues/5646
from xyz import AnotherClassWithProperty


class ParentWithProperty:

@classmethod
@property
def parent_function(cls):
return {}


class MyClassWithProxy(ParentWithProperty):

attribute = True

@property
@classmethod
def my_property(cls):
return {}

@property
@classmethod
def my_difficult_property(cls):
if cls.attribute:
return {}
return MyClassWithProxy()



def test_func():
"""Some assertions against empty dicts."""
my_class = MyClassWithProxy()
assert my_class.parent_function == {} # [use-implicit-booleaness-not-comparison]
assert my_class.my_property == {} # [use-implicit-booleaness-not-comparison]

# If the return value is not always implicit boolean, don't raise
assert my_class.my_difficult_property == {}
# Uninferable does not raise
assert AnotherClassWithProperty().my_property == {}
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@ use-implicit-booleaness-not-comparison:160:3:160:20::'numpy_array >= ()' can be
use-implicit-booleaness-not-comparison:185:3:185:13::'data == {}' can be simplified to 'not data' as an empty sequence is falsey:UNDEFINED
use-implicit-booleaness-not-comparison:187:3:187:13::'data != {}' can be simplified to 'data' as an empty sequence is falsey:UNDEFINED
use-implicit-booleaness-not-comparison:195:3:195:26::'long_test == {}' can be simplified to 'not long_test' as an empty sequence is falsey:UNDEFINED
use-implicit-booleaness-not-comparison:233:11:233:41:test_func:'my_class.parent_function == {}' can be simplified to 'not my_class.parent_function' as an empty sequence is falsey:UNDEFINED
use-implicit-booleaness-not-comparison:234:11:234:37:test_func:'my_class.my_property == {}' can be simplified to 'not my_class.my_property' as an empty sequence is falsey:UNDEFINED