-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Handling warnings from C++ #4535
Changes from all commits
3f54c50
ed16550
78235ac
ad9eaf1
0cc1c3e
3dc83a4
60314d5
0dfebe1
d754b47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -828,6 +828,60 @@ inline void raise_from(error_already_set &err, PyObject *type, const char *messa | |
raise_from(type, message); | ||
} | ||
|
||
PYBIND11_NAMESPACE_BEGIN(detail) | ||
// Check if obj is a subclass of PyExc_Warning. | ||
inline bool PyWarning_Check(PyObject *obj) { | ||
int result = PyObject_IsSubclass(obj, PyExc_Warning); | ||
if (result == 1) { | ||
return true; | ||
} | ||
if (result == -1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably have a more visible error / warning here right? Isn't -1 from IsSubclass supposed to be propagated to users? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is new commit OK? I am just not sure what "guidelines" on errors are applied for this specific scenario. |
||
PyErr_Clear(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never discard an error only to replace it with something more opaque: people will want to know what the "internal error" is (line below). Best here: use |
||
pybind11_fail("PyWarning_Check(): internal error of Python C API while " | ||
"checking a subclass of the object!"); | ||
} | ||
return false; | ||
} | ||
PYBIND11_NAMESPACE_END(detail) | ||
|
||
/// Namespace for Python warning categories | ||
PYBIND11_NAMESPACE_BEGIN(warnings) | ||
|
||
// Warning class | ||
static PyObject *const warning_base = PyExc_Warning; | ||
|
||
// BytesWarning class | ||
static PyObject *const bytes = PyExc_BytesWarning; | ||
|
||
// DeprecationWarning class | ||
static PyObject *const deprecation = PyExc_DeprecationWarning; | ||
|
||
// FutureWarning class | ||
static PyObject *const future = PyExc_FutureWarning; | ||
|
||
// ImportWarning class | ||
static PyObject *const import = PyExc_ImportWarning; | ||
|
||
// PendingDeprecationWarning class | ||
static PyObject *const pending_deprecation = PyExc_PendingDeprecationWarning; | ||
|
||
// ResourceWarning class | ||
static PyObject *const resource = PyExc_ResourceWarning; | ||
|
||
// RuntimeWarning class | ||
static PyObject *const runtime = PyExc_RuntimeWarning; | ||
|
||
// RuntimeWarning class | ||
static PyObject *const syntax = PyExc_SyntaxWarning; | ||
|
||
// DeprecationWarning class | ||
static PyObject *const unicode = PyExc_UnicodeWarning; | ||
|
||
// UserWarning class | ||
static PyObject *const user = PyExc_UserWarning; | ||
|
||
PYBIND11_NAMESPACE_END(warnings) | ||
|
||
/** \defgroup python_builtins const_name | ||
Unless stated otherwise, the following C++ functions behave the same | ||
as their Python counterparts. | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,77 @@ | ||||||||
/* | ||||||||
tests/test_warnings.cpp -- usage of raise_warning() and warnings categories | ||||||||
|
||||||||
Copyright (c) 2023 Jan Iwaszkiewicz <[email protected]> | ||||||||
|
||||||||
All rights reserved. Use of this source code is governed by a | ||||||||
BSD-style license that can be found in the LICENSE file. | ||||||||
*/ | ||||||||
|
||||||||
#include "pybind11_tests.h" | ||||||||
|
||||||||
#include <utility> | ||||||||
|
||||||||
namespace warning_helpers { | ||||||||
void warn_function(py::module &m, const char *name, py::handle category, const char *message) { | ||||||||
m.def(name, [category, message]() { py::raise_warning(message, category); }); | ||||||||
} | ||||||||
} // namespace warning_helpers | ||||||||
|
||||||||
class CustomWarning {}; | ||||||||
|
||||||||
TEST_SUBMODULE(warnings_, m) { | ||||||||
|
||||||||
// Test warning mechanism base | ||||||||
m.def("raise_and_return", []() { | ||||||||
std::string message = "Warning was raised!"; | ||||||||
py::raise_warning(message.c_str(), py::warnings::warning_base); | ||||||||
return 21; | ||||||||
}); | ||||||||
|
||||||||
m.def("raise_default", []() { py::raise_warning("RuntimeWarning is raised!"); }); | ||||||||
|
||||||||
m.def("raise_from_cpython", | ||||||||
[]() { py::raise_warning("UnicodeWarning is raised!", PyExc_UnicodeWarning); }); | ||||||||
|
||||||||
m.def("raise_and_fail", | ||||||||
[]() { py::raise_warning("RuntimeError should be raised!", PyExc_Exception); }); | ||||||||
|
||||||||
// Test custom warnings | ||||||||
static py::warning<CustomWarning> my_warning(m, "CustomWarning", py::warnings::deprecation); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a very common pitfall: The destructor may run (and call Best to adopt this approach: pybind11/tests/test_exceptions.cpp Lines 119 to 121 in 43de801
|
||||||||
|
||||||||
m.def("raise_custom", []() { | ||||||||
py::raise_warning("CustomWarning was raised!", my_warning); | ||||||||
return 37; | ||||||||
}); | ||||||||
|
||||||||
m.def("raise_with_wrapper", []() { | ||||||||
my_warning("This is raised from a wrapper."); | ||||||||
return 42; | ||||||||
}); | ||||||||
|
||||||||
// Bind warning categories | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_base_warning", py::warnings::warning_base, "This is Warning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_bytes_warning", py::warnings::bytes, "This is BytesWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_deprecation_warning", py::warnings::deprecation, "This is DeprecationWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_future_warning", py::warnings::future, "This is FutureWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_import_warning", py::warnings::import, "This is ImportWarning!"); | ||||||||
warning_helpers::warn_function(m, | ||||||||
"raise_pending_deprecation_warning", | ||||||||
py::warnings::pending_deprecation, | ||||||||
"This is PendingDeprecationWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_resource_warning", py::warnings::resource, "This is ResourceWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_runtime_warning", py::warnings::runtime, "This is RuntimeWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_syntax_warning", py::warnings::syntax, "This is SyntaxWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_unicode_warning", py::warnings::unicode, "This is UnicodeWarning!"); | ||||||||
warning_helpers::warn_function( | ||||||||
m, "raise_user_warning", py::warnings::user, "This is UserWarning!"); | ||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
from __future__ import annotations | ||
|
||
import warnings | ||
|
||
import pytest | ||
|
||
import pybind11_tests # noqa: F401 | ||
from pybind11_tests import warnings_ as m | ||
|
||
|
||
@pytest.mark.parametrize( | ||
("expected_category", "expected_message", "expected_value", "module_function"), | ||
[ | ||
(Warning, "Warning was raised!", 21, m.raise_and_return), | ||
(RuntimeWarning, "RuntimeWarning is raised!", None, m.raise_default), | ||
(UnicodeWarning, "UnicodeWarning is raised!", None, m.raise_from_cpython), | ||
], | ||
) | ||
def test_warning_simple( | ||
expected_category, expected_message, expected_value, module_function | ||
): | ||
with pytest.warns(Warning) as excinfo: | ||
value = module_function() | ||
|
||
assert issubclass(excinfo[0].category, expected_category) | ||
assert str(excinfo[0].message) == expected_message | ||
assert value == expected_value | ||
|
||
|
||
def test_warning_fail(): | ||
with pytest.raises(Exception) as excinfo: | ||
m.raise_and_fail() | ||
|
||
assert issubclass(excinfo.type, RuntimeError) | ||
assert ( | ||
str(excinfo.value) | ||
== "raise_warning(): cannot raise warning, category must be a subclass of PyExc_Warning!" | ||
) | ||
|
||
|
||
def test_warning_register(): | ||
assert m.CustomWarning is not None | ||
assert issubclass(m.CustomWarning, DeprecationWarning) | ||
|
||
with pytest.warns(m.CustomWarning) as excinfo: | ||
warnings.warn("This is warning from Python!", m.CustomWarning, stacklevel=1) | ||
|
||
assert issubclass(excinfo[0].category, DeprecationWarning) | ||
assert issubclass(excinfo[0].category, m.CustomWarning) | ||
assert str(excinfo[0].message) == "This is warning from Python!" | ||
|
||
|
||
@pytest.mark.parametrize( | ||
( | ||
"expected_category", | ||
"expected_base", | ||
"expected_message", | ||
"expected_value", | ||
"module_function", | ||
), | ||
[ | ||
( | ||
m.CustomWarning, | ||
DeprecationWarning, | ||
"CustomWarning was raised!", | ||
37, | ||
m.raise_custom, | ||
), | ||
( | ||
m.CustomWarning, | ||
DeprecationWarning, | ||
"This is raised from a wrapper.", | ||
42, | ||
m.raise_with_wrapper, | ||
), | ||
], | ||
) | ||
def test_warning_custom( | ||
expected_category, expected_base, expected_message, expected_value, module_function | ||
): | ||
with pytest.warns(expected_category) as excinfo: | ||
value = module_function() | ||
|
||
assert issubclass(excinfo[0].category, expected_base) | ||
assert issubclass(excinfo[0].category, expected_category) | ||
assert str(excinfo[0].message) == expected_message | ||
assert value == expected_value | ||
|
||
|
||
@pytest.mark.parametrize( | ||
("expected_category", "module_function"), | ||
[ | ||
(Warning, m.raise_base_warning), | ||
(BytesWarning, m.raise_bytes_warning), | ||
(DeprecationWarning, m.raise_deprecation_warning), | ||
(FutureWarning, m.raise_future_warning), | ||
(ImportWarning, m.raise_import_warning), | ||
(PendingDeprecationWarning, m.raise_pending_deprecation_warning), | ||
(ResourceWarning, m.raise_resource_warning), | ||
(RuntimeWarning, m.raise_runtime_warning), | ||
(SyntaxWarning, m.raise_syntax_warning), | ||
(UnicodeWarning, m.raise_unicode_warning), | ||
(UserWarning, m.raise_user_warning), | ||
], | ||
) | ||
def test_warning_categories(expected_category, module_function): | ||
with pytest.warns(Warning) as excinfo: | ||
module_function() | ||
|
||
assert issubclass(excinfo[0].category, expected_category) | ||
assert str(excinfo[0].message) == f"This is {expected_category.__name__}!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the existing
exception
type is a failed design that is better not replicated here. All it does is wrap aPyErr_NewException()
call, without any connection to the "reserved" templatetype
. It's more likely to be misleading/confusing than helpful. E.g. anyone uninitiated reading client code like this ...https://github.com/pybind/pybind11_abseil/blob/32cf30f118708025a92456d076cb15a73c7bea73/pybind11_abseil/status_utils.cc#L91
... would think there is a formal connection between the C++ exception type and the Python exception type, but there is none here. That connection is (maybe) only established elsewhere, with an exception translator.
I think what we need here is something simpler and straightforward, for example:
Could you please try that?
I think it's best implemented in a new pybind11/warnings.h file.
That's all I'd do. I definitely wouldn't want to introduce a bunch of aliases (
warning_base
,bytes
,deprections
, ...), which are essentially just a level of obfuscation: people would have to learn ah, it's justPyExc_BytesWarning
etc.I recommend closing this PR and starting a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the point. I have already opened new PR for it, let's continue there.