Repository: RosettaCommons/binder (language: python)

Issue:
Support for pybind11 smart_holder
Hello,

For one of the projects for which I am using Binder, we need to use the smart_holder branch of pybindd11 ( https://github.com/pybind/pybind11/tree/smart_holder ). The differences, in term of pybindd11 code associated to the interface, are very minimal .

Would it be fine if I create during the next weeks a PR to binder to add an option to generate pybindd11 codes that use the smart_holder branch? Could this extra feature be accepted?

Thanks!


Discussion / hints:
I am open to the idea assuming that switching logic could be implemented neatly so it does not make code much harder to maintain. Do you already know what kind of changes in generated code this command-line will trigger? If so, could you please outline them here so it is possible to look at the concrete example? Thanks.
Thanks @lyskov for your comments!

(This issue https://github.com/pybind/pybind11/pull/2839 is a good extra context of why it could be useful)

Up to now, I have been using the [conservative mode](https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst#conservative-mode) of the smart holder branch.

So the code: 

```
#include <pybind11/pybind11.h>

struct Foo {};

PYBIND11_MODULE(example_bindings, m) {
    namespace py = pybind11;
    py::class_<Foo>(m, "Foo");
}
```

would now be replaced by:

```
#include <pybind11/smart_holder.h>

struct Foo {};

PYBIND11_SMART_HOLDER_TYPE_CASTERS(Foo)

PYBIND11_MODULE(example_bindings, m) {
    namespace py = pybind11;
    py::classh<Foo>(m, "Foo");
}
```

In my previous successful tests, I have been using `py::classh` and `PYBIND11_SMART_HOLDER_TYPE_CASTERS` only for the classes for which it was useful.

Concretely, for binder, I was thinking of adding a configuration option that can be enabled per class like this:
```
+use_smart_holder_for_class Foo
```
If the option is set, then we would add `PYBIND11_SMART_HOLDER_TYPE_CASTERS` and use `py::classh` for that class.
And if there is at least one `use_smart_holder_for_class`, we would use `#include <pybind11/smart_holder.h>` instead of `#include <pybind11/pybind11.h>`.

What do you think of this?

@kliegeois this looks quite reasonable to me, pull-request implementing this will be welcome! Could you please also include test in your PR? If so please take a look at https://github.com/RosettaCommons/binder/blob/master/test/T81.custom_trampoline_with_args.config test on how to specify specific config directive for particular test. Thanks,

Also, thank you for mentioning https://github.com/pybind/pybind11/pull/2839 issue! I noticed similar issue in my project and having general solution for them would great!
Thanks for your comment @lyskov !
I will start working on this and will ping you on the PR once it is ready for review.

Fix patch:
diff --git a/documentation/config.rst b/documentation/config.rst
index ac86350e..60b6f178 100644
--- a/documentation/config.rst
+++ b/documentation/config.rst
@@ -241,3 +241,16 @@ Config file directives:
 
 
 * ``+prefix_for_static_member_functions``: specify name prefix to use for static member functions, could be useful as workaround Pybind11 limitation restricting having both virtual and static member functions having the same name
+
+* ``smart_holder``: use to specify that a class requires the usage of the progressive mode of the pybind11 smart_holder branch (https://github.com/pybind/pybind11/tree/smart_holder). As discussed in https://github.com/pybind/pybind11/blob/smart_holder/README_smart_holder.rst, the smart_holder branch is a strict superset of the pybind11 master branch that supports safely passing trampoline objects back to C++: associated Python objects are automatically kept alive for the lifetime of the smart-pointer. This config file directive has been added to fulfil https://github.com/RosettaCommons/binder/issues/263.
+
+.. code-block:: bash
+
+  +smart_holder example::class
+
+* ``pybind11_include_file``: use to specify which header file of pybind11 should be included. The header pybind11/pybind11.h is used by default.
+
+.. code-block:: bash
+
+  +pybind11_include_file pybind11/smart_holder.h
+
diff --git a/source/class.cpp b/source/class.cpp
index 926b6862..4ff4688e 100644
--- a/source/class.cpp
+++ b/source/class.cpp
@@ -1258,11 +1258,17 @@ void ClassBinder::bind(Context &context)
 
 	std::string extra_annotation = module_local_annotation + buffer_protocol_annotation;
 
-	if( named_class )
+	if( named_class ) {
+		if (Config::get().is_smart_holder_requested(qualified_name_without_template)) {
+			c += '\t' +
+				R"(PYBIND11_TYPE_CASTER_BASE_HOLDER({} {}))"_format(qualified_name, maybe_holder_type) +
+				'\n';
+		}
 		c += '\t' +
 			 R"(pybind11::class_<{}{}{}{}> cl({}, "{}", "{}"{});)"_format(qualified_name, maybe_holder_type, maybe_trampoline, maybe_base_classes(context), module_variable_name, python_class_name(C),
 																		generate_documentation_string_for_declaration(C), extra_annotation) +
 			 '\n';
+	}
 	// c += "\tpybind11::handle cl_type = cl;\n\n";
 
 	// if( C->isAbstract()  and  callback_structure) c += "\tcl.def(pybind11::init<>());\n";
diff --git a/source/config.cpp b/source/config.cpp
index 86e525a1..4c70aa9c 100644
--- a/source/config.cpp
+++ b/source/config.cpp
@@ -77,6 +77,10 @@ void Config::read(string const &file_name)
 
 	string const _custom_shared_{"custom_shared"};
 
+	string const _smart_holder_{"smart_holder"};
+
+	string const _pybind11_include_file_{"pybind11_include_file"};
+
 	string const _default_static_pointer_return_value_policy_{"default_static_pointer_return_value_policy"};
 	string const _default_static_lvalue_reference_return_value_policy_{"default_static_lvalue_reference_return_value_policy"};
 	string const _default_static_rvalue_reference_return_value_policy_{"default_static_rvalue_reference_return_value_policy"};
@@ -216,6 +220,16 @@ void Config::read(string const &file_name)
 		}
 		else if( token == _custom_shared_ ) holder_type_ = name_without_spaces;
 
+		else if( token == _smart_holder_ ) {
+			if(bind) {
+				smart_held_classes.push_back(name_without_spaces);
+			}
+		}
+
+		else if( token == _pybind11_include_file_ ) {
+			pybind11_include_file_ = name_without_spaces;
+		}
+
 		else if( token == _default_static_pointer_return_value_policy_ ) default_static_pointer_return_value_policy_ = name_without_spaces;
 		else if( token == _default_static_lvalue_reference_return_value_policy_ ) default_static_lvalue_reference_return_value_policy_ = name_without_spaces;
 		else if( token == _default_static_rvalue_reference_return_value_policy_ ) default_static_rvalue_reference_return_value_policy_ = name_without_spaces;
@@ -444,6 +458,21 @@ bool Config::is_module_local_requested(string const &namespace_) const
 	return false;
 }
 
+bool Config::is_smart_holder_requested(string const &class__) const
+{
+	string class_{class__};
+	class_.erase(std::remove(class_.begin(), class_.end(), ' '), class_.end());
+
+	auto smart_held_class = std::find(smart_held_classes.begin(), smart_held_classes.end(), class_);
+
+	if( smart_held_class != smart_held_classes.end() ) {
+		// outs() << "Using smart holder for class : " << class_ << "\n";
+		return true;
+	}
+
+	return false;
+}
+
 bool Config::is_include_skipping_requested(string const &include) const
 {
 	for( auto &i : includes_to_skip )
diff --git a/source/config.hpp b/source/config.hpp
index 46418d7f..784e6077 100644
--- a/source/config.hpp
+++ b/source/config.hpp
@@ -52,6 +52,7 @@ class Config
 	string default_member_rvalue_reference_return_value_policy_ = "pybind11::return_value_policy::automatic";
 	string default_call_guard_ = "";
 	string holder_type_ = "std::shared_ptr";
+	string pybind11_include_file_ = "pybind11/pybind11.h";
 	string prefix_for_static_member_functions_ = "";
 
 	std::vector<string> enums_to_bind, enums_to_skip;
@@ -65,7 +66,7 @@ class Config
 	string root_module;
 
 	std::vector<string> namespaces_to_bind, classes_to_bind, functions_to_bind, namespaces_to_skip, classes_to_skip, functions_to_skip, includes_to_add, includes_to_skip, fields_to_skip;
-	std::vector<string> buffer_protocols, module_local_namespaces_to_add, module_local_namespaces_to_skip;
+	std::vector<string> buffer_protocols, module_local_namespaces_to_add, module_local_namespaces_to_skip, smart_held_classes;
 
 	std::map<string, string> const &binders() const { return binders_; }
 	std::map<string, string> const &add_on_binders() const { return add_on_binders_; }
@@ -90,6 +91,7 @@ class Config
 	string const &prefix_for_static_member_functions() { return prefix_for_static_member_functions_; }
 
 	string const &holder_type() const { return holder_type_; }
+	string const &pybind11_include_file() const { return pybind11_include_file_; }
 
 	string prefix;
 
@@ -111,6 +113,8 @@ class Config
 
 	bool is_buffer_protocol_requested(string const &class_) const;
 
+	bool is_smart_holder_requested(string const &class_) const;
+
 	bool is_include_skipping_requested(string const &include) const;
 
 	string is_custom_trampoline_function_requested(string const &function__) const;
diff --git a/source/context.cpp b/source/context.cpp
index 2a945e13..d7d524cf 100644
--- a/source/context.cpp
+++ b/source/context.cpp
@@ -87,14 +87,14 @@ const char *main_module_header = R"_(#include <map>
 #include <stdexcept>
 #include <string>
 
-#include <pybind11/pybind11.h>
+{0}
 
 typedef std::function< pybind11::module & (std::string const &) > ModuleGetter;
 
-{0}
+{1}
 
-PYBIND11_MODULE({1}, root_module) {{
-	root_module.doc() = "{1} module";
+PYBIND11_MODULE({2}, root_module) {{
+	root_module.doc() = "{2} module";
 
 	std::map <std::string, pybind11::module> modules;
 	ModuleGetter M = [&](std::string const &namespace_) -> pybind11::module & {{
@@ -115,25 +115,25 @@ PYBIND11_MODULE({1}, root_module) {{
 	);
 
 	std::vector< std::pair<std::string, std::string> > sub_modules {{
-{2}	}};
+{3}	}};
 	for(auto &p : sub_modules ) modules[p.first.size() ? p.first+"::"+p.second : p.second] = modules[p.first].def_submodule( mangle_namespace_name(p.second).c_str(), ("Bindings for " + p.first + "::" + p.second + " namespace").c_str() );
 
 	//pybind11::class_<std::shared_ptr<void>>(M(""), "_encapsulated_data_");
 
-{3}
+{4}
 }}
 )_";
 
 const char *module_header = R"_(
 #include <functional>
-#include <pybind11/pybind11.h>
+{0}
 #include <string>
-{}
+{1}
 #ifndef BINDER_PYBIND11_TYPE_CASTER
 	#define BINDER_PYBIND11_TYPE_CASTER
-	{}
+	{2}
 	PYBIND11_DECLARE_HOLDER_TYPE(T, T*)
-	{}
+	{3}
 #endif
 
 )_";
@@ -437,7 +437,8 @@ void Context::generate(Config const &config)
 		string shared_declare = "PYBIND11_DECLARE_HOLDER_TYPE(T, "+holder_type+"<T>)";
 		string shared_make_opaque = "PYBIND11_MAKE_OPAQUE("+holder_type+"<void>)";
 
-		code = generate_include_directives(includes) + fmt::format(module_header, config.includes_code(), shared_declare, shared_make_opaque) + prefix_code + "void " + function_name + module_function_suffix + "\n{\n" + code + "}\n";
+		string const pybind11_include = "#include <" + Config::get().pybind11_include_file() + ">";
+		code = generate_include_directives(includes) + fmt::format(module_header, pybind11_include, config.includes_code(), shared_declare, shared_make_opaque) + prefix_code + "void " + function_name + module_function_suffix + "\n{\n" + code + "}\n";
 
 		if( O_single_file ) root_module_file_handle << "// File: " << file_name << '\n' << code << "\n\n";
 		else update_source_file(config.prefix, file_name, code);
@@ -462,8 +463,10 @@ void Context::generate(Config const &config)
 		binding_function_calls += "\t" + f + "(M);\n";
 	}
 
+	string const pybind11_include = "#include <" + Config::get().pybind11_include_file() + ">";
+
 	std::stringstream s;
-	s << fmt::format(main_module_header, binding_function_decls, config.root_module, namespace_pairs, binding_function_calls);
+	s << fmt::format(main_module_header, pybind11_include, binding_function_decls, config.root_module, namespace_pairs, binding_function_calls);
 
 	root_module_file_handle << s.str();
 


Test patch:
diff --git a/test/T61.smart_holder.config b/test/T61.smart_holder.config
new file mode 100644
index 00000000..5f585706
--- /dev/null
+++ b/test/T61.smart_holder.config
@@ -0,0 +1,3 @@
++custom_shared my_shared_ptr
++smart_holder A
++pybind11_include_file pybind11/smart_holder.h
\ No newline at end of file
diff --git a/test/T61.smart_holder.hpp b/test/T61.smart_holder.hpp
new file mode 100644
index 00000000..737420cf
--- /dev/null
+++ b/test/T61.smart_holder.hpp
@@ -0,0 +1,54 @@
+// -*- mode:c++;tab-width:2;indent-tabs-mode:t;show-trailing-whitespace:t;rm-trailing-spaces:t -*-
+// vi: set ts=2 noet:
+//
+// Copyright (c) 2016 Sergey Lyskov <sergey.lyskov@jhu.edu>
+//
+// All rights reserved. Use of this source code is governed by a
+// MIT license that can be found in the LICENSE file.
+
+/// @file   binder/test/T01.enum.hpp
+/// @brief  Binder self-test file. Bindings of enum's functionality.
+/// @author Sergey Lyskov
+
+#ifndef _INCLUDED_T60_hpp_
+#define _INCLUDED_T60_hpp_
+
+#include <memory>
+
+template<typename T>
+using my_shared_ptr = std::shared_ptr<T>;
+
+enum E1 { E1_V0, E1_V1 };
+
+enum struct E2_struct { V0, V1 };
+enum class E3_class { V0, V1 };
+
+
+template<typename T>
+class A
+{
+public:
+	enum AE1 { AE1_V0, AE1_V1 };
+	enum struct AE2_struct { AE3_V0, AE3_V1 };
+	enum class AE3_class { AE2_V0, AE2_V1 };
+
+protected:
+	enum AE3_not_binded { AE3_V0_not_binded, AE3_V1_not_binded };
+	enum class AE4_not_binded { AE4_V0_not_binded, AE4_V1_not_binded };
+
+private:
+	enum AE5_not_binded { AE5_V0_not_binded, AE5_V1_not_binded };
+	enum class AE6_not_binded { AE6_V0_not_binded, AE6_V1_not_binded };
+};
+
+void fi_instantiated_by_use_in_function(A<int>)
+{
+}
+void fi(A<int> &)
+{
+}
+void fi(A<int> *)
+{
+}
+
+#endif // _INCLUDED_T01_enum_hpp_
diff --git a/test/T61.smart_holder.ref b/test/T61.smart_holder.ref
new file mode 100644
index 00000000..9bc19f86
--- /dev/null
+++ b/test/T61.smart_holder.ref
@@ -0,0 +1,126 @@
+// File: T61_smart_holder.cpp
+#include <T61.smart_holder.hpp> // A
+#include <T61.smart_holder.hpp> // E1
+#include <T61.smart_holder.hpp> // E2_struct
+#include <T61.smart_holder.hpp> // E3_class
+#include <T61.smart_holder.hpp> // fi
+#include <T61.smart_holder.hpp> // fi_instantiated_by_use_in_function
+#include <sstream> // __str__
+
+#include <functional>
+#include <pybind11/smart_holder.h>
+#include <string>
+
+#ifndef BINDER_PYBIND11_TYPE_CASTER
+	#define BINDER_PYBIND11_TYPE_CASTER
+	PYBIND11_DECLARE_HOLDER_TYPE(T, my_shared_ptr<T>)
+	PYBIND11_DECLARE_HOLDER_TYPE(T, T*)
+	PYBIND11_MAKE_OPAQUE(my_shared_ptr<void>)
+#endif
+
+void bind_T61_smart_holder(std::function< pybind11::module &(std::string const &namespace_) > &M)
+{
+	// E1 file:T61.smart_holder.hpp line:21
+	pybind11::enum_<E1>(M(""), "E1", pybind11::arithmetic(), "")
+		.value("E1_V0", E1_V0)
+		.value("E1_V1", E1_V1)
+		.export_values();
+
+;
+
+	// E2_struct file:T61.smart_holder.hpp line:23
+	pybind11::enum_<E2_struct>(M(""), "E2_struct", "")
+		.value("V0", E2_struct::V0)
+		.value("V1", E2_struct::V1)
+		.export_values();
+
+;
+
+	// E3_class file:T61.smart_holder.hpp line:24
+	pybind11::enum_<E3_class>(M(""), "E3_class", "")
+		.value("V0", E3_class::V0)
+		.value("V1", E3_class::V1);
+
+;
+
+	{ // A file:T61.smart_holder.hpp line:28
+		PYBIND11_TYPE_CASTER_BASE_HOLDER(A<int> , my_shared_ptr<A<int>>)
+		pybind11::class_<A<int>, my_shared_ptr<A<int>>> cl(M(""), "A_int_t", "");
+		cl.def( pybind11::init( [](){ return new A<int>(); } ) );
+
+		pybind11::enum_<A<int>::AE1>(cl, "AE1", pybind11::arithmetic(), "")
+			.value("AE1_V0", A<int>::AE1_V0)
+			.value("AE1_V1", A<int>::AE1_V1)
+			.export_values();
+
+
+		pybind11::enum_<A<int>::AE2_struct>(cl, "AE2_struct", "")
+			.export_values();
+
+
+		pybind11::enum_<A<int>::AE3_class>(cl, "AE3_class", "");
+
+	}
+	// fi_instantiated_by_use_in_function(class A<int>) file:T61.smart_holder.hpp line:44
+	M("").def("fi_instantiated_by_use_in_function", (void (*)(class A<int>)) &fi_instantiated_by_use_in_function, "C++: fi_instantiated_by_use_in_function(class A<int>) --> void", pybind11::arg(""));
+
+	// fi(class A<int> &) file:T61.smart_holder.hpp line:47
+	M("").def("fi", (void (*)(class A<int> &)) &fi, "C++: fi(class A<int> &) --> void", pybind11::arg(""));
+
+	// fi(class A<int> *) file:T61.smart_holder.hpp line:50
+	M("").def("fi", (void (*)(class A<int> *)) &fi, "C++: fi(class A<int> *) --> void", pybind11::arg(""));
+
+}
+
+
+#include <map>
+#include <algorithm>
+#include <functional>
+#include <memory>
+#include <stdexcept>
+#include <string>
+
+#include <pybind11/smart_holder.h>
+
+typedef std::function< pybind11::module & (std::string const &) > ModuleGetter;
+
+void bind_T61_smart_holder(std::function< pybind11::module &(std::string const &namespace_) > &M);
+
+
+PYBIND11_MODULE(T61_smart_holder, root_module) {
+	root_module.doc() = "T61_smart_holder module";
+
+	std::map <std::string, pybind11::module> modules;
+	ModuleGetter M = [&](std::string const &namespace_) -> pybind11::module & {
+		auto it = modules.find(namespace_);
+		if( it == modules.end() ) throw std::runtime_error("Attempt to access pybind11::module for namespace " + namespace_ + " before it was created!!!");
+		return it->second;
+	};
+
+	modules[""] = root_module;
+
+	static std::vector<std::string> const reserved_python_words {"nonlocal", "global", };
+
+	auto mangle_namespace_name(
+		[](std::string const &ns) -> std::string {
+			if ( std::find(reserved_python_words.begin(), reserved_python_words.end(), ns) == reserved_python_words.end() ) return ns;
+			else return ns+'_';
+		}
+	);
+
+	std::vector< std::pair<std::string, std::string> > sub_modules {
+	};
+	for(auto &p : sub_modules ) modules[p.first.size() ? p.first+"::"+p.second : p.second] = modules[p.first].def_submodule( mangle_namespace_name(p.second).c_str(), ("Bindings for " + p.first + "::" + p.second + " namespace").c_str() );
+
+	//pybind11::class_<std::shared_ptr<void>>(M(""), "_encapsulated_data_");
+
+	bind_T61_smart_holder(M);
+
+}
+
+// Source list file: TEST/T61_smart_holder.sources
+// T61_smart_holder.cpp
+// T61_smart_holder.cpp
+
+// Modules list file: TEST/T61_smart_holder.modules
+//