redefined-outer-name emitted for variables in the same namespace

[superbobry]

Bug description

pylint emits redefined-outer-name for loop variables defined in nested for statements. For example,

for y in range(10):
    for y in range(10):
        ...

Technically, both definitions of y are in the same namespace, and while this code does look fishy, the redefined-outer-name message seems irrelevant. The message is not emitted for

x = 42
x = 24

nor

for z in range(10):
    ...

for z in range(10):
    ...

Command used

python3 -m pylint /tmp/foo.py

Pylint output

************* Module foo
/tmp/foo.py:7:4: W0621: Redefining name 'y' from outer scope (line 6) (redefined-outer-name)

Expected behavior

redefined-outer-name is not emitted.

Pylint version

pylint 2.8.3
astroid 2.5.6
Python 3.9.9 (main, Nov 16 2021, 10:24:31) 
[GCC 11.2.0]

[jacobtylerwalls]

The message is not emitted for ...

Hi there @superbobry, I think your contrasting examples articulate quite nicely what redefined-outer-name is looking for. Your contrasting examples would be something like redefined-name, not redefined-outer-name. Does that make sense why redefined-outer-name does what it does? It's specifically looking for nested redefinitions.

[superbobry]

Thanks for the clarification @jacobtylerwalls.

I think I understand why redefined-outer-name is emitted there. It would probably be better to emit something like redefined-name instead, but my point is slightly different. I find the fact that special-casing is for loops only confusing. For example, nothing is emitted for

i = 42
for i in range(...):
    ...

nor for

for i in range(...):
    i = 42

even though technically the loop variable redefines a preceding name in the first snippet and is itself redefined in the loop body in the second one.

Is it worth having that special-case at all in your opinion?

[DanielNoord]

Just my two cents:

Your first example seems like a false positive. redefined-outer-name serves to remind you of "outer-names" you might have forgotten about and I think this is an example of such a case.

The second example should probably be another (new) message like redefined-loop-name? Creating a new message would also allow disabling or enabling it specifically.

[jacobtylerwalls]

Your first example seems like a false positive. redefined-outer-name serves to remind you of "outer-names" you might have forgotten about and I think this is an example of such a case.

You mean false negative, right? As in, we should emit redefined-outer-name? :-)

[DanielNoord]

@jacobtylerwalls Ah yes!

[superbobry]

Note that to really get the first example right pylint would likely have to do proper reachability analysis (and I really think it shouldn't tbh). E.g. imagine i = 42 happens inside control flow

for x in range(...):
  if x > 0:
      i = 42
for i in range(...):
    ...

[jacobtylerwalls]

Ah, but in that example, i is no longer an outer name, no? I think there there should be no message, because i is a variable defined in a loop that when the loop exits, we haven't bothered to delete (idiomatic Python).

[superbobry]

I guess that would depend on what "outer" means. The help string for redefined-outer-name refers to "outer scope"

https://github.com/PyCQA/pylint/blob/532be9afa75bd090a389db0f172c47ba9d37b411/pylint/checkers/variables.py#L488-L490

in which case redefined-outer-name is probably not applicable to any of the examples in this issue, since all variable definitions occur in the same namespace.

[DanielNoord]

Wouldn't it be better if redefined-outer-name would warn on a loop variable names that shadow a previously declared variable? Taking the latest example and making it a bit more realistic:

def func(coords):
    x_coords, y_coords = [], []
    for x, y in coords:
        x_coords.append(x)
        y_coords.append(y)
        # Check a condition and set future return values
        if x > y:
            x_coord, y_coord = x, y

    # Some additional code that makes you forget about x_coord
    ...

    # For loop that shadows one of the return values
    for x_coord in x_coords:
        print(x_coord)
    return x_coord, y_coord

This is still very made-up and doesn't necessarily reflect a real world scenario but I think this is a potential bug that pylint could/should warn about.

[superbobry]

I have no specific examples to back this up, but my gut feeling is that the false positive rate of such a check would be too high for it to be useful.

[DanielNoord]

Could you give an example of a potential false positive? I mean, if x_coord is defined somewhere before being declared as the loop variable this isn't a false positive. That should be fairly trivial to check.

[jacobtylerwalls]

After reading the discussion on #5146, I think that the first example in #5608 (comment) (and which @DanielNoord elaborated on) is valid usage, and should not emit a message (so pylint's current behavior is right.) This is the correct way to guard against the iterable being empty if the loop variable is accessed afterward:

x_coord = 0
for x_coord in maybe_empty:
    x_coord = random.random()
print(x_coord)

The second example should probably be another (new) message like redefined-loop-name? Creating a new message would also allow disabling or enabling it specifically.

I agree that should warn, but I don't see much of a difference between redefined-loop-name and redefined-outer-name, so I would just argue for extending redefined-outer-name. Granting the ability to disable things specifically has to be weighed against confusing folks trying to understand why checkers have special cases (which was @superbobry's original question):

for y in range(...):
    for y in range(...):  # [already emits redefined-outer-name]
        ...

for y in range(...):
    y = None  # [I think this should also emit redefined-outer-name]

Both have the effect of overwriting/defeating the first y.

I guess that would depend on what "outer" means. The help string for redefined-outer-name refers to "outer scope"

I agree this is worded poorly. Maybe "outer scope, or enclosing loop or except handler".

[jacobtylerwalls]

This is tough, because although I'm somewhat partial to creating a new checker instead of extending redefined-outer-name, I feel the new case we are discussing:

for line in f:
    line = line.strip()

... is very similar to the nested loop scenario that already works, and if we make them both redefined-loop-name, then the previous disables for redefined-outer-name will break:

for y in range(...):
    for y in range(...):  # pylint: disable=redefined-outer-name
        ...

[jacobtylerwalls]

very similar to the nested loop scenario that already works, and if we make them both redefined-loop-name, then the previous disables for redefined-outer-name will break.

Is there precedent for allowing, say, a disable of redefined-outer-name to also disable redefined-loop-name? Especially (or only?) when the type of message is being changed such that the message emitted used to be redefined-outer-name and is changed to redefined-loop-name? That might be the most elegant solution.

[Pierre-Sassoulas]

the message emitted used to be redefined-outer-name and is changed to redefined-loop-name?

There's old_names defined in the MSGS dict of BaseCheckers, but the two messages needs to be equivalent all the time. See for example C0111 missing-docstring.

[superbobry]

We should probably do the same for redefined exceptions, which were added in #5370, since they are also technically in the same namespace.

[DanielNoord]

That should probably be discussed in a new issue in order not to lose this. Would you be able to open one @superbobry?