Skip to content

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

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 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

if not isinstance(decorator.expr, nodes.Name):
return False

_, 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
14 changes: 14 additions & 0 deletions tests/functional/m/method_hidden.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
# pylint: disable=too-few-public-methods,missing-docstring
# pylint: disable=unused-private-member
# pylint: disable=import-error
"""check method hiding ancestor attribute
"""
import functools as ft
import something_else as functools


class Abcd:
Expand Down Expand Up @@ -106,13 +109,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

@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:20:4:20:12:Cdef.abcd:An attribute defined in functional.m.method_hidden line 14 hides this method:UNDEFINED
method-hidden:88:4:88:11:One.one:An attribute defined in functional.m.method_hidden line 86 hides this method:UNDEFINED
method-hidden:116:4:116:18:Child._protected:An attribute defined in functional.m.method_hidden line 111 hides this method:UNDEFINED
142 changes: 142 additions & 0 deletions tests/functional/m/method_hidden_py39.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
# pylint: disable=too-few-public-methods,missing-docstring
# pylint: disable=unused-private-member
# pylint: disable=import-error
"""check method hiding ancestor attribute
"""
import functools as ft
import something_else as functools


class Abcd:
"""dummy"""

def __init__(self):
self.abcd = 1


class Cdef(Abcd):
"""dummy"""

def abcd(self): # [method-hidden]
"""test"""
print(self)


class AbcdMixin:
def abcd(self):
pass


class Dabc(AbcdMixin, Abcd):
def abcd(self):
pass


class CustomProperty:
"""dummy"""

def __init__(self, _):
pass

def __get__(self, obj, __):
if not obj:
return self
return 5

def __set__(self, _, __):
pass


class Ddef:
"""dummy"""

def __init__(self):
self.five = "five"

@CustomProperty
def five(self):
"""Always 5."""
return self


def my_decorator(*args, **kwargs):
return CustomProperty(*args, **kwargs)


class Foo:
def __init__(self):
self._bar = 42
self._baz = 84

@my_decorator
def method(self): # E0202
return self._baz

@method.setter
def method(self, value):
self._baz = value

def do_something_with_baz(self, value):
self.method = value


class One:
def __init__(self, one=None):
if one is not None:
self.one = one

def one(self): # [method-hidden]
pass


class Two(One):
def one(self):
pass


try:
import unknown as js
except ImportError:
import json as js


class JsonEncoder(js.JSONEncoder):
# pylint: disable=useless-super-delegation,super-with-arguments
def default(self, o):
return super(JsonEncoder, self).default(o)


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


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


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

@functools.cached_property
def _protected_two(self):
pass

@functools().cached_property
def _protected_three(self):
pass


class ParentTwo:
def __init__(self):
self.__private = None


class ChildTwo(ParentTwo):
def __private(self):
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
3 changes: 3 additions & 0 deletions tests/functional/m/method_hidden_py39.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
method-hidden:20:4:20:12:Cdef.abcd:An attribute defined in functional.m.method_hidden_py39 line 14 hides this method:UNDEFINED
method-hidden:88:4:88:11:One.one:An attribute defined in functional.m.method_hidden_py39 line 86 hides this method:UNDEFINED
method-hidden:117:4:117:18:Child._protected:An attribute defined in functional.m.method_hidden_py39 line 111 hides this method:UNDEFINED