Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress unnecessary-list-index-lookup for whole loop on write #6845

Merged
merged 8 commits into from Jun 11, 2022

Conversation

timmartin
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

The unnecessary-list-index-lookup checker was assuming that if the
subscript was written to then it would only invalidate access on later
lines, but this is not necessarily the case if an inner loop is nested
inside the one being checked.

Closes #6818

The ``unnecessary-list-index-lookup`` checker was assuming that if the
subscript was written to then it would only invalidate access on later
lines, but this is not necessarily the case if an inner loop is nested
inside the one being checked.

Fixes pylint-dev#6818
@timmartin
Copy link
Contributor Author

I was going to apply the same change to unnecessary-dict-index-lookup, but I discovered that we already have a regression test that would be regressed by this change:

for k, v in a_dict.items():
    print(a_dict[k])  # [unnecessary-dict-index-lookup]
    print(b_dict[k])  # Should not emit warning, accessing other dictionary
    a_dict[k] = 123  # Should not emit warning, key access necessary
    a_dict[k] += 123  # Should not emit warning, key access necessary
    print(a_dict[k])  # Should not emit warning, v != a_dict[k]****

We could just update the regression test, but I was going to have a go at changing the behavior to detect whether there is a loop of any kind nested within the outer for loop. I think if there is no inner loop we can stick with the old behavior and only suppress the warnings if there is a nested loop. This seems reasonable, but I'm worried that it will make the code hard to follow.

@coveralls
Copy link

coveralls commented Jun 5, 2022

Pull Request Test Coverage Report for Build 2478175748

  • 17 of 19 (89.47%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 95.519%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/checkers/refactoring/refactoring_checker.py 17 19 89.47%
Totals Coverage Status
Change from base Build 2478058828: -0.007%
Covered Lines: 16371
Relevant Lines: 17139

πŸ’› - Coveralls

@github-actions

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Jun 5, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.1 milestone Jun 5, 2022
@timmartin
Copy link
Contributor Author

I'm not sure I understand the build errors here. It seems to be a problem reported from a Javascript process, so it's not obvious how my changes could have affected it.

@Pierre-Sassoulas
Copy link
Member

I think it's due to other fails before:

************* Module pylint.checkers.refactoring.refactoring_checker
pylint/checkers/refactoring/refactoring_checker.py:2136:0: C0401: Wrong spelling of a word 'fo' in a comment:
# end fo the loop without updating the collection
      ^^
Did you mean: ''few' or 'of''? (wrong-spelling-in-comment)

Primer are costly, so if other job fails we cut them short I think.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit about typing. Change itself looks good to resolve the current issue!

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @timmartin !

@github-actions
Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit bd612cf

@Pierre-Sassoulas Pierre-Sassoulas merged commit deec94c into pylint-dev:main Jun 11, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 11, 2022
Pierre-Sassoulas pushed a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 15, 2022
…ylint-dev#6845)

The ``unnecessary-list-index-lookup`` checker was assuming that if the
subscript was written to then it would only invalidate access on later
lines, but this is not necessarily the case if an inner loop is nested
inside the one being checked.

Fixes pylint-dev#6818

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
Pierre-Sassoulas pushed a commit that referenced this pull request Jun 15, 2022
…6845)

The ``unnecessary-list-index-lookup`` checker was assuming that if the
subscript was written to then it would only invalidate access on later
lines, but this is not necessarily the case if an inner loop is nested
inside the one being checked.

Fixes #6818

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unnecessary-list-index-lookup doesn't detect list mutation in a nested while loop
5 participants