Extending redefined-outer-name check to cover the case of loop variables #8663

Lucas-C · 2023-05-05T14:44:06Z

Document your change, if it is a non-trivial one.
Keep the change small, separate the consensual changes from the opinionated one.

Type of Changes

	Type
✓	✨ New feature

Description

Extend the rule redefined-outer-name to cover the case of loop variables shadowing previously defined variables.

Closes #8646

Lucas-C · 2023-05-05T16:44:26Z

There is a case that I have trouble to handle / ignore :

#--- Notable cases where redefined-outer-name should NOT be raised:

# When a similarly named loop variable is assigned in another loop:
for n in range(10):
    n  = 1
for n in range(10):
    n  = 1

Would you have suggestions on how to NOT raise redefined-outer-name for this situation?

Lucas-C · 2023-05-05T17:30:09Z

There is a case that I have trouble to handle / ignore :
#--- Notable cases where redefined-outer-name should NOT be raised:

# When a similarly named loop variable is assigned in another loop:
for n in range(10):
    n  = 1
for n in range(10):
    n  = 1
Would you have suggestions on how to NOT raise redefined-outer-name for this situation?

Don't bother answering, I found a solution 😊

Now this is the case I'm wondering about:

for value in range(10):
    pass
value = 42

The current version of the checker will raise a redefined-outer-name for this code snippet.

is that OK?
if not, would you have any suggestions on how to distinguish it from the other valid cases of redefined-outer-name?

(edit: I'm working on a potential solution...)

Pierre-Sassoulas · 2023-05-06T16:36:56Z

Now this is the case I'm wondering about:
for value in range(10):
pass
value = 42
The current version of the checker will raise a redefined-outer-name for this code snippet.

It seems like it's more of a redefined-inner-name (or predefined-outer-name) in this case 😄 Potentially problematic but does not feel like the same issue.

Lucas-C · 2023-05-09T07:18:04Z

I think this PR is ready for review! 😊

Lucas-C · 2023-05-09T07:29:29Z

The mypy check from pre-commit signals a problem that I do not understand:

pylint/checkers/variables.py:3395: error: Returning Any from function declared to return "bool"  [no-any-return]

Related code (line 3395 in the number comparison):

    for node in assign.node_ancestors():
        if node is for_node.parent:
            return assign.lineno > for_node.lineno
    return False

codecov · 2023-05-09T07:31:44Z

Codecov Report

Merging #8663 (b2aaaa1) into main (1427461) will decrease coverage by 0.01%.
The diff coverage is 95.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8663       /-   ##
==========================================
- Coverage   95.76%   95.76%   -0.01%     
==========================================
  Files         173      173              
  Lines       18616    18663       47     
==========================================
  Hits        17827    17872       45     
- Misses        789      791        2

Files Changed	Coverage Δ
pylint/testutils/_primer/primer_compare_command.py	`95.55% <50.00%> (-1.04%)`	⬇️
pylint/checkers/variables.py	`97.20% <97.82%> ( 0.01%)`	⬆️

Pierre-Sassoulas · 2023-05-09T10:29:24Z

astroid is not officially typed so assign.lineno and for_node.lineno are considered an Any (not an int like they should). That being said, it feels like a mypy bug because > returns a boolean.

Pierre-Sassoulas · 2023-05-09T10:40:28Z

python/mypy#5697 (comment)

Pierre-Sassoulas · 2023-05-09T10:41:58Z

It look like I already linked to this mypy issue , one year and one day ago. Guess I never learn.

Lucas-C · 2023-05-09T12:29:32Z

python/mypy#5697 (comment)

Interesting.

What do you recommend in that case? Adding # type: ignore on this line?

Pierre-Sassoulas · 2023-05-09T12:32:05Z

# type: ignore

Yeah, # type: ignore[no-any-return], then we're going to remove it once astroid is typed officially, a lot of effort is made in this direction.

Lucas-C · 2023-05-09T12:44:58Z

Yeah, # type: ignore[no-any-return], then we're going to remove it once astroid is typed officially, a lot of effort is made in this direction.

OK, done

Lucas-C · 2023-05-09T12:55:20Z

For reference, with this change Pylint now raises several extra issues on its own code:

************* Module pylint.pyreverse.mermaidjs_printer
pylint/pyreverse/mermaidjs_printer.py:62:8: W0621: Redefining name 'line' from outer scope (line 54) (redefined-outer-name)
************* Module pylint.checkers.typecheck
pylint/checkers/typecheck.py:1166:12: W0621: Redefining name 'name' from outer scope (line 1097) (redefined-outer-name)
pylint/checkers/typecheck.py:1599:12: W0621: Redefining name 'i' from outer scope (line 1552) (redefined-outer-name)
pylint/checkers/typecheck.py:1620:8: W0621: Redefining name 'name' from outer scope (line 1506) (redefined-outer-name)
************* Module pylint.checkers.exceptions
pylint/checkers/exceptions.py:52:4: W0621: Redefining name 'inferred' from outer scope (line 48) (redefined-outer-name)
************* Module pylint.testutils.functional.find_functional_tests
pylint/testutils/functional/find_functional_tests.py:100:12: W0621: Redefining name 'error_msg' from outer scope (line 83) (redefined-outer-name)
pylint/testutils/functional/find_functional_tests.py:128:8: W0621: Redefining name 'possible_dir' from outer scope (line 114) (redefined-outer-name)
************* Module pylint.checkers.format
pylint/checkers/format.py:524:8: W0621: Redefining name 'line' from outer scope (line 511) (redefined-outer-name)
************* Module pylint.pyreverse.inspector
pylint/pyreverse/inspector.py:367:12: W0621: Redefining name 'fpath' from outer scope (line 353) (redefined-outer-name)
************* Module pylint.checkers.classes.class_checker
pylint/checkers/classes/class_checker.py:90:4: W0621: Redefining name 'arg' from outer scope (line 80) (redefined-outer-name)
pylint/checkers/classes/class_checker.py:1828:8: W0621: Redefining name 'callee' from outer scope (line 1825) (redefined-outer-name)
pylint/checkers/classes/class_checker.py:2165:8: W0621: Redefining name 'method' from outer scope (line 2155) (redefined-outer-name)
************* Module pylint.checkers.strings
pylint/checkers/strings.py:316:20: W0621: Redefining name 'key' from outer scope (line 302) (redefined-outer-name)
pylint/checkers/strings.py:321:16: W0621: Redefining name 'key' from outer scope (line 302) (redefined-outer-name)
pylint/checkers/strings.py:326:16: W0621: Redefining name 'key' from outer scope (line 302) (redefined-outer-name)
pylint/checkers/strings.py:386:16: W0621: Redefining name 'format_type' from outer scope (line 329) (redefined-outer-name)
************* Module pylint.checkers.imports
pylint/checkers/imports.py:99:4: W0621: Redefining name 'first' from outer scope (line 95) (redefined-outer-name)
************* Module unittest_multi_naming_style
tests/checkers/base/unittest_multi_naming_style.py:49:12: W0621: Redefining name 'cls' from outer scope (line 48) (redefined-outer-name)
tests/checkers/base/unittest_multi_naming_style.py:98:12: W0621: Redefining name 'cls' from outer scope (line 97) (redefined-outer-name)
tests/checkers/base/unittest_multi_naming_style.py:136:12: W0621: Redefining name 'func' from outer scope (line 135) (redefined-outer-name)
tests/checkers/base/unittest_multi_naming_style.py:173:12: W0621: Redefining name 'func' from outer scope (line 172) (redefined-outer-name)
************* Module pylint.checkers.similar
pylint/checkers/similar.py:439:8: W0621: Redefining name 'num' from outer scope (line 413) (redefined-outer-name)
pylint/checkers/similar.py:464:12: W0621: Redefining name 'line_set' from outer scope (line 463) (redefined-outer-name)
pylint/checkers/similar.py:464:12: W0621: Redefining name 'start_line' from outer scope (line 463) (redefined-outer-name)
pylint/checkers/similar.py:464:12: W0621: Redefining name 'end_line' from outer scope (line 463) (redefined-outer-name)
pylint/checkers/similar.py:871:12: W0621: Redefining name 'lineset' from outer scope (line 870) (redefined-outer-name)
pylint/checkers/similar.py:871:12: W0621: Redefining name 'start_line' from outer scope (line 870) (redefined-outer-name)
pylint/checkers/similar.py:871:12: W0621: Redefining name 'end_line' from outer scope (line 870) (redefined-outer-name)
************* Module pylint.pyreverse.plantuml_printer
pylint/pyreverse/plantuml_printer.py:78:8: W0621: Redefining name 'line' from outer scope (line 68) (redefined-outer-name)
************* Module pylint.checkers.refactoring.refactoring_checker
pylint/checkers/refactoring/refactoring_checker.py:1576:8: W0621: Redefining name 'value' from outer scope (line 1566) (redefined-outer-name)

I can do the job of fixing all those warnings,
but could I have some approval of the overral relevance of this PR please?

Basically, I don't want to spend a few more hours on this PR to then discover that it is not really welcome,
for example because it triggers to many new warnings 😅

Pierre-Sassoulas · 2023-05-09T13:03:33Z

Basically, I don't want to spend a few more hours on this PR to then discover that it is not really welcome,
for example because it triggers to many new warnings

Yes, it seems like a risk. I think we need the pylint check to be green in order to see the primer result, could you add a disable for redefined-outer-name in our pylintrc so the pylint test is green and we can see the result of the primer on more packages please ?

Lucas-C · 2023-05-09T13:12:51Z

could you add a disable for redefined-outer-name in our pylintrc so the pylint test is green and we can see the result of the primer on more packages please ?

Sure, done

Lucas-C · 2023-05-12T09:24:18Z

Hi
Just a suggestion: as this PR is waiting for feedback, maybe it should be tagged Needs decision or Needs review?

Pierre-Sassoulas · 2023-05-12T10:38:35Z

Hey @Lucas-C could you rebase on main ? There was an issue in the primer that was fixed in #8676

for more information, see https://pre-commit.ci

…Pierre-Sassoulas

…Pierre-Sassoulas

Lucas-C · 2023-05-12T10:40:59Z

Hey @Lucas-C could you rebase on main ? There was an issue in the primer that was fixed in #8676

Sure, done

We now need to use repr_name.

…-loop-variables

jacobtylerwalls

Thanks for your patience with the review. I pushed some temporary changes that should get the primer to finish. We're generating so many messages (in general, but also with this PR in particular) that the job could never finish. Obviously this is a symptom of needing to parallelize our testing tool, but it's also a symptom that a lot of messages will be emitted.

I'll circle back when the primer tool posts its comment. In the meantime, found one bug ⬇️

jacobtylerwalls · 2023-05-15T18:18:56Z

pylint/checkers/variables.py

+ for target in targets:
+ if isinstance(target, nodes.Starred):
+ target = target.value
+ for ref in node.scope().locals.get(target.name, []):


name might not exist. See test case:

class A: def go(self): for self.current in range(5): pass

Gives:

File "/Users/.../pylint/pylint/checkers/variables.py", line 1306, in visit_for for ref in node.scope().locals.get(target.name, []): ^^^^^^^^^^^ AttributeError: 'AssignAttr' object has no attribute 'name'

You could use repr_name() (new in astroid 3.0 alpha, so you'll want to rebase on pylint-dev@upgrade-astroid) EDIT: my pushes did that for you! (but you can remove the bleeding-edge commit)

Pierre-Sassoulas · 2023-05-15T20:48:31Z

If there's so much messages that it's crashing the primer we probably can't just extend an existing message, we'd need a new one (if we even want to add the message, because if a construct is used extensively in popular project it's probably not a code smell ?). There's a lot of violations in pylint's as we saw previously.

jacobtylerwalls · 2023-05-15T21:19:49Z

I can just stop fiddling with the primer and run it manually. Here are the five results from linting astroid:

% pylint astroid --disable=all --enable=redefined-outer-name
************* Module astroid.builder
astroid/builder.py:342:8: W0621: Redefining name 'child' from outer scope (line 333) (redefined-outer-name)
************* Module astroid.inference
astroid/inference.py:542:8: W0621: Redefining name 'value' from outer scope (line 541) (redefined-outer-name)
************* Module astroid.nodes.node_classes
astroid/nodes/node_classes.py:107:4: W0621: Redefining name 'inferred' from outer scope (line 102) (redefined-outer-name)
************* Module astroid.brain.brain_gi
astroid/brain/brain_gi.py:140:4: W0621: Redefining name 'obj' from outer scope (line 75) (redefined-outer-name)
************* Module astroid.brain.brain_multiprocessing
astroid/brain/brain_multiprocessing.py:37:4: W0621: Redefining name 'node' from outer scope (line 24) (redefined-outer-name)

1
I'd consider a false positive, since the two uses are in exclusive branches. Try:

diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index 55fce29d3..ca73a5182 100644
--- a/pylint/checkers/variables.py
    b/pylint/checkers/variables.py
@@ -1306,6  1306,8 @@ class VariablesChecker(BaseChecker):
             for ref in node.scope().locals.get(target.name, []):
                 if ref == target:
                     continue
                 if astroid.nodes.node_classes.are_exclusive(ref, target):  # with a better import...
                     continue
                 if isinstance(ref.parent, nodes.Arguments):
                     continue  # ignoring case covered by redefined-argument-from-local
                 if self._dummy_rgx and self._dummy_rgx.match(target.name):

2-5
I'd consider false positive-ish: they all follow a return or a yield. I'd suggest checking for return/yield anywhere between ref and target and short-circuit.

It's possible that would get the number of new messages down to an acceptable level, but I can't guarantee it until seeing the results (sorry!)

This reverts commit ff5488f.

…bles

github-actions · 2023-09-01T02:07:39Z

This PR needs take over because because it has been open 8 weeks with no activity.

…bles

github-actions · 2023-10-28T02:01:48Z

This PR needs take over because because it has been open 8 weeks with no activity.

jacobtylerwalls · 2024-01-24T15:27:46Z

Hi @Lucas-C 👋
Do you think you will have time to reengage here?

jacobtylerwalls · 2024-02-11T13:53:59Z

Feel free to reopen if you return to this 👍

Lucas-C mentioned this pull request May 5, 2023

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

Open

Lucas-C force-pushed the extending-redefined-outer-name-to-loop-variables branch 2 times, most recently from 3a5dc28 to a1ce31f Compare May 5, 2023 15:05

Pierre-Sassoulas added Work in progress False Negative 🦋 No message is emitted but something is wrong with the code labels May 6, 2023

Lucas-C marked this pull request as ready for review May 9, 2023 07:17

Pierre-Sassoulas added Needs review 🔍 Needs to be reviewed by one or multiple more persons and removed Work in progress labels May 12, 2023

Lucas-C and others added 5 commits May 12, 2023 12:40

Extending redefined-outer-name check to cover the case of loop variables

1409f42

[pre-commit.ci] auto fixes from pre-commit.com hooks

11fa067

for more information, see https://pre-commit.ci

Adding documentation

2f6aef8

[pre-commit.ci] auto fixes from pre-commit.com hooks

a10bad0

for more information, see https://pre-commit.ci

Work in progress

d75a981

Lucas-C added 2 commits May 12, 2023 12:40

Pleasing mypy

c3e5ade

Disabling redefined-outer-name for Pylint temporarily as required by @…

30e74e2

…Pierre-Sassoulas

Lucas-C force-pushed the extending-redefined-outer-name-to-loop-variables branch from 2a961c2 to 30e74e2 Compare May 12, 2023 10:40

Pierre-Sassoulas and others added 7 commits May 14, 2023 19:58

Upgrade astroid to 3.0.0a3

a3092a1

We now need to use repr_name.

Ignore more stdlib classes with complex ancestry

c831559

Add dummy args to empty Dict instantiation

11bb684

Merge branch 'upgrade-astroid' into extending-redefined-outer-name-to…

75e7954

…-loop-variables

Use bleeding-edge astroid

ff5488f

[temporary] Skip home-assistant

cb3f914

[primer] Don't cross-reference missing package

e733253

jacobtylerwalls requested changes May 15, 2023

View reviewed changes

[temporary] Check out this branch in comment flow

17aea9f

jacobtylerwalls mentioned this pull request May 17, 2023

[primer] Parallelize primer workflow into package batches #8697

Merged

Pierre-Sassoulas added Waiting on author Indicate that maintainers are waiting for a message of the author and removed Needs review 🔍 Needs to be reviewed by one or multiple more persons labels May 20, 2023

jacobtylerwalls added 2 commits July 6, 2023 09:37

Revert "Use bleeding-edge astroid"

3edb200

This reverts commit ff5488f.

Merge branch 'main' into extending-redefined-outer-name-to-loop-varia…

41c6d83

…bles

github-actions bot added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Sep 1, 2023

Merge branch 'main' into extending-redefined-outer-name-to-loop-varia…

b2aaaa1

…bles

github-actions bot removed the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Sep 2, 2023

github-actions bot added the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Oct 28, 2023

github-actions bot removed the Needs take over 🛎️ Orignal implementer went away but the code could be salvaged. label Jan 25, 2024

jacobtylerwalls closed this Feb 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending redefined-outer-name check to cover the case of loop variables #8663

Extending redefined-outer-name check to cover the case of loop variables #8663

Lucas-C commented May 5, 2023 •

edited

Loading

Lucas-C commented May 5, 2023 •

edited

Loading

Lucas-C commented May 5, 2023 •

edited

Loading

Pierre-Sassoulas commented May 6, 2023 •

edited

Loading

Lucas-C commented May 9, 2023

Lucas-C commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

Pierre-Sassoulas commented May 9, 2023

Pierre-Sassoulas commented May 9, 2023

Pierre-Sassoulas commented May 9, 2023

Lucas-C commented May 9, 2023

Pierre-Sassoulas commented May 9, 2023 •

edited

Loading

Lucas-C commented May 9, 2023

Lucas-C commented May 9, 2023 •

edited

Loading

Pierre-Sassoulas commented May 9, 2023

Lucas-C commented May 9, 2023

Lucas-C commented May 12, 2023

Pierre-Sassoulas commented May 12, 2023

Lucas-C commented May 12, 2023

jacobtylerwalls left a comment

jacobtylerwalls May 15, 2023 •

edited

Loading

Pierre-Sassoulas commented May 15, 2023

jacobtylerwalls commented May 15, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Oct 28, 2023

jacobtylerwalls commented Jan 24, 2024

jacobtylerwalls commented Feb 11, 2024

Extending redefined-outer-name check to cover the case of loop variables #8663

Extending redefined-outer-name check to cover the case of loop variables #8663

Conversation

Lucas-C commented May 5, 2023 • edited Loading

Type of Changes

Description

Lucas-C commented May 5, 2023 • edited Loading

Lucas-C commented May 5, 2023 • edited Loading

Pierre-Sassoulas commented May 6, 2023 • edited Loading

Lucas-C commented May 9, 2023

Lucas-C commented May 9, 2023 • edited Loading

codecov bot commented May 9, 2023 • edited Loading

Codecov Report

Pierre-Sassoulas commented May 9, 2023

Pierre-Sassoulas commented May 9, 2023

Pierre-Sassoulas commented May 9, 2023

Lucas-C commented May 9, 2023

Pierre-Sassoulas commented May 9, 2023 • edited Loading

Lucas-C commented May 9, 2023

Lucas-C commented May 9, 2023 • edited Loading

Pierre-Sassoulas commented May 9, 2023

Lucas-C commented May 9, 2023

Lucas-C commented May 12, 2023

Pierre-Sassoulas commented May 12, 2023

Lucas-C commented May 12, 2023

jacobtylerwalls left a comment

Choose a reason for hiding this comment

jacobtylerwalls May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Pierre-Sassoulas commented May 15, 2023

jacobtylerwalls commented May 15, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Oct 28, 2023

jacobtylerwalls commented Jan 24, 2024

jacobtylerwalls commented Feb 11, 2024

Lucas-C commented May 5, 2023 •

edited

Loading

Lucas-C commented May 5, 2023 •

edited

Loading

Lucas-C commented May 5, 2023 •

edited

Loading

Pierre-Sassoulas commented May 6, 2023 •

edited

Loading

Lucas-C commented May 9, 2023 •

edited

Loading

codecov bot commented May 9, 2023 •

edited

Loading

Pierre-Sassoulas commented May 9, 2023 •

edited

Loading

Lucas-C commented May 9, 2023 •

edited

Loading

jacobtylerwalls May 15, 2023 •

edited

Loading