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.

Sign up for GitHub

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

Jump to bottom

Suggestion to improve redefined-outer-name or to introduce a new rule #8646

Open
Lucas-C opened this issue May 1, 2023 · 6 comments
Open

Suggestion to improve redefined-outer-name or to introduce a new rule #8646

Lucas-C opened this issue May 1, 2023 · 6 comments
Labels
False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection
Milestone
4.0.0

Comments

@Lucas-C
Copy link
Contributor

Lucas-C commented May 1, 2023 •
edited
Loading

Current problem

The following code may not print what you expect on the last line:

from collections import defaultdict
errors = [
    Exception("E101", "Boom!"),
    Exception("E101", "Bam!"),
    Exception("E102", "Shazam!"),
]
errors_per_arg0 = defaultdict(list)
for error in errors:
    errors_per_arg0[error.args[0]].append(error)
for arg0, errors in errors_per_arg0.items():
    print(arg0, "count:", len(errors))
print(errors)

The issue comes from the for loop, where the errors loop variable shadows the previously defined list.

The unexpected problem is that, on the last line, errors is now a 1-item list!
There is this script output:

E101 count: 2
E102 count: 1
[Exception('E102', 'Shazam!')]

As far as I know, Pylint currently does not have any rule that detects this (tested using Pylint 2.17.3 & 3.0.0a6).
redefined-outer-name for example, does not get triggered.

Desired solution

First, I'd like to know what Pylint maintainers think to be the best approach:

  • improve the existing redefined-outer-name to cover this
  • introduce a new rule to detect this case

Additional context

There are some edge cases to consider when such case happens, where the definition of loop variable "erases" a variable in the current scope:

  • if this variable is not used after the loop, should an error be raised?
  • if this variable is re-assigned after the loop, before being read, should an error be raised?
@Lucas-C Lucas-C added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 1, 2023
@Lucas-C Lucas-C changed the title Suggetion to improve redefined-outer-name or to introduce a new rule Suggestion to improve redefined-outer-name or to introduce a new rule May 1, 2023
@Lucas-C
Copy link
Contributor Author

Lucas-C commented May 1, 2023 •
edited
Loading

Related issues & PRs:

Note that pylint --load-plugins=pylint.extensions.redefined_loop_name ./issue_repro.py does not raise any error on the code sample provided in this issue description (tested using Pylint 2.17.3 & 3.0.0a6).

@Pierre-Sassoulas Pierre-Sassoulas added False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 1, 2023
@jacobtylerwalls
Copy link
Member

This one's tough! We'd need to distinguish this from the general case where reassigning a variable (for clarity?) is perfectly fine.

x = apple or banana
x = x or candy

Do you have an idea about what would distinguish these cases?

@jacobtylerwalls
Copy link
Member

Wasn't meaning to be dense. I take it the idea is that defining loop variables are special. Just wondering your thoughts.

@Lucas-C Lucas-C mentioned this issue May 5, 2023
Closed
2 tasks
@Lucas-C
Copy link
Contributor Author

Lucas-C commented May 5, 2023 •
edited
Loading

Wasn't meaning to be dense. I take it the idea is that defining loop variables are special. Just wondering your thoughts.

I opened #8663 as a starting point to explore how this could be implemented.

What do you think of the approach taken there?

@jacobtylerwalls
Copy link
Member

I like it! Sorry for the delay in reviewing, we'd really like to get a report from the primer tool, and I haven't debugged why it's not working on your branch. I can fire it up locally on just a subset of packages if need be.

@Lucas-C
Copy link
Contributor Author

Lucas-C commented May 15, 2023

I like it! Sorry for the delay in reviewing, we'd really like to get a report from the primer tool, and I haven't debugged why it's not working on your branch. I can fire it up locally on just a subset of packages if need be.

Thank you for the feedback 😊
Alright, no worry.
Can I do anything to help?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 4.0.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Milestone
4.0.0
Development

Successfully merging a pull request may close this issue.

Extending redefined-outer-name check to cover the case of loop variables Lucas-C/pylint
3 participants
@Lucas-C @Pierre-Sassoulas @jacobtylerwalls