From e0b2b9c5c4f3d2fb0efaa667e1cc7e143fba58a8 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 27 Oct 2022 12:05:53 -0300 Subject: [PATCH 1/7] do not report unnecessary list index lookup if start arg is passed --- .../checkers/refactoring/refactoring_checker.py | 5 +++++ .../unnecessary_list_index_lookup.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index df3c3bccc8..2fc50822e0 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -2146,6 +2146,11 @@ def _check_unnecessary_list_index_lookup( # destructured, so we can't necessarily use it. return + if node.iter.keywords or len(node.iter.args) > 1: + # enumerate is being called with start arg/kwarg so resulting index lookup + # is not redundant so we should not report an error. + return + iterating_object_name = node.iter.args[0].name value_variable = node.target.elts[1] diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py index 62690ca5a3..9d98ab1ec3 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py @@ -81,3 +81,20 @@ def process_list_again(data): if i == 3: # more complex condition actually parts.insert(i, "X") print(part, parts[i]) + +# regression tests for https://github.com/PyCQA/pylint/issues/7682 +series = [1, 2, 3, 4, 5] +output_list = [ + (item, series[index]) + for index, item in enumerate(series, start=1) + if index < len(series) +] + +output_list = [ + (item, series[index]) + for index, item in enumerate(series, 1) + if index < len(series) +] + +for idx, val in enumerate(series, start=2): + print(my_list[idx]) From 809f39d94aa5e2cef08bd84d7cd0a786e5bf30db Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Thu, 27 Oct 2022 13:12:31 -0300 Subject: [PATCH 2/7] refactor --- doc/whatsnew/fragments/7682.bugfix | 3 +++ pylint/checkers/refactoring/refactoring_checker.py | 14 +++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) create mode 100644 doc/whatsnew/fragments/7682.bugfix diff --git a/doc/whatsnew/fragments/7682.bugfix b/doc/whatsnew/fragments/7682.bugfix new file mode 100644 index 0000000000..326c5628de --- /dev/null +++ b/doc/whatsnew/fragments/7682.bugfix @@ -0,0 +1,3 @@ +``unnecessary-list-index-lookup`` will not be emitted if ``enumerate`` is called with a `start` arg or kwarg. + +Closes #7682 diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 2fc50822e0..9018d2a7be 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -2146,7 +2146,7 @@ def _check_unnecessary_list_index_lookup( # destructured, so we can't necessarily use it. return - if node.iter.keywords or len(node.iter.args) > 1: + if self._enumerate_with_start(node): # enumerate is being called with start arg/kwarg so resulting index lookup # is not redundant so we should not report an error. return @@ -2242,3 +2242,15 @@ def _check_unnecessary_list_index_lookup( args=(node.target.elts[1].name,), confidence=HIGH, ) + + def _enumerate_with_start(self, node: nodes.For | nodes.Comprehension) -> bool: + """Check presence of `start` kwarg or second argument to enumerate + For example + `enumerate([1,2,3], start=1)` + `enumerate([1,2,3], 1)` + """ + if len(node.iter.args) > 1: + # We assume the second argument to `enumerate` is the `start` arg. + return True + + return any(keyword.arg == "start" for keyword in node.iter.keywords) From 9cb990b8b36be74e287efd3267e9d6f1a5ec08f9 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Fri, 28 Oct 2022 09:38:01 -0300 Subject: [PATCH 3/7] account for calling start with 0 or negative num --- doc/whatsnew/fragments/7682.bugfix | 2 +- .../refactoring/refactoring_checker.py | 34 ++++++++++++++++--- .../unnecessary_list_index_lookup.py | 17 +++++++++- .../unnecessary_list_index_lookup.txt | 2 ++ 4 files changed, 48 insertions(+), 7 deletions(-) diff --git a/doc/whatsnew/fragments/7682.bugfix b/doc/whatsnew/fragments/7682.bugfix index 326c5628de..3f94f447fd 100644 --- a/doc/whatsnew/fragments/7682.bugfix +++ b/doc/whatsnew/fragments/7682.bugfix @@ -1,3 +1,3 @@ -``unnecessary-list-index-lookup`` will not be emitted if ``enumerate`` is called with a `start` arg or kwarg. +``unnecessary-list-index-lookup`` will not be wrongly emitted if ``enumerate`` is called with ``start``. Closes #7682 diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 9018d2a7be..19e96b40d4 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -2244,13 +2244,37 @@ def _check_unnecessary_list_index_lookup( ) def _enumerate_with_start(self, node: nodes.For | nodes.Comprehension) -> bool: - """Check presence of `start` kwarg or second argument to enumerate - For example + """Check presence of `start` kwarg or second argument to enumerate. + + For example: + `enumerate([1,2,3], start=1)` `enumerate([1,2,3], 1)` + + If `start` is assigned to `0`, the default value, this is equivalent to + not calling `enumerate` with start. """ if len(node.iter.args) > 1: - # We assume the second argument to `enumerate` is the `start` arg. - return True + # We assume the second argument to `enumerate` is the `start` int arg. + # It's a reasonable assumption for now as it's the only possible argument: + # https://docs.python.org/3/library/functions.html#enumerate + start_arg = node.iter.args[1] + + start_val = ( + start_arg.operand.value + if isinstance(start_arg, nodes.UnaryOp) + else start_arg.value + ) + return not start_val == 0 + + for keyword in node.iter.keywords: + if keyword.arg == "start": + start_val = ( + keyword.value.operand.value + if isinstance(keyword.value, nodes.UnaryOp) + else keyword.value.value + ) - return any(keyword.arg == "start" for keyword in node.iter.keywords) + return not start_val == 0 + + return False diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py index 9d98ab1ec3..60eb1029ea 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py @@ -97,4 +97,19 @@ def process_list_again(data): ] for idx, val in enumerate(series, start=2): - print(my_list[idx]) + print(series[idx]) + +for idx, val in enumerate(series, 2): + print(series[idx]) + +for idx, val in enumerate(series, start=-2): + print(series[idx]) + +for idx, val in enumerate(series, -2): + print(series[idx]) + +for idx, val in enumerate(series, start=0): + print(series[idx]) # [unnecessary-list-index-lookup] + +for idx, val in enumerate(series, 0): + print(series[idx]) # [unnecessary-list-index-lookup] diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt index 9a894e139e..5d803a15cd 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt @@ -2,3 +2,5 @@ unnecessary-list-index-lookup:8:10:8:22::Unnecessary list index lookup, use 'val unnecessary-list-index-lookup:43:52:43:64::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:46:10:46:22::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:74:10:74:27::Unnecessary list index lookup, use 'val' instead:HIGH +unnecessary-list-index-lookup:112:10:112:21::Unnecessary list index lookup, use 'val' instead:HIGH +unnecessary-list-index-lookup:115:10:115:21::Unnecessary list index lookup, use 'val' instead:HIGH From ba5467bfdbe27553b429c6a6f4e85c7db1f163b1 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Mon, 31 Oct 2022 10:19:43 -0300 Subject: [PATCH 4/7] test for start as a var --- .../refactoring/refactoring_checker.py | 28 +++++++++++-------- .../unnecessary_list_index_lookup.py | 7 +++++ .../unnecessary_list_index_lookup.txt | 2 ++ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 19e96b40d4..80a5473c5b 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -2259,22 +2259,26 @@ def _enumerate_with_start(self, node: nodes.For | nodes.Comprehension) -> bool: # It's a reasonable assumption for now as it's the only possible argument: # https://docs.python.org/3/library/functions.html#enumerate start_arg = node.iter.args[1] - - start_val = ( - start_arg.operand.value - if isinstance(start_arg, nodes.UnaryOp) - else start_arg.value - ) + start_val = self._get_start_value(start_arg) + if start_val is None: + return False return not start_val == 0 for keyword in node.iter.keywords: if keyword.arg == "start": - start_val = ( - keyword.value.operand.value - if isinstance(keyword.value, nodes.UnaryOp) - else keyword.value.value - ) - + start_val = self._get_start_value(keyword.value) + if start_val is None: + return False return not start_val == 0 return False + + def _get_start_value(self, node: nodes.NodeNG) -> Any: + if isinstance(node, nodes.Name): + inferred = utils.safe_infer(node) + start_val = inferred.value if inferred else None + elif isinstance(node, nodes.UnaryOp): + start_val = node.operand.value + else: + start_val = node.value + return start_val diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py index 60eb1029ea..fbf0597255 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py @@ -113,3 +113,10 @@ def process_list_again(data): for idx, val in enumerate(series, 0): print(series[idx]) # [unnecessary-list-index-lookup] + +START = 0 +for idx, val in enumerate(series, start=START): + print(series[idx]) # [unnecessary-list-index-lookup] + +for idx, val in enumerate(series, START): + print(series[idx]) # [unnecessary-list-index-lookup] diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt index 5d803a15cd..74c6605f30 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt @@ -4,3 +4,5 @@ unnecessary-list-index-lookup:46:10:46:22::Unnecessary list index lookup, use 'v unnecessary-list-index-lookup:74:10:74:27::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:112:10:112:21::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:115:10:115:21::Unnecessary list index lookup, use 'val' instead:HIGH +unnecessary-list-index-lookup:119:10:119:21::Unnecessary list index lookup, use 'val' instead:HIGH +unnecessary-list-index-lookup:122:10:122:21::Unnecessary list index lookup, use 'val' instead:HIGH From fd93b4b4adf7d5c98c9dbe4946d4231dcbde6f1c Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Mon, 31 Oct 2022 16:56:37 -0300 Subject: [PATCH 5/7] use inference confidence when appropriate --- .../refactoring/refactoring_checker.py | 39 ++++++++++++------- .../unnecessary_list_index_lookup.txt | 4 +- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 80a5473c5b..f87e9f60e1 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -21,7 +21,7 @@ from pylint import checkers from pylint.checkers import utils from pylint.checkers.utils import node_frame_class -from pylint.interfaces import HIGH, INFERENCE +from pylint.interfaces import Confidence, HIGH, INFERENCE if TYPE_CHECKING: from pylint.lint import PyLinter @@ -2146,9 +2146,10 @@ def _check_unnecessary_list_index_lookup( # destructured, so we can't necessarily use it. return - if self._enumerate_with_start(node): + has_start_arg, confidence = self._enumerate_with_start(node) + if has_start_arg: # enumerate is being called with start arg/kwarg so resulting index lookup - # is not redundant so we should not report an error. + # is not redundant, hence we should not report an error. return iterating_object_name = node.iter.args[0].name @@ -2232,7 +2233,7 @@ def _check_unnecessary_list_index_lookup( "unnecessary-list-index-lookup", node=subscript, args=(node.target.elts[1].name,), - confidence=HIGH, + confidence=confidence, ) for subscript in bad_nodes: @@ -2240,10 +2241,12 @@ def _check_unnecessary_list_index_lookup( "unnecessary-list-index-lookup", node=subscript, args=(node.target.elts[1].name,), - confidence=HIGH, + confidence=confidence, ) - def _enumerate_with_start(self, node: nodes.For | nodes.Comprehension) -> bool: + def _enumerate_with_start( + self, node: nodes.For | nodes.Comprehension + ) -> tuple[bool, Confidence]: """Check presence of `start` kwarg or second argument to enumerate. For example: @@ -2254,31 +2257,37 @@ def _enumerate_with_start(self, node: nodes.For | nodes.Comprehension) -> bool: If `start` is assigned to `0`, the default value, this is equivalent to not calling `enumerate` with start. """ + confidence = HIGH + if len(node.iter.args) > 1: # We assume the second argument to `enumerate` is the `start` int arg. # It's a reasonable assumption for now as it's the only possible argument: # https://docs.python.org/3/library/functions.html#enumerate start_arg = node.iter.args[1] - start_val = self._get_start_value(start_arg) + start_val, confidence = self._get_start_value(start_arg) if start_val is None: - return False - return not start_val == 0 + return False, confidence + return not start_val == 0, confidence for keyword in node.iter.keywords: if keyword.arg == "start": - start_val = self._get_start_value(keyword.value) + start_val, confidence = self._get_start_value(keyword.value) if start_val is None: - return False - return not start_val == 0 + return False, confidence + return not start_val == 0, confidence - return False + return False, confidence + + def _get_start_value(self, node: nodes.NodeNG) -> tuple[int | None, Confidence]: + confidence = HIGH - def _get_start_value(self, node: nodes.NodeNG) -> Any: if isinstance(node, nodes.Name): inferred = utils.safe_infer(node) start_val = inferred.value if inferred else None + confidence = INFERENCE elif isinstance(node, nodes.UnaryOp): start_val = node.operand.value else: start_val = node.value - return start_val + + return start_val, confidence diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt index 74c6605f30..a38788cc89 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.txt @@ -4,5 +4,5 @@ unnecessary-list-index-lookup:46:10:46:22::Unnecessary list index lookup, use 'v unnecessary-list-index-lookup:74:10:74:27::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:112:10:112:21::Unnecessary list index lookup, use 'val' instead:HIGH unnecessary-list-index-lookup:115:10:115:21::Unnecessary list index lookup, use 'val' instead:HIGH -unnecessary-list-index-lookup:119:10:119:21::Unnecessary list index lookup, use 'val' instead:HIGH -unnecessary-list-index-lookup:122:10:122:21::Unnecessary list index lookup, use 'val' instead:HIGH +unnecessary-list-index-lookup:119:10:119:21::Unnecessary list index lookup, use 'val' instead:INFERENCE +unnecessary-list-index-lookup:122:10:122:21::Unnecessary list index lookup, use 'val' instead:INFERENCE From fe3302741f695f9123d33707ccec2f8b6b0cbe17 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Wed, 9 Nov 2022 20:07:50 -0300 Subject: [PATCH 6/7] add support for Call node --- pylint/checkers/refactoring/refactoring_checker.py | 4 ++-- .../u/unnecessary/unnecessary_list_index_lookup.py | 10 ++++++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index f87e9f60e1..674ebe19ab 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -21,7 +21,7 @@ from pylint import checkers from pylint.checkers import utils from pylint.checkers.utils import node_frame_class -from pylint.interfaces import Confidence, HIGH, INFERENCE +from pylint.interfaces import HIGH, INFERENCE, Confidence if TYPE_CHECKING: from pylint.lint import PyLinter @@ -2281,7 +2281,7 @@ def _enumerate_with_start( def _get_start_value(self, node: nodes.NodeNG) -> tuple[int | None, Confidence]: confidence = HIGH - if isinstance(node, nodes.Name): + if isinstance(node, (nodes.Name, nodes.Call)): inferred = utils.safe_infer(node) start_val = inferred.value if inferred else None confidence = INFERENCE diff --git a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py index fbf0597255..e5cb135148 100644 --- a/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py +++ b/tests/functional/u/unnecessary/unnecessary_list_index_lookup.py @@ -120,3 +120,13 @@ def process_list_again(data): for idx, val in enumerate(series, START): print(series[idx]) # [unnecessary-list-index-lookup] + +START = [1, 2, 3] +for i, k in enumerate(series, len(START)): + print(series[idx]) + +def return_start(start): + return start + +for i, k in enumerate(series, return_start(20)): + print(series[idx]) From bf59f13eb440bef9d7dcfed615fc0868bee674c6 Mon Sep 17 00:00:00 2001 From: clavedeluna Date: Sat, 12 Nov 2022 18:14:08 -0300 Subject: [PATCH 7/7] update file name --- doc/whatsnew/fragments/{7682.bugfix => 7682.false_positive} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename doc/whatsnew/fragments/{7682.bugfix => 7682.false_positive} (100%) diff --git a/doc/whatsnew/fragments/7682.bugfix b/doc/whatsnew/fragments/7682.false_positive similarity index 100% rename from doc/whatsnew/fragments/7682.bugfix rename to doc/whatsnew/fragments/7682.false_positive