astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
33.1k stars 1.11k forks source link

False positive F821 and F841 for an exception object inside a lambda function #14521

Closed dqd closed 3 days ago

dqd commented 4 days ago

Hello, when I run ruff on this code sample, I get F821 and F841 errors:

try:
    raise Exception("test")
except Exception as e:
    print((lambda: e)())

This is the exact output of the ruff check sample.py command (using the current ruff version, i.e. 0.7.4):

sample.py:3:21: F841 [*] Local variable `e` is assigned to but never used
  |
1 | try:
2 |     raise Exception("test")
3 | except Exception as e:
  |                     ^ F841
4 |     print((lambda: e)())
  |
  = help: Remove assignment to unused variable `e`

sample.py:4:20: F821 Undefined name `e`
  |
2 |     raise Exception("test")
3 | except Exception as e:
4 |     print((lambda: e)())
  |                    ^ F821
  |

Found 2 errors.
[*] 1 fixable with the `--fix` option.

I believe that this is a false positive, as this code runs fine on cPython 3.9. It prints "test".

dylwil3 commented 3 days ago

Thanks for this! Great detective work because this seems to be truly minimal - the issue really has to do with except handlers.

Is it possible, @dhruvmanila or maybe @AlexWaygood , that the AST node for except handlers is meant to have an ExprName here instead of just an identifier? Otherwise how will the semantic model know that e is being stored in except Exception as e?

https://github.com/astral-sh/ruff/blob/e53ac7985daa7722b17e692967c96efe2e053586/crates/ruff_python_ast/src/nodes.rs#L3135-L3142

Edit: It looks like the semantic model is smarter than me, at least, but something fishy still must be going on... I can try to figure it out, sorry for the ping 😄

https://github.com/astral-sh/ruff/blob/e53ac7985daa7722b17e692967c96efe2e053586/crates/ruff_linter/src/checkers/ast/mod.rs#L1570-L1590

harupy commented 3 days ago

Although this looks like a false positive, NameError occurs if the function is called outside the exception handler:

try:
    raise Exception("test")
except Exception as e:
    f = lambda: print(e)

    f()  # works

f()  # throws

throws:

test
Traceback (most recent call last):
  File "a.py", line 9, in <module>
    f()  # throws
  File "a.py", line 4, in <lambda>
    f = lambda: print(f"{e!r}")
NameError: name 'e' is not defined
dylwil3 commented 3 days ago

Thanks @harupy, that's an interesting point! It still feels like a false positive to me, though, since e is used in the "local scope" (I use scope loosely here since except clauses are a bit weird). After all - the first call to f works.

When you leave an except clause, there is an implicit deletion of the binding of the exception to the variable in the as clause, which is the cause of the behavior you point out. So you can get the same behavior like this:

def outer():
    x = 1
    def inner():
        print(x)
    inner()   # fine
    del x
    inner()   # throws an error

Since the code

def outer():
    x = 1
    def inner():
        print(x)
    inner()   # fine
    del x

works just fine, and the printed "1" on my screen seems like evidence that x was used, I think it's a bug to flag this with either rule.

And, interestingly enough, this working example fools F821 playground link but for some reason does not trigger F841.

As it happens similar sorts of buggy behavior with these rules have been pointed out in many issues:

Including, as it happens - the same behavior we're talking about here, which I somehow missed: