From b0164a65543bbdfa4014992b18920c3b9fe4538f Mon Sep 17 00:00:00 2001 From: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Date: Sun, 1 Aug 2021 17:33:04 +0200 Subject: [PATCH] PR 4783 - nested attributes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Squashed commit of the following: commit 49c4bbaa82189c4bc90245063db22c4c635a0973 Author: Yu Shao, Pang Date: Sun Aug 1 19:56:51 2021 +0800 Fix crash for `unused-private-member` when there are nested attributes commit 2ad824799c112336afa7da17217d563402b418b7 Merge: 8ceb26dc 1d09bc89 Author: yushao2 <36848472+yushao2@users.noreply.github.com> Date: Sun Aug 1 20:13:05 2021 +0800 Merge pull request #4709 from yushao2/fix-unused-private-member-4673 [unused-private-member] add logic for checking nested functions commit 1d09bc89fd5bac7e230170a63e35d0d4e08675e6 Author: Yu Shao, Pang Date: Sun Aug 1 20:03:42 2021 +0800 update pr based on review commit a4198cd4429bab1e9eed3a62146f155e4acdf1bc Author: Yu Shao, Pang Date: Sun Aug 1 19:21:36 2021 +0800 Update pr based on review commit c8b2cbbe379eb1337f059c27e48349e645f88399 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 4ffea0b03f6d9e9feb25585c84b3ba227e53cb05 Author: Yu Shao, Pang Date: Sun Jul 25 13:12:29 2021 +0800 Update pr based on review commit e4d624346b1be8ae0600a226d282c806cc130b1d Author: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Date: Sun Jul 18 17:23:31 2021 +0200 Remove empty line commit cce5833f31d5d0412bce26010835e863a10820c5 Author: Yu Shao, Pang Date: Sun Jul 18 22:40:54 2021 +0800 update PR based on comments commit 712dc2b0b8128f3a56ecef8347cf23bdd538069a Author: Yu Shao, Pang 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 8ceb26dc66912e8f65adce0bc6ba22e87ceed3bb Author: Michal Vasilek Date: Sun Aug 1 08:14:58 2021 +0200 Fix IsADirectoryError in tests/lint/unittest_lint (#4781) pylintd is a directory, so os.remove throws IsADirectoryError commit a31e6bce1cc97611c4f58407bc613db42eb174c7 Author: Pierre Sassoulas Date: Sat Jul 31 11:21:46 2021 +0200 Add documentation for useless-suppression Closes #4757 commit b71be8a07ecc7d8aafbc07733e458b338eef6155 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 (#4745) * Pyreverse - Show class has-a relationships inferred from type-hints Closes #4744 Co-authored-by: Pierre Sassoulas commit 5e5f48d05fb6f1322a7420d6ea7586966b08f80a 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 (#4771) * Add ``no-breakpoint`` checker this adds a checker for calls to ``breakpoint()``, ``pdb.set_trace()``, or ``sys.breakpointhook()``. Closes #3692 Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com> Co-authored-by: Pierre Sassoulas --- CONTRIBUTORS.txt | 2 + ChangeLog | 16 ++++++ doc/user_guide/message-control.rst | 8 +++ doc/whatsnew/2.10.rst | 6 ++ pylint/checkers/classes.py | 24 ++++++-- pylint/checkers/stdlib.py | 10 ++++ pylint/pyreverse/utils.py | 6 +- tests/data/classes_No_Name.dot | 12 ++-- tests/data/clientmodule_test.py | 5 +- tests/data/suppliermodule_test.py | 2 + .../f/forgotten_debug_statement_py37.py | 10 ++++ .../f/forgotten_debug_statement_py37.rc | 2 + .../f/forgotten_debug_statement_py37.txt | 4 ++ .../u/unused/unused_private_member.py | 57 +++++++++++++++++++ .../u/unused/unused_private_member.txt | 3 + tests/lint/unittest_lint.py | 2 +- tests/unittest_pyreverse_diadefs.py | 4 ++ tests/unittest_pyreverse_inspector.py | 4 +- tests/unittest_pyreverse_writer.py | 18 ++++++ 19 files changed, 178 insertions(+), 17 deletions(-) create mode 100644 tests/functional/f/forgotten_debug_statement_py37.py create mode 100644 tests/functional/f/forgotten_debug_statement_py37.rc create mode 100644 tests/functional/f/forgotten_debug_statement_py37.txt diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index d19b12c4727..65cba0af15e 100644 --- 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 index b8dce9a625b..8cf5d2a8c86 100644 --- 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 index 18c9b18a8dd..217b1b5560a 100644 --- 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 index 6bfea5b5f65..26b792a6ab9 100644 --- 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 index 65ebe3423c5..ddf5710ec09 100644 --- 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 index afc5faa80fb..7eabe9bb406 100644 --- 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 index 06f8d3b7e2a..6eb9d43ccfb 100644 --- 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 index 8867b4e416b..c5eb70f0c45 100644 --- 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 index 40db2e77efe..82deaaf6f86 100644 --- 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 index 24dc9a02fef..6af30fa08a7 100644 --- 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 new file mode 100644 index 00000000000..ebe0a7a44d3 --- /dev/null +++ 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 new file mode 100644 index 00000000000..a17bb22dafb --- /dev/null +++ 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 new file mode 100644 index 00000000000..40e8914a678 --- /dev/null +++ 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 index 74dad6fe446..5aba1a9f7cc 100644 --- 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 index b21806c2c9c..73ee18050d5 100644 --- 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 index cd4f5667bb7..20c8a750040 100644 --- 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 index fbcf7fae320..4355b703acd 100644 --- 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 index bc37bea7139..a927e18ad96 100644 --- 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 index 9e6cb755bd1..8a33eb7a35f 100644 --- 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)