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

Ignore cached_property in method-hidden check (#8753) #8758

Merged
merged 10 commits into from
Jun 22, 2023
3 changes: 3 additions & 0 deletions doc/whatsnew/fragments/8753.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fix a false positive for ``method-hidden`` when using ``cached_property`` decorator.

Closes #8753
25 changes: 24 additions & 1 deletion pylint/checkers/classes/class_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
_AccessNodes = Union[nodes.Attribute, nodes.AssignAttr]

INVALID_BASE_CLASSES = {"bool", "range", "slice", "memoryview"}
ALLOWED_PROPERTIES = {"bultins.property", "functools.cached_property"}
BUILTIN_DECORATORS = {"builtins.property", "builtins.classmethod"}
ASTROID_TYPE_COMPARATORS = {
nodes.Const: lambda a, b: a.value == b.value,
Expand Down Expand Up @@ -1252,11 +1253,15 @@ def visit_functiondef(self, node: nodes.FunctionDef) -> None:
# attribute affectation will call this method, not hiding it
return
if isinstance(decorator, nodes.Name):
if decorator.name == "property":
if decorator.name in ALLOWED_PROPERTIES:
# attribute affectation will either call a setter or raise
# an attribute error, anyway not hiding the function
return

if isinstance(decorator, nodes.Attribute):
if self._check_functools_or_not(decorator):
return

# Infer the decorator and see if it returns something useful
inferred = safe_infer(decorator)
if not inferred:
Expand Down Expand Up @@ -1454,6 +1459,24 @@ def _check_invalid_overridden_method(
node=function_node,
)

def _check_functools_or_not(self, decorator: nodes.Attribute) -> bool:
if decorator.attrname != "cached_property":
return False
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

if not isinstance(decorator.expr, nodes.Name):
return False
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

_, import_nodes = decorator.expr.lookup(decorator.expr.name)

if not import_nodes:
return False
Copy link
Member

Choose a reason for hiding this comment

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

These last two lines should (hopefully) be easy to cover, also. Get a "functools" that's not from an import statement, and one that's missing (and disable undefined-variable inline, or whatever similar msg)

Copy link
Member

Choose a reason for hiding this comment

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

However, those would make pretty low-value tests, so I'm approving now with an eye toward merging later tonight. Thank you!

import_node = import_nodes[0]

if not isinstance(import_node, (astroid.Import, astroid.ImportFrom)):
return False

return "functools" in dict(import_node.names)

def _check_slots(self, node: nodes.ClassDef) -> None:
if "__slots__" not in node.locals:
return
Expand Down
13 changes: 13 additions & 0 deletions tests/functional/m/method_hidden.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# pylint: disable=unused-private-member
"""check method hiding ancestor attribute
"""
import functools as ft
import something_else as functools # pylint: disable=import-error


class Abcd:
Expand Down Expand Up @@ -106,13 +108,24 @@ def default(self, o):
class Parent:
def __init__(self):
self._protected = None
self._protected_two = None


class Child(Parent):
def _protected(self): # [method-hidden]
pass


class CachedChild(Parent):
@ft.cached_property
def _protected(self):
pass

Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved
@functools.cached_property
def _protected_two(self):
Copy link
Contributor Author

@kyoto7250 kyoto7250 Jun 11, 2023

Choose a reason for hiding this comment

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

There is no actual definition cached_property, so safe_infer returns UnInferable, and this line did not detect.

By adding the actual definition, this line will be detected, any good ideas?

inferred = safe_infer(decorator)
if not inferred:

Copy link
Member

Choose a reason for hiding this comment

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

It's the expected behavior πŸ‘ We should not raise when something is uninferable (unless the real functools.cached_property is always uninferable).

pass


class ParentTwo:
def __init__(self):
self.__private = None
Expand Down
6 changes: 3 additions & 3 deletions tests/functional/m/method_hidden.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
method-hidden:17:4:17:12:Cdef.abcd:An attribute defined in functional.m.method_hidden line 11 hides this method:UNDEFINED
method-hidden:85:4:85:11:One.one:An attribute defined in functional.m.method_hidden line 83 hides this method:UNDEFINED
method-hidden:112:4:112:18:Child._protected:An attribute defined in functional.m.method_hidden line 108 hides this method:UNDEFINED
method-hidden:19:4:19:12:Cdef.abcd:An attribute defined in functional.m.method_hidden line 13 hides this method:UNDEFINED
method-hidden:87:4:87:11:One.one:An attribute defined in functional.m.method_hidden line 85 hides this method:UNDEFINED
method-hidden:115:4:115:18:Child._protected:An attribute defined in functional.m.method_hidden line 110 hides this method:UNDEFINED
16 changes: 16 additions & 0 deletions tests/functional/m/method_hidden_py39.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# pylint: disable=too-few-public-methods,missing-docstring
"""check method hiding ancestor attribute
"""
import something_else as functools # pylint: disable=import-error


class Parent:
def __init__(self):
self._protected = None


class Child(Parent):
@functools().cached_property
def _protected(self):
# This test case is only valid for python3.9 and above
pass
2 changes: 2 additions & 0 deletions tests/functional/m/method_hidden_py39.rc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[testoptions]
min_pyver = 3.9