gh-138890: Add clearer message when deleting builtins with del
#139078
Conversation
Python/generated_cases.c.h
Outdated
| UNBOUNDLOCAL_ERROR_MSG, | ||
| PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, oparg) | ||
| ); | ||
| name = PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, |
There was a problem hiding this comment.
Maybe it is better no to break the declaration and the assignment?
Python/generated_cases.c.h
Outdated
| name = PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, | ||
| oparg); | ||
| const char *err_msg = PyMapping_HasKey(BUILTINS(), name) | ||
| ? CANNOT_DELETE_BUILTIN_ERROR_MSG |
There was a problem hiding this comment.
I think it is better to proper align these two lines.
There was a problem hiding this comment.
Shouldn't generated_cases.c.h be left without review?
There was a problem hiding this comment.
Yes, this is a generated file.
Python/bytecodes.c
Outdated
| ERROR_NO_POP(); | ||
| } | ||
| if (err == 0) { | ||
| const char *err_msg = PyMapping_HasKey(BUILTINS(), name) |
There was a problem hiding this comment.
I believe it is safe to use PyDict_* here.
There was a problem hiding this comment.
There's no PyDict_* equivalent of PyMapping_HasKey, but we should be using PyMapping_HasKeyWithError here.
Python/bytecodes.c
Outdated
| ERROR_NO_POP(); | ||
| } | ||
| if (err == 0) { | ||
| const char *err_msg = PyMapping_HasKey(BUILTINS(), name) |
There was a problem hiding this comment.
There's no PyDict_* equivalent of PyMapping_HasKey, but we should be using PyMapping_HasKeyWithError here.
Python/bytecodes.c
Outdated
| PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, oparg) | ||
| ); | ||
| PyObject *name; | ||
| name = PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, |
There was a problem hiding this comment.
PyTuple_GetItem can fail. We should be able to expect that it can't, so let's use PyTuple_GET_ITEM.
There was a problem hiding this comment.
Shouldn't we leave this as is? This seems like an unrelated change.
Python/bytecodes.c
Outdated
| PyObject *name; | ||
| name = PyTuple_GetItem(_PyFrame_GetCode(frame)->co_localsplusnames, | ||
| oparg); | ||
| const char *err_msg = PyMapping_HasKey(BUILTINS(), name) |
There was a problem hiding this comment.
Ditto -- we should be using PyMapping_HasKeyWithError.
| @@ -0,0 +1 @@ | |||
| Deleting builtin names with ``del`` now gives a clearer error message. | |||
There was a problem hiding this comment.
| Deleting builtin names with ``del`` now gives a clearer error message. | |
| Deleting built-in names with :keyword:`del` now gives a clearer error message. |
Lib/test/test_scope.py
Outdated
|
|
||
| def test_builtin_deletion_err_message(self): | ||
| with self.assertRaisesRegex( | ||
| NameError, "cannot delete builtin 'all'" |
There was a problem hiding this comment.
The formatting makes this a bit difficult to read. This isn't coming near PEP 8's 79 character limit, so let's just keep all the arguments on the same line. This applies to all the assertRaisesRegex calls below.
Try to fit C code in reasonable width
Uh oh!
There was an error while loading. Please reload this page.