False negative unused-variable for catched exception when used in the inner context

[Pierre-Sassoulas]

Steps to reproduce

Given a file a.py:

#pylint: disable=missing-docstring

def function():
    unused = 1  # [unused-variable]
    try:
        1 / 0
    except ZeroDivisionError as error:  # [unused-variable]
        try:
            1 / 0
        except ZeroDivisionError as error:
            raise Exception("") from error

Current behavior

************* Module a
a.py:4:4: W0612: Unused variable 'unused' (unused-variable)

------------------------------------------------------------------
Your code has been rated at 8.89/10 (previous run: 8.89/10, +0.00)

Expected behavior

************* Module a
a.py:4:4: W0612: Unused variable 'unused' (unused-variable)
a.py:8:30: W0612: Unused variable 'error' (unused-variable)

pylint --version output

pylint 2.7.4
astroid 2.5.3
Python 3.8.5 (default, Jan 27 2021, 15:41:15) 
[GCC 9.3.0]

[ksaketou]

Hello, I think that pylint considers the error variable as used due to the raise ... from error. I ran pylint on this code and I got the correct output.

#pylint: disable=missing-docstring

def function():
    unused = 1  # [unused-variable]
    try:
        1 / 0
    except ZeroDivisionError as error:  # [unused-variable]
        try:
            1 / 0
        except ZeroDivisionError as error:
            raise Exception("")

Output:

unused-variable.py:4:4: W0612: Unused variable 'unused' (unused-variable)
unused-variable.py:7:4: W0612: Unused variable 'error' (unused-variable)

[Pierre-Sassoulas]

Good point, this is certainely part of it, but I think that without the raise Exception("") from error the correct output would become:

unused-variable.py:4:4: W0612: Unused variable 'unused' (unused-variable)
unused-variable.py:7:4: W0612: Unused variable 'error' (unused-variable)
unused-variable.py:10:4: W0612: Unused variable 'error' (unused-variable)

[ksaketou]

Without the raise Exception("") from error the output stays the same. However, the output you suggest makes sense. I could give this a try if there is no one working on it

[Pierre-Sassoulas]

I assigned you :) Thanks for handling this !

[ksaketou]

Thank you:) One question, are there separate test files for the unused-variable error somewhere?

[Pierre-Sassoulas]

Separate test files ? I don't know if I understood but there is some functional tests here: https://github.com/PyCQA/pylint/tree/master/tests/functional/u/unused

[ksaketou]

okay great thanks

[ksaketou]

Now that I am looking at it a bit better, I notice that the same exception has been named as error twice, which I don't think that makes any sense. The ZeroDivisionError could be named once and then call it through the error value.

[Pierre-Sassoulas]

The second error shadows the first one, but this is valid code, we could imagine printing the first error for example:

#pylint: disable=missing-docstring

def function():
    unused = 1  # [unused-variable]
    try:
        1 / 0
    except ZeroDivisionError as error:
        print(f"First error : {error}")
        try:
            1 / 0
        except ZeroDivisionError as error:
            raise Exception("") from error

[ksaketou]

In the print statement, we consider that error is being used or not?

[Pierre-Sassoulas]

Yes the error would be used in the print then so no error should appear if there is a print. I think flake8 has a check for unused variable that works for this use case.

[Pierre-Sassoulas]

This fix is high effort as it require to modify control flow in astroid, please refer to this if you want to tackle it :)

[Pierre-Sassoulas]

Should probably be handled the same way as #5630 with a LIFO queue.

[orSolocate]

@Pierre-Sassoulas I guess this can be closed now :)

[DanielNoord]

We currently do:

❯ cat test.py
# pylint: disable=missing-docstring


def function():
    unused = 1  # [unused-variable]
    try:
        1 / 0
    except ZeroDivisionError as error:  # [unused-variable]
        try:
            1 / 0
        except ZeroDivisionError as error:
            raise Exception("") from error
❯ pylint test.py
************* Module _binding.test
test.py:11:8: W0621: Redefining name 'error' from outer scope (line 8) (redefined-outer-name)
test.py:5:4: W0612: Unused variable 'unused' (unused-variable)

------------------------------------------------------------------
Your code has been rated at 7.78/10 (previous run: 7.78/10, +0.00)

Is this the desired behaviour?

[Pierre-Sassoulas]

Thank you for testing this @DanielNoord , I think one of the unused variable is still a false negative (the first level as error). But there are flying MR in the variable checker, we should reacess once they're merged.

[jacobtylerwalls]

I think the output Daniel shows is correct -- line 8 overwrites "error" from line 5, so in line 9, the only "error" that exists is the one from line 8, and it's used in line 9. All done. "Error" was used. (We are warning about the redefined-outer-name going on, also.)

Without from error, then yes, we should warn that error is not used. But although it's a great test case to work on nested excepts, it doesn't even work for the basic case, see #2223. I think we could close this as a duplicate of #2223 and mention a nice edge case was discussed here.

[jacobtylerwalls]

Hold on!

#2223 only presents cases in a module scope. There you have to use --allow-global-unused-variables=False. It works; I'll close it.

Here, I think the nested example is fine, because we have redefined-outer-name to flag fishiness. The variables checker doesn't really have any sense of "you overwrote something and so some are used and some aren't".

For instance, you just get one message here:

def f():
    x = 0
    x = 0

 pylint h.py
************* Module h
h.py:2:4: W0612: Unused variable 'x' (unused-variable)

So, I think we can close.