classification
Title: Unhelpful UnboundLocalError due to del'ing of exception target
Type: enhancement Stage: patch review
Components: Interpreter Core Versions: Python 3.10
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, barry, benjamin.peterson, christian.heimes, eric.snow, ezio.melotti, iritkatriel, levkivskyi, ncoghlan, pconnell, pmpp, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-04-18 23:55 by barry, last changed 2021-06-02 13:13 by iritkatriel.

Files
File name Uploaded Description Edit
issue17792.diff ezio.melotti, 2013-04-19 04:53 review
issue17792-2.diff ezio.melotti, 2013-04-23 11:37 review
Pull Requests
URL Status Linked Edit
PR 24976 merged iritkatriel, 2021-03-22 17:48
Messages (19)
msg187313 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-04-18 23:55
As described here:

http://www.wefearchange.org/2013/04/python-3-language-gotcha-and-short.html

the following code will produce an UnboundLocalError when the exception is triggered:

def bad():
    e = None
    try:
        do_something()
    except KeyError as e:
        print('ke')
    except ValueError as e:
        print('ve')
    print(e)

The reason is that the exception handling machinery del's `e` from the local namespace in order to break circular references caused by __traceback__.  The ULE is pretty mysterious and certainly not helpful for figuring out what's going wrong (the blog post above describes how long it took me to find it ;).

Can we do better?  What if instead of del'ing the target, we set it to None?  We wouldn't get a ULE but the fact that print(e) would print None might be just as mysterious.  Any other ideas?

(BTW, there's likely nothing to be done for Python < 3.4.)
msg187315 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-04-19 00:01
Maybe we could raise a warning when the deleted name already exists in the local namespace?

(FWIW I knew about the fact that the name gets deleted, and still managed to get bitten by it a couple of times.)
msg187319 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-04-19 00:21
On Apr 19, 2013, at 12:01 AM, Ezio Melotti wrote:

>
>Maybe we could raise a warning when the deleted name already exists in the
>local namespace?

Ideally, I think a SyntaxError if you could detect a previously bound name in
the namespace being used as an exception target.

-Barry
msg187320 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-19 00:37
And what if it weren't a print statement?  An error is better than a "randomly" changed value, I think.  I'm really not sure there is anything we can do here, beyond Ezio's warning suggestion.
msg187322 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-19 00:40
This used to raise a SyntaxError.  See issue 4617.
msg187326 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-04-19 01:03
On Apr 19, 2013, at 12:37 AM, R. David Murray wrote:

>And what if it weren't a print statement?  An error is better than a
>"randomly" changed value, I think.  I'm really not sure there is anything we
>can do here, beyond Ezio's warning suggestion.

Right.  The point is, is it more helpful to have an unexpected value in the
local (e.g. None, which in fact may not be *totally* unexpected) or an
mystical exception? ;)

A warning would be fine.  I just want to give programmers some help in trying
to figure out why their code is wrong.
msg187327 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-04-19 01:29
It seems to me that this kind of dubious-but-legal code is what http://docs.python.org/3/library/exceptions.html#SyntaxWarning is designed to handle.

Something like "SyntaxWarning: implicitly deleted exception variable referenced after end of except clause".
msg187328 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-19 01:33
It occurs to me that one of the problems here is that the UnboundLocalError only occurs when the except clause is executed.  Do you think it would be helpful and acceptable (in 3.4, as it is a behavior change) to have the 'as' variable *always* deleted at the end of the try/except block?  Then you would always get the UnboundLocalError, instead of it mysteriously appearing only when the except was executed.  

Granted it is a bit weird, but then so is the variable getting deleted in the first place.
msg187339 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2013-04-19 03:15
I don't think we want to mess with the control flow, as that would be even weirder and may interact strangely with else and finally clauses.

A SyntaxWarning would be unconditional (so it doesn't matter whether or not the exception is triggered at run time), happens at compile time (so developers will see it), but won't be triggered when loading from a cached bytecode file (so end users typically won't see it).
msg187342 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-04-19 04:53
Attached a sketchy proof of concept.  It only checks for locals, but it should probably check for globals too.  Does the approach make sense?
msg187382 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2013-04-19 18:52
FWIW this has come up before:

http://mail.python.org/pipermail/python-dev/2012-October/122504.html

and relatedly:

issue16429
msg187433 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-04-20 15:12
Ezio, the problem with your patch is that it also gives a warning on this code, which is totally safe:

def good():
    exc = None
    try:
        bar(int(sys.argv[1]))
    except KeyError as e:
        print('ke')
        exc = e
    except ValueError as e:
        print('ve')
        exc = e
    print(exc)
msg187627 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-04-23 11:37
Attached a new patch that improves the following things:
* added some tests;
* the code in the previous message is now handled correctly;
* the patch now raises a proper SyntaxWarning;

There are still several problems though:
* it doesn't work for the global namespace.  Is there a way to know if we are inside a function or not?
* the patch leaks, and I couldn't figure out where the leak is;
* I tried to include the name of the variable in the warning, but:
  - PyErr_WarnExplicit wants a const char* as msg and doesn't support formatting;
  - It doesn't seem possible to use other PyErr_Warn* functions here;
  - Using PyUnicode_FromFormat() works, but then I don't know how to convert the result to const char* (I used PyObject_REPR(), but that is probably wrong; PyUnicode_AS_DATA() might work but is deprecated; the PyUnicode_nBYTE_DATA() functions return a Py_UCSn*);
* more testcases are needed


While compiling I got this warning:
./setup.py:330: SyntaxWarning: "name 'why' is already defined but implicitly deleted after end of except clause"
  except ImportError as why:

This comes from a code like:
try: foo()
except Exception as why: pass
try: bar()
except Exception as why: pass

and in this case there shouldn't be any warning. A possible way to fix this is to keep a "whitelist" of locals that are first defined as an except target, but then it will still break if there's a `why = ...` between the two try/except and it's starting to get too complicated...
msg187639 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-23 13:04
>   - Using PyUnicode_FromFormat() works, but then I don't know how to convert the result to const char*

PyUnicode_AsUTF8()
msg187640 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-04-23 13:09
I considered that, but is it guaranteed that the output encoding is UTF-8?  What if the warning is printed on a Windows console that uses cp1252?  I also considered encoding it and then use PyBytes_AsString, but I was hoping in something simpler (also I'm not sure where to get the right encoding).
msg187645 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-23 14:24
> I considered that, but is it guaranteed that the output encoding is UTF-8? 

PyErr_WarnExplicit() calls PyUnicode_FromString() which made an inverse 
decoding from UTF-8.
msg389264 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-03-21 23:22
I think the issue is that the error message for UnboundLocalError is wrong, see this example:

>>> def g():
...    x = 42
...    del x
...    print(x)
...
>>> g()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in g
UnboundLocalError: local variable 'x' referenced before assignment
>>>


How about we change it to "local variable 'x' referenced before assignment or after deletion"?
msg394902 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-06-02 13:09
New changeset 7b1f527d5b37dc3aa085ebbe11a1a2dd29ef210f by Irit Katriel in branch 'main':
bpo-17792: more accurate error message for unbound variable access exceptions (GH-24976)
https://github.com/python/cpython/commit/7b1f527d5b37dc3aa085ebbe11a1a2dd29ef210f
msg394903 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-06-02 13:13
Following a discussion on PR24976 we opted for

"cannot access local variable 'x' where it is not associated with a value"

This is because binding it not always related to assignment.

I don't know whether this completely resolves this issue, or whether there is still something to change around the None-setting of except targets.
History
Date User Action Args
2021-06-02 13:13:18iritkatrielsetmessages: + msg394903
2021-06-02 13:09:17iritkatrielsetmessages: + msg394902
2021-03-22 17:48:29iritkatrielsetstage: patch review
pull_requests: + pull_request23735
2021-03-21 23:39:26pmppsetnosy: + pmpp
2021-03-21 23:22:07iritkatrielsetversions: + Python 3.10, - Python 3.4
nosy: + iritkatriel

messages: + msg389264

components: + Interpreter Core
2017-03-21 13:21:06levkivskyisetnosy: + levkivskyi
2013-07-06 00:36:42christian.heimessetnosy: + christian.heimes
2013-05-30 15:41:56Arfreversetnosy: + Arfrever
2013-05-30 14:08:29pconnellsetnosy: + pconnell
2013-04-23 14:24:56serhiy.storchakasetmessages: + msg187645
2013-04-23 13:09:48ezio.melottisetmessages: + msg187640
2013-04-23 13:04:52serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg187639
2013-04-23 11:37:04ezio.melottisetfiles: + issue17792-2.diff

messages: + msg187627
2013-04-20 15:12:14barrysetmessages: + msg187433
2013-04-19 18:52:14eric.snowsetnosy: + eric.snow
messages: + msg187382
2013-04-19 04:53:22ezio.melottisetfiles: + issue17792.diff

nosy: + benjamin.peterson
messages: + msg187342

keywords: + patch
type: enhancement
2013-04-19 03:15:03ncoghlansetmessages: + msg187339
2013-04-19 01:33:05r.david.murraysetmessages: + msg187328
2013-04-19 01:29:05ncoghlansetnosy: + ncoghlan
messages: + msg187327
2013-04-19 01:03:24barrysetmessages: + msg187326
2013-04-19 00:40:49r.david.murraysetmessages: + msg187322
2013-04-19 00:37:12r.david.murraysetnosy: + r.david.murray
messages: + msg187320
2013-04-19 00:21:50barrysetmessages: + msg187319
2013-04-19 00:01:42ezio.melottisetnosy: + ezio.melotti
messages: + msg187315
2013-04-18 23:55:22barrycreate