Skip to content
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

Closed
wants to merge 9 commits into from
Closed
44 changes: 44 additions & 0 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -2673,6 +2673,50 @@ class exception : public object {
void operator()(const char *message) const { set_error(*this, message); }
};

/**
* Wrapper to generate a new Python warning type.
*
* It is not (yet) possible to use PyObject as a py::base.
Copy link
Collaborator

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 a PyErr_NewException() call, without any connection to the "reserved" template type. 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:

PYBIND11_NAMESPACE_BEGIN(warnings)

inline object
new_warning_type(handle scope, const char *name, handle base = PyExc_RuntimeWarning) {
    /* similar to what you have below */
}

// Similar to Python `warnings.warn()`
inline void
warn(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) {
    /* similar to what you have below */
}

PYBIND11_NAMESPACE_END(warnings)

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 just PyExc_BytesWarning etc.

I recommend closing this PR and starting a new one.

Copy link
Contributor Author

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.

* Template type argument is reserved for future use.
*/
template <typename type>
class warning : public object {
public:
warning() = default;
warning(handle scope, const char *name, handle base = PyExc_RuntimeWarning) {
if (!detail::PyWarning_Check(base.ptr())) {
pybind11_fail("warning(): cannot create custom warning, base must be a subclass of "
"PyExc_Warning!");
}
if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name)) {
pybind11_fail("Error during initialization: multiple incompatible "
"definitions with name \""
+ std::string(name) + "\"");
}
std::string full_name
= scope.attr("__name__").cast<std::string>() + std::string(".") + name;
m_ptr = PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), nullptr);
scope.attr(name) = *this;
}

void operator()(const char *message, int stack_level = 2) {
raise_warning(message, *this, stack_level);
}
};

// Raise Python warning based on the Python warning category.
inline void
raise_warning(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) {
if (!pybind11::detail::PyWarning_Check(category.ptr())) {
pybind11_fail("raise_warning(): cannot raise warning, category must be a subclass of "
"PyExc_Warning!");
}

if (PyErr_WarnEx(category.ptr(), message, stack_level) == -1) {
throw error_already_set();
}
}

PYBIND11_NAMESPACE_BEGIN(detail)

template <>
Expand Down
54 changes: 54 additions & 0 deletions include/pybind11/pytypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 raise_from() followed by throw error_already_set().

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.
Expand Down
3 changes: 2 additions & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ set(PYBIND11_TEST_FILES
test_unnamed_namespace_a
test_unnamed_namespace_b
test_vector_unique_ptr_member
test_virtual_functions)
test_virtual_functions
test_warnings)

# Invoking cmake with something like:
# cmake -DPYBIND11_TEST_OVERRIDE="test_callbacks.cpp;test_pickling.cpp" ..
Expand Down
77 changes: 77 additions & 0 deletions tests/test_warnings.cpp
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a very common pitfall:

The destructor may run (and call Py_DECREF) only after the Python interpreter is finalized already.

Best to adopt this approach:

PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;
ex_storage.call_once_and_store_result(
[&]() { return py::exception<MyException>(m, "MyException"); });


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!");
}
111 changes: 111 additions & 0 deletions tests/test_warnings.py
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__}!"