8000 gh-106672: C API: Report indiscriminately ignored errors by serhiy-storchaka · Pull Request #106674 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

gh-106672: C API: Report indiscriminately ignored errors #106674

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Improve error messages for ignoring errors set before the calls.
  • Loading branch information
serhiy-storchaka committed Nov 5, 2023
commit 241576a570416648ed31727268a7d9dad217eae4
8 changes: 4 additions & 4 deletions Objects/abstract.c
Original file line number Diff line number Diff line change
Expand Up @@ -2492,8 +2492,8 @@ PyMapping_HasKeyString(PyObject *obj, const char *key)
// if the key is not found.
if (rc == 0 && PyErr_Occurred()) {
PyErr_FormatUnraisable(
"Exception ignored in PyMapping_HasKeyString(); consider using "
"PyMapping_HasKeyStringWithError(), "
"Ignore exception set before calling in PyMapping_HasKeyString(); "
"consider using PyMapping_HasKeyStringWithError(), "
"PyMapping_GetOptionalItemString() or PyMapping_GetItemString()");
Copy link
Member
@vstinner vstinner Nov 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these 3 functions be called with an exception set? They don't override the exception? That sounds surprising. I would prefer suggesting to not call the function with an exception set. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got catch. Indeed, PyMapping_GetOptionalItemString() can only return 0 if no exception is set, so this condition is always false. Also, the alternative functions also can clear exceptions.

I think that we should classify the C API by classes:

  1. Function that can be called when an exception is set, and they do not change it.
  2. Function that can be called when an exception is set, and they do not change it unless they fail.
  3. Function that can be called when an exception is set, but they can change it even at success.
  4. Function that can be called when an exception is set, but the result is ambiguous in some cases (you cannot distinguish some successful results from failure).
  5. Function that should never be called when an exception is set.

There may be more classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the general case, to write safe code handling a raised exception, I think the safest option is to keep the exception aside using PyErr_GetRaisedException(). Maybe today some functions are perfectly fine and never override the currently raised exception. But what if tomorrow their implementation changes, and they may start to clear the currently raised exception?

In Python, in an except: block, there is no "currently raised exception" in the C level, even if sys.exc_info() returns the exception. The difference between PyThreadState.exc_info and PyThreadState.current_exception is subtle.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why it should be clearly documented.

Obviously you can call PyErr_Occurred(), PyErr_GetRaisedException() or PyErr_WriteUnraisable() when an exception is set.

return 0;
}
Expand Down Expand Up @@ -2526,8 +2526,8 @@ PyMapping_HasKey(PyObject *obj, PyObject *key)
// if the key is not found.
if (rc == 0 && PyErr_Occurred()) {
PyErr_FormatUnraisable(
"Exception ignored in PyMapping_HasKey(); consider using "
"PyMapping_HasKeyWithError(), "
"Ignore exception set before calling in PyMapping_HasKey(); "
"consider using PyMapping_HasKeyWithError(), "
"PyMapping_GetOptionalItem() or PyObject_GetItem()");
return 0;
}
Expand Down
1 change: 0 additions & 1 deletion Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3942,7 +3942,6 @@ PyDict_GetItemString(PyObject *v, const char *key)
"PyDict_GetItemRefString()");
return NULL;
}
rv = PyDict_GetItem(v, kv);
rv = dict_getitem(v, kv,
"Exception ignored in PyDict_GetItemString(); consider using "
"PyDict_GetItemRefString()");
Expand Down
0