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

feat: warnings wrappers to use from C++ #5291

Merged
merged 9 commits into from
Aug 29, 2024
3 changes: 2 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ set(PYBIND11_HEADERS
include/pybind11/stl_bind.h
include/pybind11/stl/filesystem.h
include/pybind11/type_caster_pyobject_ptr.h
include/pybind11/typing.h)
include/pybind11/typing.h
include/pybind11/warnings.h)

# Compare with grep and warn if mismatched
if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)
Expand Down
70 changes: 70 additions & 0 deletions include/pybind11/warnings.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*
pybind11/warnings.h: Python warnings wrappers.

Copyright (c) 2024 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.
*/

#pragma once

#include "pybind11.h"
#include "detail/common.h"

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

PYBIND11_NAMESPACE_BEGIN(detail)

inline bool PyWarning_Check(PyObject *obj) {
int result = PyObject_IsSubclass(obj, PyExc_Warning);
if (result == 1) {
return true;
}
if (result == -1) {
raise_from(PyExc_SystemError,
"PyWarning_Check(): internal error of Python C API while "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a little simpler?

raise_from(PyExc_SystemError, "pybind11::detail::PyWarning_Check(): PyObject_IsSubclass() call failed.");

I think the fully-qualified function name is important here, otherwise people will think is a C Python function.

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, applied.

"checking a subclass of the object!");
throw error_already_set();
}
return false;
}

PYBIND11_NAMESPACE_END(detail)

PYBIND11_NAMESPACE_BEGIN(warnings)

inline object
new_warning_type(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 "
Copy link
Collaborator

Choose a reason for hiding this comment

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

pybind11_fail("warnings::new_warning_type(): ...")

?

To make it easier to pin-point this code based on the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied, also I have followed the pybind11::* convention from #5291 (comment) . I think it's better to keep it uniform across all messages, what do you think?

"PyExc_Warning!");
}
if (hasattr(scope, "__dict__") && scope.attr("__dict__").contains(name)) {
pybind11_fail("Error during initialization: multiple incompatible "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe

pybind11_fail("new_warning_type(): an attribute with name \"" + std::string(name) + "\" exists already.");

?

I'm also wondering, could the condition simply be hasattr(scope, name)? (I'm really not sure.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it works the same. Also added test case for it.

For the message same as #5291 (comment)

"definitions with name \""
+ std::string(name) + "\"");
}
std::string full_name = scope.attr("__name__").cast<std::string>() + std::string(".") + name;
handle h(PyErr_NewException(const_cast<char *>(full_name.c_str()), base.ptr(), nullptr));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the PyErr_NewException() documentation is incomplete because there is no mention of errors, but almost certainly we need at least this:

if (!h) {
    throw error_already_set();
}

I think that'd be sufficient, although raise_from() like you have above might be even better. I'm not sure how to judge cost:benefit here.

Copy link
Contributor Author

@jiwaszki jiwaszki Aug 15, 2024

Choose a reason for hiding this comment

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

I agree on that, I have modified it and move creation of handle after the check -- so any null should be verified first.

About the docs... yes, they are "very vague" about error messages so referring to them and cpython code this is the best that can be done. However, I am not able to think of a way of testing it. Seems like there is no way to break it. To break it down:

  • name is always consisting one dot - it's forced by full_name.
  • base is also validated by PyWarning_Check (plus it could also be null by design of cpython function).
  • dict by itself is always a nullptr here so that check in cpython is automatically "passed".

I can only imagine if somehow modulename is broken. Maybe I am missing something but it seems to me like trying to break just to prove "that it's breakable". Do you have any thoughts on that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's very common that testing of error conditions is incomplete. It's often just too difficult.

What I usually do: temporarily change e.g. if (ptr == nullptr) to if (ptr != nullptr) so that the if branch is reached, run the tests to convince myself that it works as intended, then change it back.

object obj = reinterpret_steal<object>(h);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I saw clang-tidy wants auto obj here.

scope.attr(name) = obj;
return obj;
}

// Similar to Python `warnings.warn()`
inline void
warn(const char *message, handle category = PyExc_RuntimeWarning, int stack_level = 2) {
if (!detail::PyWarning_Check(category.ptr())) {
pybind11_fail("raise_warning(): cannot raise warning, category must be a subclass of "
Copy link
Collaborator

Choose a reason for hiding this comment

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

pybind11_fail("wanings::warn(): ...")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as #5291 (comment)

"PyExc_Warning!");
}

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

PYBIND11_NAMESPACE_END(warnings)

PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)
3 changes: 2 additions & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,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
76 changes: 76 additions & 0 deletions tests/test_warnings.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
tests/test_warnings.cpp -- usage of warnings::warn() and warnings categories.

Copyright (c) 2024 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/warnings.h>

#include "pybind11_tests.h"

#include <utility>

namespace warning_helpers {
Copy link
Collaborator

@rwgk rwgk Aug 16, 2024

Choose a reason for hiding this comment

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

Could you please put this in a namespace?

Usually, in new code, I'd have this here:

namespace pybind11_tests {
namespace warnings {

This is because we're linking most tests together into one extension (something I wanted to change for years, but it's still like that). Without using namespaces we might get accidental ODR violations (in the link step).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

void warn_function(py::module &m, const char *name, py::handle category, const char *message) {
m.def(name, [category, message]() { py::warnings::warn(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::warnings::warn(message.c_str(), PyExc_Warning);
return 21;
});

m.def("raise_default", []() { py::warnings::warn("RuntimeWarning is raised!"); });

m.def("raise_from_cpython",
[]() { py::warnings::warn("UnicodeWarning is raised!", PyExc_UnicodeWarning); });

m.def("raise_and_fail",
[]() { py::warnings::warn("RuntimeError should be raised!", PyExc_Exception); });
Copy link
Collaborator

Choose a reason for hiding this comment

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

This confused me:

"RuntimeError should be raised!"

Suggestion to change to:

"Invalid category"

I'd also change raise_and_fail to raise_invalid_category

Actually: is "raise" a good term to use here (all test functions)?

Because there is no "raise" in warnings.warn().

Maybe warn_with_invalid_category etc? So that it is more clear to someone who gets here from a different context (e.g. refactoring or cleanup) what's happening.

Copy link
Contributor Author

@jiwaszki jiwaszki Aug 19, 2024

Choose a reason for hiding this comment

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

I see the motivation, I hope now names and messages are more self-explainatory/clear.


// Test custom warnings
PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store<py::object> ex_storage;
ex_storage.call_once_and_store_result([&]() {
return py::warnings::new_warning_type(m, "CustomWarning", PyExc_DeprecationWarning);
});

m.def("raise_custom", []() {
py::warnings::warn("CustomWarning was raised!", ex_storage.get_stored());
return 37;
});

// Bind warning categories
warning_helpers::warn_function(m, "raise_base_warning", PyExc_Warning, "This is Warning!");
warning_helpers::warn_function(
m, "raise_bytes_warning", PyExc_BytesWarning, "This is BytesWarning!");
warning_helpers::warn_function(
m, "raise_deprecation_warning", PyExc_DeprecationWarning, "This is DeprecationWarning!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd delete all warning below: mostly because it doesn't exercise anything specific in your implementation, but also because it will look old after a few years, because we're likely to not keep up with new warning types, and the wall of repetitive code is distracting from potentially more important things.

I think it's best to only keep PyExc_Warning (the base class) and one randomly picked derived class.

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 have removed them all as other functions like warn_and_return_value or warn_with_different_category use these categories. This also result in obsolecence of namespace pybind11_tests/warnings as no helpers are needed now (ref #5291 (comment)).

warning_helpers::warn_function(
m, "raise_future_warning", PyExc_FutureWarning, "This is FutureWarning!");
warning_helpers::warn_function(
m, "raise_import_warning", PyExc_ImportWarning, "This is ImportWarning!");
warning_helpers::warn_function(m,
"raise_pending_deprecation_warning",
PyExc_PendingDeprecationWarning,
"This is PendingDeprecationWarning!");
warning_helpers::warn_function(
m, "raise_resource_warning", PyExc_ResourceWarning, "This is ResourceWarning!");
warning_helpers::warn_function(
m, "raise_runtime_warning", PyExc_RuntimeWarning, "This is RuntimeWarning!");
warning_helpers::warn_function(
m, "raise_syntax_warning", PyExc_SyntaxWarning, "This is SyntaxWarning!");
warning_helpers::warn_function(
m, "raise_unicode_warning", PyExc_UnicodeWarning, "This is UnicodeWarning!");
warning_helpers::warn_function(
m, "raise_user_warning", PyExc_UserWarning, "This is UserWarning!");
}
104 changes: 104 additions & 0 deletions tests/test_warnings.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
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",
),
[
(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be the only one list item.

I'd either remove the @pytest.mark.parametrize() here and just assign the variables directly (e.g. expected_value = 37) or try to think if there could be another test we could meaningfully add here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I guess it's a leftover from previous iterations (not sure but looks like one:)). Removed!

m.CustomWarning,
DeprecationWarning,
"CustomWarning was raised!",
37,
m.raise_custom,
),
],
)
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__}!"
Loading