PR 4783 - nested attributes

Squashed commit of the following: commit 49c4bba Author: Yu Shao, Pang <p.yushao2@gmail.com> Date: Sun Aug 1 19:56:51 2021 +0800 Fix crash for `unused-private-member` when there are nested attributes commit 2ad8247 Merge: 8ceb26d 1d09bc8 Author: yushao2 <36848472+yushao2@users.noreply.github.com> Date: Sun Aug 1 20:13:05 2021 +0800 Merge pull request pylint-dev#4709 from yushao2/fix-unused-private-member-4673 [unused-private-member] add logic for checking nested functions commit 1d09bc8 Author: Yu Shao, Pang <p.yushao2@gmail.com> Date: Sun Aug 1 20:03:42 2021 +0800 update pr based on review commit a4198cd Author: Yu Shao, Pang <p.yushao2@gmail.com> Date: Sun Aug 1 19:21:36 2021 +0800 Update pr based on review commit c8b2cbb Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun Jul 25 05:20:42 2021 +0000 [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci commit 4ffea0b Author: Yu Shao, Pang <p.yushao2@gmail.com> Date: Sun Jul 25 13:12:29 2021 +0800 Update pr based on review commit e4d6243 Author: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Date: Sun Jul 18 17:23:31 2021 +0200 Remove empty line commit cce5833 Author: Yu Shao, Pang <p.yushao2@gmail.com> Date: Sun Jul 18 22:40:54 2021 +0800 update PR based on comments commit 712dc2b Author: Yu Shao, Pang <p.yushao2@gmail.com> Date: Wed Jul 14 16:42:25 2021 +0800 [unused-private-member] add logic for checking nested functions also, improve error message for nested functions commit 8ceb26d Author: Michal Vasilek <michal@vasilek.cz> Date: Sun Aug 1 08:14:58 2021 +0200 Fix IsADirectoryError in tests/lint/unittest_lint (pylint-dev#4781) pylintd is a directory, so os.remove throws IsADirectoryError commit a31e6bc Author: Pierre Sassoulas <pierre.sassoulas@gmail.com> Date: Sat Jul 31 11:21:46 2021 +0200 Add documentation for useless-suppression Closes pylint-dev#4757 commit b71be8a Author: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com> Date: Fri Jul 30 20:21:02 2021 +0200 Handle has-a relationships for type-hinted arguments in class diagrams (pylint-dev#4745) * Pyreverse - Show class has-a relationships inferred from type-hints Closes pylint-dev#4744 Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com> commit 5e5f48d Author: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Date: Thu Jul 29 23:44:30 2021 +0200 Add ``forgotten-debug-statement`` checker (pylint-dev#4771) * Add ``no-breakpoint`` checker this adds a checker for calls to ``breakpoint()``, ``pdb.set_trace()``, or ``sys.breakpointhook()``. Closes pylint-dev#3692 Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
cdce8p · Aug 1, 2021 · b0164a6 · b0164a6
1 parent a051aa8
commit b0164a6
Show file tree

Hide file tree

Showing 19 changed files with 178 additions and 17 deletions.
diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
@@ -527,3 +527,5 @@ contributors:
 * Eisuke Kawashima (e-kwsm): contributor
 
 * Daniel van Noord (DanielNoord): contributor
+
+* Michal Vasilek: contributor
diff --git a/ChangeLog b/ChangeLog
@@ -12,6 +12,10 @@ Release date: TBA
 ..
   Put bug fixes that should not wait for a new minor version here
 
+* pyreverse: Show class has-a relationships inferred from the type-hint
+
+  Closes #4744
+
 * Added ``ignored-parents`` option to the design checker to ignore specific
   classes from the ``too-many-ancestors`` check (R0901).
 
@@ -48,6 +52,18 @@ Release date: TBA
 
   * Extended ``consider-using-tuple`` check to cover ``in`` comparisons.
 
+* Added ``forgotten-debug-statement``: Emitted when ``breakpoint``, ``pdb.set_trace`` or ``sys.breakpointhook`` calls are found
+
+  Closes #3692
+
+* Fix false-positive of ``unused-private-member`` when using nested functions in a class
+
+  Closes #4673
+
+* Fix crash for ``unused-private-member`` that occurs when there is a presence of nested attributes
+
+  Closes #4755
+
 
 What's New in Pylint 2.9.6?
 ===========================

diff --git a/doc/user_guide/message-control.rst b/doc/user_guide/message-control.rst
@@ -178,3 +178,11 @@ Here's an example with all these rules in a single place:
             # no error
             print(self.bla)
             print(self.blop)
+
+
+Detecting useless disables
+--------------------------
+
+When pylint get better and false positives are removed,
+disable that became useless can accumulate and clutter the code.
+In order to clean them you can enable the ``useless-suppression`` warning.
diff --git a/doc/whatsnew/2.10.rst b/doc/whatsnew/2.10.rst
@@ -24,6 +24,10 @@ New checkers
 
   Closes #4365
 
+* Added ``forgotten-debug-statement``: Emitted when ``breakpoint``, ``pdb.set_trace`` or ``sys.breakpointhook`` calls are found
+
+  Closes #3692
+
 
 Extensions
 ==========
@@ -36,6 +40,8 @@ Extensions
 Other Changes
 =============
 
+* Pyreverse - Show class has-a relationships inferred from type-hints
+
 * Performance of the Similarity checker has been improved.
 
 * Added ``time.clock`` to deprecated functions/methods for python 3.3

diff --git a/pylint/checkers/classes.py b/pylint/checkers/classes.py
@@ -909,6 +909,13 @@ def _check_unused_private_functions(self, node: astroid.ClassDef) -> None:
             function_def = cast(astroid.FunctionDef, function_def)
             if not is_attr_private(function_def.name):
                 continue
+            parent_scope = function_def.parent.scope()
+            if isinstance(parent_scope, astroid.FunctionDef):
+                # Handle nested functions
+                if function_def.name in (
+                    n.name for n in parent_scope.nodes_of_class(astroid.Name)
+                ):
+                    continue
             for attribute in node.nodes_of_class(astroid.Attribute):
                 attribute = cast(astroid.Attribute, attribute)
                 if (
@@ -931,11 +938,19 @@ def _check_unused_private_functions(self, node: astroid.ClassDef) -> None:
                     ):
                         break
             else:
-                function_repr = f"{function_def.name}({function_def.args.as_string()})"
+                name_stack = []
+                curr = parent_scope
+                # Generate proper names for nested functions
+                while curr != node:
+                    name_stack.append(curr.name)
+                    curr = curr.parent.scope()
+
+                outer_level_names = f"{'.'.join(reversed(name_stack))}"
+                function_repr = f"{outer_level_names}.{function_def.name}({function_def.args.as_string()})"
                 self.add_message(
                     "unused-private-member",
                     node=function_def,
-                    args=(node.name, function_repr),
+                    args=(node.name, function_repr.lstrip(".")),
                 )
 
     def _check_unused_private_variables(self, node: astroid.ClassDef) -> None:
@@ -959,9 +974,10 @@ def _check_unused_private_variables(self, node: astroid.ClassDef) -> None:
 
     def _check_unused_private_attributes(self, node: astroid.ClassDef) -> None:
         for assign_attr in node.nodes_of_class(astroid.AssignAttr):
-            if not is_attr_private(assign_attr.attrname):
+            if not is_attr_private(assign_attr.attrname) or not isinstance(
+                assign_attr.expr, astroid.Name
+            ):
                 continue
-
             # Logic for checking false positive when using __new__,
             # Get the returned object names of the __new__ magic function
             # Then check if the attribute was consumed in other instance methods

diff --git a/pylint/checkers/stdlib.py b/pylint/checkers/stdlib.py
@@ -55,6 +55,7 @@
 SUBPROCESS_POPEN = "subprocess.Popen"
 SUBPROCESS_RUN = "subprocess.run"
 OPEN_MODULE = "_io"
+DEBUG_BREAKPOINTS = ("builtins.breakpoint", "sys.breakpointhook", "pdb.set_trace")
 
 
 DEPRECATED_MODULES = {
@@ -434,6 +435,12 @@ class StdlibChecker(DeprecatedMixin, BaseChecker):
             "Using the system default implicitly can create problems on other operating systems. "
             "See https://www.python.org/dev/peps/pep-0597/",
         ),
+        "W1515": (
+            "Leaving functions creating breakpoints in production code is not recommended",
+            "forgotten-debug-statement",
+            "Calls to breakpoint(), sys.breakpointhook() and pdb.set_trace() should be removed "
+            "from code that is not actively being debugged.",
+        ),
     }
 
     def __init__(self, linter=None):
@@ -495,6 +502,7 @@ def _check_shallow_copy_environ(self, node):
         "subprocess-run-check",
         "deprecated-class",
         "unspecified-encoding",
+        "forgotten-debug-statement",
     )
     def visit_call(self, node):
         """Visit a Call node."""
@@ -531,6 +539,8 @@ def visit_call(self, node):
                         self._check_env_function(node, inferred)
                     elif name == SUBPROCESS_RUN:
                         self._check_for_check_kw_in_run(node)
+                    elif name in DEBUG_BREAKPOINTS:
+                        self.add_message("forgotten-debug-statement", node=node)
                 self.check_deprecated_method(node, inferred)
         except astroid.InferenceError:
             return

diff --git a/pylint/pyreverse/utils.py b/pylint/pyreverse/utils.py
@@ -269,9 +269,9 @@ def infer_node(node: Union[astroid.AssignAttr, astroid.AssignName]) -> set:
     otherwise return a set of the inferred types using the NodeNG.infer method"""
 
     ann = get_annotation(node)
-    if ann:
-        return {ann}
     try:
+        if ann:
+            return set(ann.infer())
         return set(node.infer())
     except astroid.InferenceError:
-        return set()
+        return {ann} if ann else set()
diff --git a/tests/data/classes_No_Name.dot b/tests/data/classes_No_Name.dot
@@ -3,10 +3,12 @@ charset="utf-8"
 rankdir=BT
 "0" [label="{Ancestor|attr : str\lcls_member\l|get_value()\lset_value(value)\l}", shape="record"];
 "1" [label="{DoNothing|\l|}", shape="record"];
-"2" [label="{Interface|\l|get_value()\lset_value(value)\l}", shape="record"];
-"3" [label="{Specialization|TYPE : str\lrelation\ltop : str\l|}", shape="record"];
-"3" -> "0" [arrowhead="empty", arrowtail="none"];
-"0" -> "2" [arrowhead="empty", arrowtail="node", style="dashed"];
+"2" [label="{DoNothing2|\l|}", shape="record"];
+"3" [label="{Interface|\l|get_value()\lset_value(value)\l}", shape="record"];
+"4" [label="{Specialization|TYPE : str\lrelation\lrelation2\ltop : str\l|}", shape="record"];
+"4" -> "0" [arrowhead="empty", arrowtail="none"];
+"0" -> "3" [arrowhead="empty", arrowtail="node", style="dashed"];
 "1" -> "0" [arrowhead="diamond", arrowtail="none", fontcolor="green", label="cls_member", style="solid"];
-"1" -> "3" [arrowhead="diamond", arrowtail="none", fontcolor="green", label="relation", style="solid"];
+"1" -> "4" [arrowhead="diamond", arrowtail="none", fontcolor="green", label="relation", style="solid"];
+"2" -> "4" [arrowhead="diamond", arrowtail="none", fontcolor="green", label="relation2", style="solid"];
 }
diff --git a/tests/data/clientmodule_test.py b/tests/data/clientmodule_test.py
@@ -1,5 +1,5 @@
 """ docstring for file clientmodule.py """
-from data.suppliermodule_test import Interface, DoNothing
+from data.suppliermodule_test import Interface, DoNothing, DoNothing2
 
 class Ancestor:
     """ Ancestor method """
@@ -23,7 +23,8 @@ class Specialization(Ancestor):
     TYPE = 'final class'
     top = 'class'
 
-    def __init__(self, value, _id):
+    def __init__(self, value, _id, relation2: DoNothing2):
         Ancestor.__init__(self, value)
         self._id = _id
         self.relation = DoNothing()
+        self.relation2 = relation2
diff --git a/tests/data/suppliermodule_test.py b/tests/data/suppliermodule_test.py
@@ -8,3 +8,5 @@ def set_value(self, value):
         raise NotImplementedError
 
 class DoNothing: pass
+
+class DoNothing2: pass
diff --git a/tests/functional/f/forgotten_debug_statement_py37.py b/tests/functional/f/forgotten_debug_statement_py37.py
@@ -0,0 +1,10 @@
+# pylint: disable=missing-docstring
+
+import pdb
+import sys
+
+breakpoint()  # [forgotten-debug-statement]
+sys.breakpointhook()  # [forgotten-debug-statement]
+pdb.set_trace()  # [forgotten-debug-statement]
+b = breakpoint
+b()  # [forgotten-debug-statement]
diff --git a/tests/functional/f/forgotten_debug_statement_py37.rc b/tests/functional/f/forgotten_debug_statement_py37.rc
@@ -0,0 +1,2 @@
+[testoptions]
+min_pyver=3.7
diff --git a/tests/functional/f/forgotten_debug_statement_py37.txt b/tests/functional/f/forgotten_debug_statement_py37.txt
@@ -0,0 +1,4 @@
+forgotten-debug-statement:6:0::"Leaving functions creating breakpoints in production code is not recommended"
+forgotten-debug-statement:7:0::"Leaving functions creating breakpoints in production code is not recommended"
+forgotten-debug-statement:8:0::"Leaving functions creating breakpoints in production code is not recommended"
+forgotten-debug-statement:10:0::"Leaving functions creating breakpoints in production code is not recommended"
diff --git a/tests/functional/u/unused/unused_private_member.py b/tests/functional/u/unused/unused_private_member.py
@@ -160,3 +160,60 @@ def __new__(cls, func, *args):
     def exec(self):
         print(self.__secret_bool)
         return self.func(*self.__args)
+
+
+# Test cases for false-positive reported in #4673
+# https://github.com/PyCQA/pylint/issues/4673
+class FalsePositive4673:
+    """ The testing class """
+
+    def __init__(self, in_thing):
+        self.thing = False
+        self.do_thing(in_thing)
+
+    def do_thing(self, in_thing):
+        """ Checks the false-positive condition, sets a property. """
+        def __false_positive(in_thing):
+            print(in_thing)
+
+        def __true_positive(in_thing):  # [unused-private-member]
+            print(in_thing)
+
+        __false_positive(in_thing)
+        self.thing = True
+
+    def undo_thing(self):
+        """ Unsets a property. """
+        self.thing = False
+
+    def complicated_example(self, flag):
+        def __inner_1():
+            pass
+
+        def __inner_2():
+            pass
+
+        def __inner_3(fn):
+            return fn
+
+        def __inner_4():  # [unused-private-member]
+            pass
+
+        fn_to_return = __inner_1 if flag else __inner_3(__inner_2)
+        return fn_to_return
+
+
+# Test cases for crash reported in #4755
+# https://github.com/PyCQA/pylint/issues/4755
+class Crash4755Context:
+    def __init__(self):
+        self.__messages = None  # [unused-private-member]
+
+class Crash4755Command:
+    def __init__(self):
+        self.context = Crash4755Context()
+
+    def method(self):
+        self.context.__messages = []
+        for message in self.context.__messages:
+            print(message)
diff --git a/tests/functional/u/unused/unused_private_member.txt b/tests/functional/u/unused/unused_private_member.txt
@@ -7,3 +7,6 @@ unused-private-member:34:4:HasUnusedInClass.__test:Unused private member `HasUnu
 unused-private-member:55:4:HasUnusedInClass.__test_recursive:Unused private member `HasUnusedInClass.__test_recursive(self)`:HIGH
 unused-private-member:132:8:FalsePositive4657.__init__:Unused private member `FalsePositive4657.__attr_c`:HIGH
 undefined-variable:137:15:FalsePositive4657.attr_c:Undefined variable 'cls':HIGH
+unused-private-member:179:8:FalsePositive4673.do_thing.__true_positive:Unused private member `FalsePositive4673.do_thing.__true_positive(in_thing)`:HIGH
+unused-private-member:199:8:FalsePositive4673.complicated_example.__inner_4:Unused private member `FalsePositive4673.complicated_example.__inner_4()`:HIGH
+unused-private-member:210:8:Crash4755Context.__init__:Unused private member `Crash4755Context.__messages`:HIGH
diff --git a/tests/lint/unittest_lint.py b/tests/lint/unittest_lint.py
@@ -643,7 +643,7 @@ def test_pylint_home():
             assert config.PYLINT_HOME == pylintd
         finally:
             try:
-                os.remove(pylintd)
+                rmtree(pylintd)
             except FileNotFoundError:
                 pass
     finally:

diff --git a/tests/unittest_pyreverse_diadefs.py b/tests/unittest_pyreverse_diadefs.py
@@ -99,6 +99,7 @@ class TestDefaultDiadefGenerator:
     _should_rels = [
         ("association", "DoNothing", "Ancestor"),
         ("association", "DoNothing", "Specialization"),
+        ("association", "DoNothing2", "Specialization"),
         ("implements", "Ancestor", "Interface"),
         ("specialization", "Specialization", "Ancestor"),
     ]
@@ -142,6 +143,7 @@ def test_known_values1(HANDLER, PROJECT):
     assert classes == [
         (True, "Ancestor"),
         (True, "DoNothing"),
+        (True, "DoNothing2"),
         (True, "Interface"),
         (True, "Specialization"),
     ]
@@ -170,6 +172,7 @@ def test_known_values3(HANDLER, PROJECT):
         (True, "data.clientmodule_test.Ancestor"),
         (True, special),
         (True, "data.suppliermodule_test.DoNothing"),
+        (True, "data.suppliermodule_test.DoNothing2"),
     ]
 
 
@@ -184,6 +187,7 @@ def test_known_values4(HANDLER, PROJECT):
     assert classes == [
         (True, "Ancestor"),
         (True, "DoNothing"),
+        (True, "DoNothing2"),
         (True, "Specialization"),
     ]
 

diff --git a/tests/unittest_pyreverse_inspector.py b/tests/unittest_pyreverse_inspector.py
@@ -62,9 +62,9 @@ def test_instance_attrs_resolution(project):
     klass = project.get_module("data.clientmodule_test")["Specialization"]
     assert hasattr(klass, "instance_attrs_type")
     type_dict = klass.instance_attrs_type
-    assert len(type_dict) == 2
+    assert len(type_dict) == 3
     keys = sorted(type_dict.keys())
-    assert keys == ["_id", "relation"]
+    assert keys == ["_id", "relation", "relation2"]
     assert isinstance(type_dict["relation"][0], astroid.bases.Instance), type_dict[
         "relation"
     ]

diff --git a/tests/unittest_pyreverse_writer.py b/tests/unittest_pyreverse_writer.py
@@ -204,3 +204,21 @@ def test_infer_node_2(mock_infer, mock_get_annotation):
     mock_infer.return_value = "x"
     assert infer_node(node) == set("x")
     assert mock_infer.called
+
+
+def test_infer_node_3():
+    """Return a set containing an astroid.ClassDef object when the attribute
+    has a type annotation"""
+    node = astroid.extract_node(
+        """
+        class Component:
+            pass
+
+        class Composite:
+            def __init__(self, component: Component):
+                self.component = component
+    """
+    )
+    instance_attr = node.instance_attrs.get("component")[0]
+    assert isinstance(infer_node(instance_attr), set)
+    assert isinstance(infer_node(instance_attr).pop(), astroid.ClassDef)