Skip to content

Fix missing pythonic type hints for native_enum #5619

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
merged 3 commits into from
Apr 16, 2025

Conversation

dyollb
Copy link
Contributor

@dyollb dyollb commented Apr 15, 2025

Description

Fixes #5618

Suggested changelog entry:

Fix signature for functions with a native_enum in the signature

@dyollb
Copy link
Contributor Author

dyollb commented Apr 15, 2025

I also tested the following, however, I am unsure if the return type is correct pybind11_tests.native_enum.in_class. Should it not be pybind11_tests.native_enum.class_with_enum.in_class?

struct class_with_enum {
    enum class in_class { one, two };

    in_class value = in_class::one;
};

TEST_SUBMODULE(native_enum, m) {
    py::class_<class_with_enum> py_class_with_enum(m, "class_with_enum");
    py::native_enum<class_with_enum::in_class>(py_class_with_enum, "in_class", "enum.IntEnum")
        .value("one", class_with_enum::in_class::one)
        .value("two", class_with_enum::in_class::two)
        .finalize();

    py_class_with_enum.def(py::init()).def_readwrite("value", &class_with_enum::value);
def test_property_type_hint():
    prop = m.class_with_enum.__dict__["value"]
    assert isinstance(prop, property)
    assert prop.fget.__doc__.startswith(
        "(self: pybind11_tests.native_enum.class_with_enum) -> pybind11_tests.native_enum.in_class"
    )

@dyollb
Copy link
Contributor Author

dyollb commented Apr 15, 2025

Digging a bit deeper, it looks like maybe the native_enum has the "wrong" __qualname__. If I patch it like so

inline void native_enum_data::finalize() {
    disarm_finalize_check("DOUBLE finalize");
    if (hasattr(parent_scope, enum_name)) {
        pybind11_fail("pybind11::native_enum<...>(\"" + enum_name_encoded
                      + "\"): an object with that name is already defined");
    }
    auto py_enum_type = import_or_getattr(native_type_name, " (native_type_name)");
    auto py_enum = py_enum_type(enum_name, members);
    object module_name = get_module_name_if_available(parent_scope);
    if (module_name) {
        py_enum.attr("__module__") = module_name;
    }
    # START PATCH
    if (hasattr(parent_scope, "__qualname__")) {
        const auto parent_qualname = parent_scope.attr("__qualname__").cast<std::string>();
        py_enum.attr("__qualname__") = str(parent_qualname + "." + enum_name.cast<std::string>());
    }
    # END PATCH
    parent_scope.attr(enum_name) = py_enum;
   ...

I get the expected class type in the property.fget signature above. Interestingly it also "fixes" the pickle test test_pickle_roundtrip. The nested enum no longer raises a TypeError and actually can be pickled.

EDIT: I commited the changes shown in the comments for you to review.

dyollb and others added 2 commits April 15, 2025 13:53
…_AND_MEMBERS`. Move new code around to fit in more organically.

"native" is used in so many places here, it would be very difficult to pin-point where the specific enum type is used.

Similarly, "value" is difficult to pin-point. Changed to "nested_value".
@rwgk
Copy link
Collaborator

rwgk commented Apr 15, 2025

@dyollb Thanks for the fixes, that's really awesome!

I added commit 247a1d4, could you please review? — This changes only the test code. See commit message for the motivation.

@rwgk
Copy link
Collaborator

rwgk commented Apr 15, 2025

I forgot to add: enum class func_sig_rendering now doubles as a test for the corner case that there are no members.

Copy link
Contributor Author

@dyollb dyollb left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@rwgk rwgk merged commit 223e2e9 into pybind:master Apr 16, 2025
63 of 65 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Apr 16, 2025
@dyollb
Copy link
Contributor Author

dyollb commented Apr 16, 2025

@rwgk thanks for the fast review process! It's really appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs changelog Possibly needs a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: non-pythonic function signature for native_enum
2 participants