Skip to content

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

Open
@jnooree

Description

@jnooree

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    triageNew bug, unverified

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions