gh-138890: Add clearer message when deleting builtins with del#139078
gh-138890: Add clearer message when deleting builtins with del#139078Tapeline wants to merge 5 commits intopython:mainfrom
del#139078Conversation
| 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?
| 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.
There was a problem hiding this comment.
No, the new usage of PyTuple_GetItem is potentially dangerous. Initially, it was used alongside _PyEval_FormatExcCheckArg, which would safely check for NULL to propagate exceptions during the lookup. Now, we're calling PyMapping_HasKeyWithError with a potentially NULL argument, which isn't safe. There are two solutions:
- Use
PyTuple_GET_ITEM, which prevents failure entirely. This can possibly crash if someone messed with the code object, but that's possible already. - Check the result of
PyTuple_GetItembefore callingPyMapping_HasKeyWithError.
My suggestion was choosing the former, but I missed that we were using PyTuple_GetItem initially. I think you're right that we should continue using that.
There was a problem hiding this comment.
No, the new usage of PyTuple_GetItem is potentially dangerous
I missed that we were using PyTuple_GetItem initially. I think you're right that we should continue using that.
So what should we do? Do we leave this as is?
There was a problem hiding this comment.
Let's do this:
Check the result of
PyTuple_GetItembefore callingPyMapping_HasKeyWithError.
Try to fit C code in reasonable width
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
The other two tests also assume that the user correctly typed the intended name and meant to delete a builtin rather than a masking use. I think this will be true less than half the time.
| def f(): | ||
| del all | ||
|
|
||
| with self.assertRaisesRegex(UnboundLocalError, "cannot delete builtin 'all'"): | ||
| f() |
There was a problem hiding this comment.
Within a function it does not matter whether a name not in a global statement is or is not in globals or builtins. It only matters that it is an unbound local. Leave this case alone.
There was a problem hiding this comment.
If we do so, should we also revert changes in DELETE_FAST implementation?
|
This PR is stale because it has been open for 30 days with no activity. |
6 participants
Uh oh!
There was an error while loading. Please reload this page.