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

[BUG]: keep_alive does not applied to properties #5046

Open
3 tasks done
jnooree opened this issue Mar 4, 2024 · 0 comments
Open
3 tasks done

[BUG]: keep_alive does not applied to properties #5046

jnooree opened this issue Mar 4, 2024 · 0 comments
Labels
triage New bug, unverified

Comments

@jnooree
Copy link

jnooree commented Mar 4, 2024

Required prerequisites

What version (or hash if on master) of pybind11 are you using?

2.10.4, but 8b48ff8 (the current master) still has the same problem.

Problem description

The problem

Passing keep_alive directly to def_property* family is ignored. The flow:

  1. The getter/setters are created without "extra" attributes. Related source locations:
    cpp_function(method_adaptor<type>(fget)),

    return def_property_readonly_static(

    return def_property(
    name, fget, cpp_function(method_adaptor<type>(fset), is_setter()), extra...);

    return def_property(name,
    cpp_function(method_adaptor<type>(fget)),
    fset,
    return_value_policy::reference_internal,
    extra...);

    return def_property_static(
    name, cpp_function(fget), fset, return_value_policy::reference, extra...);
  2. All the calls to def_properties* eventually forwarded to def_property_static, and "extra" attributes are processed there, with process_attributes<Extra...>::init:
    /// Uses cpp_function's return_value_policy by default
    template <typename... Extra>
    class_ &def_property_static(const char *name,
    const cpp_function &fget,
    const cpp_function &fset,
    const Extra &...extra) {
    static_assert(0 == detail::constexpr_sum(std::is_base_of<arg, Extra>::value...),
    "Argument annotations are not allowed for properties");
    auto rec_fget = get_function_record(fget), rec_fset = get_function_record(fset);
    auto *rec_active = rec_fget;
    if (rec_fget) {
    char *doc_prev = rec_fget->doc; /* 'extra' field may include a property-specific
    documentation string */
    detail::process_attributes<Extra...>::init(extra..., rec_fget);
    if (rec_fget->doc && rec_fget->doc != doc_prev) {
    std::free(doc_prev);
    rec_fget->doc = PYBIND11_COMPAT_STRDUP(rec_fget->doc);
    }
    }
    if (rec_fset) {
    char *doc_prev = rec_fset->doc;
    detail::process_attributes<Extra...>::init(extra..., rec_fset);
    if (rec_fset->doc && rec_fset->doc != doc_prev) {
    std::free(doc_prev);
    rec_fset->doc = PYBIND11_COMPAT_STRDUP(rec_fset->doc);
    }
    if (!rec_active) {
    rec_active = rec_fset;
    }
    }
    def_property_static_impl(name, fget, fset, rec_active);
    return *this;
    }
  3. ... which in turn calls process_attribute<keep_alive<>>::init:
    static void init(const Args &...args, function_record *r) {
    PYBIND11_WORKAROUND_INCORRECT_MSVC_C4100(r);
    PYBIND11_WORKAROUND_INCORRECT_GCC_UNUSED_BUT_SET_PARAMETER(r);
    using expander = int[];
    (void) expander{
    0, ((void) process_attribute<typename std::decay<Args>::type>::init(args, r), 0)...};
    }
  4. Unfortunately, the process_attribute<keep_alive<>>::init is empty.
    /**
    * Process a keep_alive call policy -- invokes keep_alive_impl during the
    * pre-call handler if both Nurse, Patient != 0 and use the post-call handler
    * otherwise
    */
    template <size_t Nurse, size_t Patient>
    struct process_attribute<keep_alive<Nurse, Patient>>
    : public process_attribute_default<keep_alive<Nurse, Patient>> {
    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N != 0 && P != 0, int> = 0>
    static void precall(function_call &call) {
    keep_alive_impl(Nurse, Patient, call, handle());
    }
    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N != 0 && P != 0, int> = 0>
    static void postcall(function_call &, handle) {}
    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N == 0 || P == 0, int> = 0>
    static void precall(function_call &) {}
    template <size_t N = Nurse, size_t P = Patient, enable_if_t<N == 0 || P == 0, int> = 0>
    static void postcall(function_call &call, handle ret) {
    keep_alive_impl(Nurse, Patient, call, ret);
    }
    };

Possible solution

Just blindly pass "extra" attributes to the cpp_function constructor. I haven't yet analyzed possible side effects of this solution.

Workaround

keep_alive works when it is passed to the cpp_function constructor directly. If the maintainers choose not to update implementations, this (confusing) behavior should be documented IMO.

Related issues

#4236, #4124, #2618, ...

Reproducible example code

No response

Is this a regression? Put the last known working version here if it is.

Not a regression

@jnooree jnooree added the triage New bug, unverified label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage New bug, unverified
Projects
None yet
Development

No branches or pull requests

1 participant