From 0c21c6e47eb13fe6d4e40ef2c13185c8182076d7 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 5 Sep 2020 22:20:27 +0200 Subject: [PATCH 01/13] Add new function to recover all base class of a Node --- pylint/checkers/refactoring/len_checker.py | 16 +++++++ tests/checkers/unittest_refactoring.py | 50 ++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 tests/checkers/unittest_refactoring.py diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index 71d439205e..07ccf4ebe7 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -2,6 +2,8 @@ # Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html # For details: https://github.com/PyCQA/pylint/blob/master/COPYING +from typing import List + import astroid from pylint import checkers, interfaces @@ -78,3 +80,17 @@ def visit_unaryop(self, node): and utils.is_call_of_name(node.operand, "len") ): self.add_message("len-as-condition", node=node) + + @staticmethod + def base_classes_of_node(instance: astroid.nodes.ClassDef) -> List[astroid.Name]: + mother_classes = [instance.name] + base_classes = instance.bases + while base_classes and "object" not in mother_classes: + new_instances = [] + for base_class in base_classes: + mother_classes.append(base_class.name) + inferred_class = next(base_class.infer()) + if inferred_class.bases: + new_instances += inferred_class.bases + base_classes = new_instances + return mother_classes diff --git a/tests/checkers/unittest_refactoring.py b/tests/checkers/unittest_refactoring.py new file mode 100644 index 0000000000..29bce13275 --- /dev/null +++ b/tests/checkers/unittest_refactoring.py @@ -0,0 +1,50 @@ +# -*- coding: utf-8 -*- + +# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html +# For details: https://github.com/PyCQA/pylint/blob/master/COPYING + +import astroid + +from pylint.checkers.refactoring import LenChecker + + +def test_class_tree_detection(): + module = astroid.parse( + """ +class ClassWithBool(list): + def __bool__(self): + return True + +class ClassWithoutBool(dict): + pass + +class ChildClassWithBool(ClassWithBool): + pass + +class ChildClassWithoutBool(ClassWithoutBool): + pass +""" + ) + with_bool, without_bool, child_with_bool, child_without_bool = module.body + assert LenChecker().base_classes_of_node(with_bool) == [ + "ClassWithBool", + "list", + "object", + ] + assert LenChecker().base_classes_of_node(without_bool) == [ + "ClassWithoutBool", + "dict", + "object", + ] + assert LenChecker().base_classes_of_node(child_with_bool) == [ + "ChildClassWithBool", + "ClassWithBool", + "list", + "object", + ] + assert LenChecker().base_classes_of_node(child_without_bool) == [ + "ChildClassWithoutBool", + "ClassWithoutBool", + "dict", + "object", + ] From 57648d51846ab505401b304d739f920eede8ea48 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 5 Sep 2020 22:23:02 +0200 Subject: [PATCH 02/13] Restrict the number of classes affected by len-as-condition --- pylint/checkers/refactoring/len_checker.py | 13 ++++++- tests/functional/l/len_checks.py | 45 +++++++++++++++++++++- tests/functional/l/len_checks.txt | 3 ++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index 07ccf4ebe7..ae4a502d03 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -63,11 +63,20 @@ def visit_call(self, node): parent = node.parent while isinstance(parent, astroid.BoolOp): parent = parent.parent - # we're finally out of any nested boolean operations so check if # this len() call is part of a test condition if utils.is_test_condition(node, parent): - self.add_message("len-as-condition", node=node) + instance = next(node.args[0].infer()) + mother_classes = self.base_classes_of_node(instance) + affected_by_pep8 = any( + t in mother_classes for t in ["str", "tuple", "range", "list"] + ) + if affected_by_pep8 and not self.instance_has_bool(instance): + self.add_message("len-as-condition", node=node) + + @staticmethod + def instance_has_bool(class_def: astroid.ClassDef) -> bool: + return any(hasattr(f, "name") and f.name == "__bool__" for f in class_def.body) @utils.check_messages("len-as-condition") def visit_unaryop(self, node): diff --git a/tests/functional/l/len_checks.py b/tests/functional/l/len_checks.py index 216a7e672a..98c33424ae 100644 --- a/tests/functional/l/len_checks.py +++ b/tests/functional/l/len_checks.py @@ -7,7 +7,7 @@ if not len('TEST'): # [len-as-condition] pass -z = False +z = [] if z and len(['T', 'E', 'S', 'T']): # [len-as-condition] pass @@ -102,3 +102,46 @@ def github_issue_1331_v4(*args): b = bool(len(z)) # [len-as-condition] c = bool(len('TEST') or 42) # [len-as-condition] + + +def github_issue_1879(): + + class ClassWithBool(list): + def __bool__(self): + return True + + class ClassWithoutBool(list): + pass + + class ChildClassWithBool(ClassWithBool): + pass + + class ChildClassWithoutBool(ClassWithoutBool): + pass + + assert len(ClassWithBool()) + # We could expect to not have a len-as-condition for ChildClassWithBool, + # but I don't think the required analysis is worth it. + assert len(ChildClassWithBool()) # [len-as-condition] + assert len(ClassWithoutBool()) # [len-as-condition] + assert len(ChildClassWithoutBool()) # [len-as-condition] + # assert len(range(0)) != 0 + + # pylint: disable=import-outside-toplevel + import numpy + numpy_array = numpy.array([0]) + if len(numpy_array) > 0: + print('numpy_array') + if len(numpy_array): + print('numpy_array') + if numpy_array: + print('b') + + import pandas as pd + pandas_df = pd.DataFrame() + if len(pandas_df): + print("this works, but pylint tells me not to use len() without comparison") + if len(pandas_df) > 0: + print("this works and pylint likes it, but it's not the solution intended by PEP-8") + if pandas_df: + print("this does not work (truth value of dataframe is ambiguous)") diff --git a/tests/functional/l/len_checks.txt b/tests/functional/l/len_checks.txt index 23e3f583f1..7fbf5e8dad 100644 --- a/tests/functional/l/len_checks.txt +++ b/tests/functional/l/len_checks.txt @@ -13,3 +13,6 @@ len-as-condition:98:github_issue_1331_v3:Do not use `len(SEQUENCE)` without comp len-as-condition:101:github_issue_1331_v4:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:103::Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:104::Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:125:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:126:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:127:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty From 651660f8e93a66ea269b1e2a65f8404e88b30aa7 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Sat, 5 Sep 2020 22:11:53 +0200 Subject: [PATCH 03/13] Handle range, generator list/dict/set comprehension --- pylint/checkers/refactoring/len_checker.py | 11 +++++++++-- tests/functional/l/len_checks.py | 6 +++++- tests/functional/l/len_checks.txt | 5 +++++ 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index ae4a502d03..5b5896e87b 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -67,11 +67,18 @@ def visit_call(self, node): # this len() call is part of a test condition if utils.is_test_condition(node, parent): instance = next(node.args[0].infer()) + try: + instance = next(node.args[0].infer()) + except astroid.InferenceError: + self.add_message("len-as-condition", node=node) + return mother_classes = self.base_classes_of_node(instance) affected_by_pep8 = any( - t in mother_classes for t in ["str", "tuple", "range", "list"] + t in mother_classes for t in ["str", "tuple", "list", "set"] ) - if affected_by_pep8 and not self.instance_has_bool(instance): + if "range" in mother_classes or ( + affected_by_pep8 and not self.instance_has_bool(instance) + ): self.add_message("len-as-condition", node=node) @staticmethod diff --git a/tests/functional/l/len_checks.py b/tests/functional/l/len_checks.py index 98c33424ae..df739bba9b 100644 --- a/tests/functional/l/len_checks.py +++ b/tests/functional/l/len_checks.py @@ -125,7 +125,11 @@ class ChildClassWithoutBool(ClassWithoutBool): assert len(ChildClassWithBool()) # [len-as-condition] assert len(ClassWithoutBool()) # [len-as-condition] assert len(ChildClassWithoutBool()) # [len-as-condition] - # assert len(range(0)) != 0 + assert len(range(0)) # [len-as-condition] + assert len([t + 1 for t in []]) # [len-as-condition] + assert len(u + 1 for u in []) # [len-as-condition] + assert len({"1":(v + 1) for v in {}}) # [len-as-condition] + assert len(set((w + 1) for w in set())) # [len-as-condition] # pylint: disable=import-outside-toplevel import numpy diff --git a/tests/functional/l/len_checks.txt b/tests/functional/l/len_checks.txt index 7fbf5e8dad..1f3b2a9b57 100644 --- a/tests/functional/l/len_checks.txt +++ b/tests/functional/l/len_checks.txt @@ -16,3 +16,8 @@ len-as-condition:104::Do not use `len(SEQUENCE)` without comparison to determine len-as-condition:125:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:126:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:127:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:128:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:129:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:130:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:131:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:132:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty From 00b4755755a629619603abc85098879b99909f71 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 7 Sep 2020 21:08:19 +0200 Subject: [PATCH 04/13] Add a message in the changelog and update contributors.txt --- CONTRIBUTORS.txt | 6 +----- ChangeLog | 4 ++++ doc/whatsnew/2.6.rst | 2 ++ 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt index 808b87da6c..e3013b912a 100644 --- a/CONTRIBUTORS.txt +++ b/CONTRIBUTORS.txt @@ -286,11 +286,7 @@ contributors: * Michael Scott Cuthbert: contributor -* Pierre Sassoulas : contributor - - Made C0412 (ungrouped import) compatible with isort - - Made multiple message with the same old name possible - - Made Pylint a little faster by refactoring the message store - - Broke down "missing-docstrings" between "module", "class" and "function" +* Pierre Sassoulas : maintainer, contributor * Nathan Marrow diff --git a/ChangeLog b/ChangeLog index b36f488adc..718db35da4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -103,6 +103,10 @@ Release date: TBA Close #3735 +* `len-as-conditions` is now triggered only for classes that are inheriting directly from list, dict, or set and not implementing the `__bool__` function, or from generators like range or list/dict/set comprehension. This should reduce the false positives for other classes, like pandas's DataFrame or numpy's Array. + + Close #1879 + What's New in Pylint 2.6.0? =========================== diff --git a/doc/whatsnew/2.6.rst b/doc/whatsnew/2.6.rst index 1002d1aadd..13929ecc71 100644 --- a/doc/whatsnew/2.6.rst +++ b/doc/whatsnew/2.6.rst @@ -56,3 +56,5 @@ Other Changes * Fix vulnerable regular expressions in ``pyreverse``. The ambiguities of vulnerable regular expressions are removed, making the repaired regular expressions safer and faster matching. .. _the migration guide in isort documentation: https://timothycrosley.github.io/isort/docs/upgrade_guides/5.0.0/#known_standard_library + +* `len-as-conditions` is now triggered only for classes that are inheriting directly from list, dict, or set and not implementing the `__bool__` function, or from generators like range or list/dict/set comprehension. This should reduce the false positive for other classes, like pandas's DataFrame or numpy's Array. From 9961c4ff98723fe621bbb5f7638d5aa7fe2fa2b8 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 7 Sep 2020 21:50:58 +0200 Subject: [PATCH 05/13] Add a docstring to LenChecker.base_classes_of_node Following review https://github.com/PyCQA/pylint/pull/3821/files#r484553606 --- pylint/checkers/refactoring/len_checker.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index 5b5896e87b..e07602c62e 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -99,6 +99,7 @@ def visit_unaryop(self, node): @staticmethod def base_classes_of_node(instance: astroid.nodes.ClassDef) -> List[astroid.Name]: + """Return all the classes names that a ClassDef inherit from including 'object'.""" mother_classes = [instance.name] base_classes = instance.bases while base_classes and "object" not in mother_classes: From 9944264cd0ccf17811d0e3d91ad192afbf888276 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 7 Sep 2020 22:20:56 +0200 Subject: [PATCH 06/13] Add a comment for unclear InferenceError --- pylint/checkers/refactoring/len_checker.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index e07602c62e..e5bbb9660a 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -70,6 +70,8 @@ def visit_call(self, node): try: instance = next(node.args[0].infer()) except astroid.InferenceError: + # Inference error happens for list comprehension, dict comprehension, + # set comprehension and generators (like range) self.add_message("len-as-condition", node=node) return mother_classes = self.base_classes_of_node(instance) From 7ff76be316d9e2f8b9a66507df59554b1122ca41 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 14 Sep 2020 20:22:14 +0200 Subject: [PATCH 07/13] Apply hippo91's suggestion for retrieving ancestors More efficient, more elegant. :ok_hand: --- pylint/checkers/refactoring/len_checker.py | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index e5bbb9660a..8073af9656 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -102,14 +102,7 @@ def visit_unaryop(self, node): @staticmethod def base_classes_of_node(instance: astroid.nodes.ClassDef) -> List[astroid.Name]: """Return all the classes names that a ClassDef inherit from including 'object'.""" - mother_classes = [instance.name] - base_classes = instance.bases - while base_classes and "object" not in mother_classes: - new_instances = [] - for base_class in base_classes: - mother_classes.append(base_class.name) - inferred_class = next(base_class.infer()) - if inferred_class.bases: - new_instances += inferred_class.bases - base_classes = new_instances - return mother_classes + try: + return [instance.name] + [x.name for x in instance.ancestors()] + except TypeError: + return [instance.name] From 43244d6991c41bf28cc82732066e070186354c92 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 14 Sep 2020 20:54:12 +0200 Subject: [PATCH 08/13] Add a test for function in len() call for len_check --- tests/functional/l/len_checks.py | 13 +++++++++++++ tests/functional/l/len_checks.txt | 1 + 2 files changed, 14 insertions(+) diff --git a/tests/functional/l/len_checks.py b/tests/functional/l/len_checks.py index df739bba9b..c1840a834e 100644 --- a/tests/functional/l/len_checks.py +++ b/tests/functional/l/len_checks.py @@ -149,3 +149,16 @@ class ChildClassWithoutBool(ClassWithoutBool): print("this works and pylint likes it, but it's not the solution intended by PEP-8") if pandas_df: print("this does not work (truth value of dataframe is ambiguous)") + + def function_returning_list(r): + if r==1: + return [1] + return [2] + + def function_returning_int(r): + if r==1: + return 1 + return 2 + + assert len(function_returning_list(z)) # [len-as-condition] + assert len(function_returning_int(z)) diff --git a/tests/functional/l/len_checks.txt b/tests/functional/l/len_checks.txt index 1f3b2a9b57..3824331371 100644 --- a/tests/functional/l/len_checks.txt +++ b/tests/functional/l/len_checks.txt @@ -21,3 +21,4 @@ len-as-condition:129:github_issue_1879:Do not use `len(SEQUENCE)` without compar len-as-condition:130:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:131:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:132:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:163:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty From 9375cb82bfdab3dbd61d14dfeb821d28b88d0b37 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 14 Sep 2020 21:59:25 +0200 Subject: [PATCH 09/13] Treat generator and comprehension more gracefully --- pylint/checkers/refactoring/len_checker.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index 8073af9656..b16e8aa797 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -5,6 +5,7 @@ from typing import List import astroid +from astroid import DictComp, GeneratorExp, ListComp, SetComp from pylint import checkers, interfaces from pylint.checkers import utils @@ -66,14 +67,13 @@ def visit_call(self, node): # we're finally out of any nested boolean operations so check if # this len() call is part of a test condition if utils.is_test_condition(node, parent): - instance = next(node.args[0].infer()) - try: - instance = next(node.args[0].infer()) - except astroid.InferenceError: - # Inference error happens for list comprehension, dict comprehension, - # set comprehension and generators (like range) + len_arg = node.args[0] + generator_or_comprehension = (ListComp, SetComp, DictComp, GeneratorExp) + if isinstance(len_arg, generator_or_comprehension): + # The node is a generator or comprehension as in len([x for x in ...]) self.add_message("len-as-condition", node=node) return + instance = next(len_arg.infer()) mother_classes = self.base_classes_of_node(instance) affected_by_pep8 = any( t in mother_classes for t in ["str", "tuple", "list", "set"] From 5234daeee2799005e700342996a443a9c03e8cf6 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Mon, 14 Sep 2020 22:12:44 +0200 Subject: [PATCH 10/13] Better and faster implementation of instance_has_bool --- pylint/checkers/refactoring/len_checker.py | 7 ++++++- tests/functional/l/len_checks.py | 4 +--- tests/functional/l/len_checks.txt | 4 ++-- 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index b16e8aa797..dc6c941539 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -85,7 +85,12 @@ def visit_call(self, node): @staticmethod def instance_has_bool(class_def: astroid.ClassDef) -> bool: - return any(hasattr(f, "name") and f.name == "__bool__" for f in class_def.body) + try: + class_def.getattr("__bool__") + return True + except astroid.AttributeInferenceError: + ... + return False @utils.check_messages("len-as-condition") def visit_unaryop(self, node): diff --git a/tests/functional/l/len_checks.py b/tests/functional/l/len_checks.py index c1840a834e..c486c6d43c 100644 --- a/tests/functional/l/len_checks.py +++ b/tests/functional/l/len_checks.py @@ -120,9 +120,7 @@ class ChildClassWithoutBool(ClassWithoutBool): pass assert len(ClassWithBool()) - # We could expect to not have a len-as-condition for ChildClassWithBool, - # but I don't think the required analysis is worth it. - assert len(ChildClassWithBool()) # [len-as-condition] + assert len(ChildClassWithBool()) assert len(ClassWithoutBool()) # [len-as-condition] assert len(ChildClassWithoutBool()) # [len-as-condition] assert len(range(0)) # [len-as-condition] diff --git a/tests/functional/l/len_checks.txt b/tests/functional/l/len_checks.txt index 3824331371..b53ce600af 100644 --- a/tests/functional/l/len_checks.txt +++ b/tests/functional/l/len_checks.txt @@ -13,6 +13,7 @@ len-as-condition:98:github_issue_1331_v3:Do not use `len(SEQUENCE)` without comp len-as-condition:101:github_issue_1331_v4:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:103::Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:104::Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:124:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:125:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:126:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:127:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty @@ -20,5 +21,4 @@ len-as-condition:128:github_issue_1879:Do not use `len(SEQUENCE)` without compar len-as-condition:129:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:130:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:131:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty -len-as-condition:132:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty -len-as-condition:163:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:161:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty From 250abec7d2bca3c1dd506b188b2924769715a264 Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Tue, 15 Sep 2020 07:43:58 +0200 Subject: [PATCH 11/13] Add a test for function with comprehension or generator Or a function returning a generator. --- tests/functional/l/len_checks.py | 15 ++++++++++++++- tests/functional/l/len_checks.txt | 2 ++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/functional/l/len_checks.py b/tests/functional/l/len_checks.py index c486c6d43c..6330d307c8 100644 --- a/tests/functional/l/len_checks.py +++ b/tests/functional/l/len_checks.py @@ -158,5 +158,18 @@ def function_returning_int(r): return 1 return 2 - assert len(function_returning_list(z)) # [len-as-condition] + def function_returning_generator(r): + for i in [r, 1, 2, 3]: + yield i + + def function_returning_comprehension(r): + return [x+1 for x in [r, 1, 2, 3]] + + def function_returning_function(r): + return function_returning_generator(r) + + assert len(function_returning_list(z)) # [len-as-condition] assert len(function_returning_int(z)) + assert len(function_returning_generator(z)) # [len-as-condition] + assert len(function_returning_comprehension(z)) # [len-as-condition] + assert len(function_returning_function(z)) # [len-as-condition] diff --git a/tests/functional/l/len_checks.txt b/tests/functional/l/len_checks.txt index b53ce600af..c149fa39ce 100644 --- a/tests/functional/l/len_checks.txt +++ b/tests/functional/l/len_checks.txt @@ -22,3 +22,5 @@ len-as-condition:129:github_issue_1879:Do not use `len(SEQUENCE)` without compar len-as-condition:130:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:131:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:161:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:170:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty +len-as-condition:171:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty From 07fc0dd786b484d92fd59f0d092803ebca91075f Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Thu, 1 Oct 2020 22:53:13 +0200 Subject: [PATCH 12/13] Use premature return to simplify the code --- pylint/checkers/refactoring/len_checker.py | 50 +++++++++++----------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/pylint/checkers/refactoring/len_checker.py b/pylint/checkers/refactoring/len_checker.py index dc6c941539..7ecb17a45c 100644 --- a/pylint/checkers/refactoring/len_checker.py +++ b/pylint/checkers/refactoring/len_checker.py @@ -58,30 +58,32 @@ def visit_call(self, node): # a len(S) call is used inside a test condition # could be if, while, assert or if expression statement # e.g. `if len(S):` - if utils.is_call_of_name(node, "len"): - # the len() call could also be nested together with other - # boolean operations, e.g. `if z or len(x):` - parent = node.parent - while isinstance(parent, astroid.BoolOp): - parent = parent.parent - # we're finally out of any nested boolean operations so check if - # this len() call is part of a test condition - if utils.is_test_condition(node, parent): - len_arg = node.args[0] - generator_or_comprehension = (ListComp, SetComp, DictComp, GeneratorExp) - if isinstance(len_arg, generator_or_comprehension): - # The node is a generator or comprehension as in len([x for x in ...]) - self.add_message("len-as-condition", node=node) - return - instance = next(len_arg.infer()) - mother_classes = self.base_classes_of_node(instance) - affected_by_pep8 = any( - t in mother_classes for t in ["str", "tuple", "list", "set"] - ) - if "range" in mother_classes or ( - affected_by_pep8 and not self.instance_has_bool(instance) - ): - self.add_message("len-as-condition", node=node) + if not utils.is_call_of_name(node, "len"): + return + # the len() call could also be nested together with other + # boolean operations, e.g. `if z or len(x):` + parent = node.parent + while isinstance(parent, astroid.BoolOp): + parent = parent.parent + # we're finally out of any nested boolean operations so check if + # this len() call is part of a test condition + if not utils.is_test_condition(node, parent): + return + len_arg = node.args[0] + generator_or_comprehension = (ListComp, SetComp, DictComp, GeneratorExp) + if isinstance(len_arg, generator_or_comprehension): + # The node is a generator or comprehension as in len([x for x in ...]) + self.add_message("len-as-condition", node=node) + return + instance = next(len_arg.infer()) + mother_classes = self.base_classes_of_node(instance) + affected_by_pep8 = any( + t in mother_classes for t in ["str", "tuple", "list", "set"] + ) + if "range" in mother_classes or ( + affected_by_pep8 and not self.instance_has_bool(instance) + ): + self.add_message("len-as-condition", node=node) @staticmethod def instance_has_bool(class_def: astroid.ClassDef) -> bool: From b228d77e5230b880774d51a30be1d6101563712d Mon Sep 17 00:00:00 2001 From: Pierre Sassoulas Date: Thu, 31 Dec 2020 11:10:10 +0100 Subject: [PATCH 13/13] Remove test for function inference as astroid is not ready yet --- tests/functional/l/len_checks.py | 22 ++++++++++++---------- tests/functional/l/len_checks.txt | 3 --- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/tests/functional/l/len_checks.py b/tests/functional/l/len_checks.py index 6330d307c8..eb10b6a3b5 100644 --- a/tests/functional/l/len_checks.py +++ b/tests/functional/l/len_checks.py @@ -158,18 +158,20 @@ def function_returning_int(r): return 1 return 2 - def function_returning_generator(r): - for i in [r, 1, 2, 3]: - yield i + # def function_returning_generator(r): + # for i in [r, 1, 2, 3]: + # yield i - def function_returning_comprehension(r): - return [x+1 for x in [r, 1, 2, 3]] + # def function_returning_comprehension(r): + # return [x+1 for x in [r, 1, 2, 3]] - def function_returning_function(r): - return function_returning_generator(r) + # def function_returning_function(r): + # return function_returning_generator(r) assert len(function_returning_list(z)) # [len-as-condition] assert len(function_returning_int(z)) - assert len(function_returning_generator(z)) # [len-as-condition] - assert len(function_returning_comprehension(z)) # [len-as-condition] - assert len(function_returning_function(z)) # [len-as-condition] + # This should raise a len-as-conditions once astroid can infer it + # See https://github.com/PyCQA/pylint/pull/3821#issuecomment-743771514 + # assert len(function_returning_generator(z)) + # assert len(function_returning_comprehension(z)) + # assert len(function_returning_function(z)) diff --git a/tests/functional/l/len_checks.txt b/tests/functional/l/len_checks.txt index c149fa39ce..3a039afca3 100644 --- a/tests/functional/l/len_checks.txt +++ b/tests/functional/l/len_checks.txt @@ -20,7 +20,4 @@ len-as-condition:127:github_issue_1879:Do not use `len(SEQUENCE)` without compar len-as-condition:128:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:129:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:130:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty -len-as-condition:131:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty -len-as-condition:161:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty -len-as-condition:170:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty len-as-condition:171:github_issue_1879:Do not use `len(SEQUENCE)` without comparison to determine if a sequence is empty