false positive `unnecessary-list-index-lookup` for enumerate #7685

clavedeluna · 2022-10-27T16:13:45Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

Pylint will now not emit a unnecessary-list-index-lookup warning if the loop / comprehension is iterating over enumerate called with a start arg/kwarg.

Closes #7682

coveralls · 2022-10-27T16:22:18Z

Pull Request Test Coverage Report for Build 3452558221

27 of 29 (93.1%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.004%) to 95.378%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pylint/checkers/refactoring/refactoring_checker.py	27	29	93.1%

Totals
Change from base Build 3452308194:	-0.004%
Covered Lines:	17315
Relevant Lines:	18154

💛 - Coveralls

pylint/checkers/refactoring/refactoring_checker.py

Pierre-Sassoulas

Thank you for the fix @clavedeluna. I think we can handle the false negative with start=0 too.

Pierre-Sassoulas · 2022-10-28T06:53:46Z

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.


Suggested change

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

Pierre-Sassoulas · 2022-10-28T06:55:14Z

tests/functional/u/unnecessary/unnecessary_list_index_lookup.py

+for idx, val in enumerate(series, start=2):
+    print(my_list[idx])


Suggested change

for idx, val in enumerate(series, start=2):

print(my_list[idx])

for idx, val in enumerate(series, start=2):

print(my_list[idx])

for idx, val in enumerate(series, 2):

print(my_list[idx])

for idx, val in enumerate(series, start=0):

print(my_list[idx]) # [unnecessary-list-index-lookup]

for idx, val in enumerate(series, 0):

print(my_list[idx]) # [unnecessary-list-index-lookup]

Pierre-Sassoulas · 2022-10-28T06:56:58Z

pylint/checkers/refactoring/refactoring_checker.py

+        `enumerate([1,2,3], 1)`
+        """
+        if len(node.iter.args) > 1:
+            # We assume the second argument to `enumerate` is the `start` arg.


Suggested change

# We assume the second argument to `enumerate` is the `start` arg.

# We assume the second argument to `enumerate` is the `start` arg.

# It's a reasonable assumption for now as it's the only possible argument:

# https://docs.python.org/3/library/functions.html#enumerate

clavedeluna · 2022-10-28T12:43:19Z

pylint/checkers/refactoring/refactoring_checker.py

+
+                return not start_val == 0
+
+        return False


I actually tried to refactor this method because it looks like a bunch of duplicate code, but:

refactoring wouldn't allow for some short-circuting which seems best for performance

args v. kwargs has sliiightly different needs, .value.operand v. .operand etc

clavedeluna · 2022-10-28T12:43:52Z

Thank you for the fix @clavedeluna. I think we can handle the false negative with start=0 too.

good call, I also went ahead and added calls for negative start value which made the logic more robust albeit a bit more complicated

pylint/checkers/refactoring/refactoring_checker.py

Pierre-Sassoulas

Great ! 👍

Pierre-Sassoulas · 2022-10-28T20:10:43Z

The failing test on python 3.11 seems genuine

pylint/checkers/refactoring/refactoring_checker.py

Pierre-Sassoulas

LGTM, thank you !

clavedeluna · 2022-11-03T12:43:25Z

primer doesn't wanna prime 😞

clavedeluna · 2022-11-09T21:17:44Z

@DanielNoord would you be able to provide a bit more info on the exception? was there a stack trace? Or maybe something I could minimally reproduce and add a test for? Great catch! No wonder my other PRs have no issues with the primer

DanielNoord · 2022-11-09T21:19:33Z

@DanielNoord would you be able to provide a bit more info on the exception? was there a stack trace? Or maybe something I could minimally reproduce and add a test for? Great catch! No wonder my other PRs have no issues with the primer

It's actually quite easy to reproduce this yourself locally. Just run:

python tests/primer/__main__.py prepare --clone
python tests/primer/__main__.py run --type=pr

It will take some time to get to pandas but the exception will be there.
You can also look at the pandas-dev/pandas/pandas/core/indexes/multi.py file as that is where the issue seems to be.

clavedeluna · 2022-11-09T22:47:11Z

glad I opened #7714

clavedeluna · 2022-11-09T23:09:34Z

Thanks @DanielNoord for getting me to finally run the primer locally. I must've done too many of the CI steps and got stuck initially. The error was easy to spot and fix, so I hope everything passes now!

clavedeluna · 2022-11-10T10:48:54Z

😢 primer, I don't think it has anything to do with this PR

Pierre-Sassoulas · 2022-11-10T11:17:14Z

A case study for #7738

Pierre-Sassoulas

Small nitpick, it's ready to go otherwise, nice fix @clavedeluna 🎉

doc/whatsnew/fragments/7682.bugfix

github-actions · 2022-11-12T21:55:16Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit bf59f13

* do not report unnecessary list index lookup if start arg is passed * account for calling start with 0 or negative num Co-authored-by: Pierre Sassoulas <[email protected]>

This comment has been minimized.

Sign in to view

jacobtylerwalls self-requested a review October 27, 2022 23:18

jacobtylerwalls reviewed Oct 28, 2022

View reviewed changes

pylint/checkers/refactoring/refactoring_checker.py Outdated Show resolved Hide resolved

Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Oct 28, 2022

Pierre-Sassoulas added this to the 2.15.6 milestone Oct 28, 2022

Pierre-Sassoulas requested changes Oct 28, 2022

View reviewed changes

clavedeluna force-pushed the 7682-enumerate branch from f9ba40a to d6d72e2 Compare October 28, 2022 12:41

clavedeluna commented Oct 28, 2022

View reviewed changes

clavedeluna requested review from Pierre-Sassoulas and jacobtylerwalls and removed request for Pierre-Sassoulas and jacobtylerwalls October 28, 2022 12:44

Pierre-Sassoulas reviewed Oct 28, 2022

View reviewed changes

pylint/checkers/refactoring/refactoring_checker.py Outdated Show resolved Hide resolved

clavedeluna requested a review from Pierre-Sassoulas October 28, 2022 19:47

Pierre-Sassoulas previously approved these changes Oct 28, 2022

View reviewed changes

clavedeluna dismissed Pierre-Sassoulas’s stale review via 2a97187 October 31, 2022 13:19

clavedeluna requested a review from Pierre-Sassoulas October 31, 2022 15:31

Pierre-Sassoulas requested changes Oct 31, 2022

View reviewed changes

pylint/checkers/refactoring/refactoring_checker.py Show resolved Hide resolved

clavedeluna requested a review from Pierre-Sassoulas October 31, 2022 20:04

Pierre-Sassoulas previously approved these changes Oct 31, 2022

View reviewed changes

Pierre-Sassoulas dismissed their stale review via 09c3ee9 October 31, 2022 21:20

Pierre-Sassoulas force-pushed the 7682-enumerate branch from 262105f to 09c3ee9 Compare October 31, 2022 21:20

clavedeluna requested a review from Pierre-Sassoulas November 2, 2022 12:01

clavedeluna force-pushed the 7682-enumerate branch from 09c3ee9 to 40edf3d Compare November 3, 2022 11:26

Pierre-Sassoulas mentioned this pull request Nov 9, 2022

Better primer tests : more reliable, easier to understand, and more stable #7738

Open

6 tasks

clavedeluna mentioned this pull request Nov 9, 2022

Update Testing Docs regarding Primer #7714

Closed

clavedeluna dismissed Pierre-Sassoulas’s stale review via c9b98e4 November 9, 2022 23:08

clavedeluna force-pushed the 7682-enumerate branch from 92c3d3e to c9b98e4 Compare November 9, 2022 23:08

clavedeluna added 6 commits November 12, 2022 16:56

do not report unnecessary list index lookup if start arg is passed

e0b2b9c

refactor

809f39d

account for calling start with 0 or negative num

9cb990b

test for start as a var

ba5467b

use inference confidence when appropriate

fd93b4b

add support for Call node

fe33027

clavedeluna force-pushed the 7682-enumerate branch from c9b98e4 to fe33027 Compare November 12, 2022 19:56

jacobtylerwalls removed the Blocked 🚧 Blocked by a particular issue label Nov 12, 2022

clavedeluna requested a review from Pierre-Sassoulas November 12, 2022 20:44

This comment has been minimized.

Sign in to view

Pierre-Sassoulas requested changes Nov 12, 2022

View reviewed changes

doc/whatsnew/fragments/7682.bugfix Show resolved Hide resolved

update file name

bf59f13

clavedeluna requested a review from Pierre-Sassoulas November 13, 2022 11:53

Pierre-Sassoulas approved these changes Nov 13, 2022

View reviewed changes

Pierre-Sassoulas merged commit 131b315 into pylint-dev:main Nov 13, 2022

Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Nov 16, 2022

		@@ -0,0 +1,3 @@
		``unnecessary-list-index-lookup`` will not be emitted if ``enumerate`` is called with a `start` arg or kwarg.

		for idx, val in enumerate(series, start=2):
		print(my_list[idx])

Uh oh!

false positive unnecessary-list-index-lookup for enumerate #7685

false positive unnecessary-list-index-lookup for enumerate #7685

Uh oh!

Conversation

clavedeluna commented Oct 27, 2022

Type of Changes

Description

Uh oh!

coveralls commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3452558221

💛 - Coveralls

Uh oh!

This comment has been minimized.

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

clavedeluna Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

clavedeluna commented Oct 28, 2022

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas commented Oct 28, 2022

Uh oh!

Uh oh!

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

clavedeluna commented Nov 3, 2022

Uh oh!

clavedeluna commented Nov 9, 2022

Uh oh!

DanielNoord commented Nov 9, 2022

Uh oh!

clavedeluna commented Nov 9, 2022

Uh oh!

clavedeluna commented Nov 9, 2022

Uh oh!

clavedeluna commented Nov 10, 2022

Uh oh!

Pierre-Sassoulas commented Nov 10, 2022

Uh oh!

This comment has been minimized.

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

false positive `unnecessary-list-index-lookup` for enumerate #7685

false positive `unnecessary-list-index-lookup` for enumerate #7685

coveralls commented Oct 27, 2022 •

edited

Loading