Add return_policy and keep alive options, log when returning a refere…

…nce in generator

modules/python/config/visual_features.json

@@ -1,5 +1,7 @@
 {
-  "ignored_headers": ["vpFeatureException.h"],
+  "ignored_headers": [
+    "vpFeatureException.h"
+  ],
   "classes": {
     "vpGenericFeature": {
       "methods": [
@@ -20,8 +22,66 @@
         }
       ]
     },
-    "vpFeatureMomentDatabase": {
+    "vpFeatureMomentDatabase": {},
+    "vpFeatureMomentCommon": {
+      "methods": [
+        {
+          "static": false,
+          "signature": "vpFeatureMomentAlpha& getFeatureAlpha()",
+          "return_policy": "reference",
+          "keep_alive": [1, 0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentAreaNormalized& getFeatureAn()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentBasic& getFeatureMomentBasic()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentCentered& getFeatureCentered()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentCInvariant& getFeatureCInvariant()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentGravityCenterNormalized& getFeatureGravityNormalized()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentArea& getFeatureArea()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        },
+        {
+          "static": false,
+          "signature": "vpFeatureMomentGravityCenter& getFeatureGravityCenter()",
+          "return_policy": "reference",
+          "keep_alive": [1,0],
+          "returns_ref_ok": true
+        }
+      ]
     }
-
   }
 }

modules/python/generator/visp_python_bindgen/gen_report.py

@@ -51,6 +51,8 @@ def __init__(self, submodule: 'Submodule'):
     self.submodule_name = submodule.name
     self.result = {
       'ignored_headers': [],
+      'returns_ref': [],
+      'holds_pointer_or_ref': [],
       'classes': {},
       'methods': {},
       'default_param_policy_methods': [],
@@ -102,15 +104,34 @@ def add_default_policy_method(self, cls_name: str, method: types.Method, signatu
       'possible_fixes': proposed_help
     }
     self.result['default_param_policy_methods'].append(report_dict)
+  def add_method_returning_ref(self, cls_name: str, method: types.Method, signature: str) -> None:
+    proposed_help = [
+      {
+        'static': method.static,
+        'signature': signature,
+        'return_policy': 'reference',
+        'keep_alive': [[1, 0]],
+        'returns_ref_ok': True
+      },
+    ]
+    report_dict = {
+      'reason': 'Method returns a reference: this can lead to memory leaks or unwarranted copies. Ensure that keep_alive and return_policy are correctly set. If ok, set return_ref_ok to True',
+      'signature': signature,
+      'static': method.static,
+      'class': cls_name,
+      'possible_fixes': proposed_help
+    }
+    self.result['returns_ref'].append(report_dict)
 
   def write(self, path: Path) -> None:
     print('=' * 50)
     print(f'Statistics for module {self.submodule_name}:')
     stats = [
       f'Ignored headers: {len(self.result["ignored_headers"])}',
       f'Ignored classes: {len(self.result["classes"].keys())}',
-      f'Not generated methods: {len(self.result["methods"].keys())}',
-      f'Method with default parameter policy: {len(self.result["default_param_policy_methods"])}',
+      f'Ignored methods: {len(self.result["methods"].keys())}',
+      f'Methods with default parameter policy: {len(self.result["default_param_policy_methods"])}',
+      f'Methods returning a reference: {len(self.result["returns_ref"])}',
     ]
     print('\n\t', '\n\t'.join(stats), '\n', sep='')
     with open(path, 'w') as report_file:

modules/python/generator/visp_python_bindgen/methods.py

@@ -231,18 +231,45 @@ def define_method(method: types.Method, method_config: Dict, is_class_method, sp
   py_method_name = method_config.get('custom_name') or method_name
   return_type = get_type(method.return_type, specs, header_env.mapping)
 
+  method_signature = get_method_signature(method_name,
+                                          get_type(method.return_type, {}, header_env.mapping),
+                                          [get_type(param.type, {}, header_env.mapping) for param in method.parameters])
+
   # Detect input and output parameters for a method
   use_default_param_policy = method_config['use_default_param_policy']
   param_is_input, param_is_output = method_config['param_is_input'], method_config['param_is_output']
   if use_default_param_policy or param_is_input is None and param_is_output is None:
     param_is_input = [True for _ in range(len(method.parameters))]
     param_is_output = list(map(lambda param: is_non_const_ref_to_immutable_type(param.type), method.parameters))
     if any(param_is_output): # Emit a warning when using default policy
-      method_signature = get_method_signature(method_name,
-                                              get_type(method.return_type, {}, header_env.mapping),
-                                              [get_type(param.type, {}, header_env.mapping) for param in method.parameters])
       header.submodule.report.add_default_policy_method(bound_object.cpp_no_template_name, method, method_signature, param_is_input, param_is_output)
+
+  # Pybind arguments
+  # Only use py::arg for input values (error otherwise)
   py_arg_strs = [py_arg_strs[i] for i in range(len(params_strs)) if param_is_input[i]]
+
+  pybind_options = py_arg_strs
+  # Custom return policy
+  return_policy = method_config.get('return_policy')
+  if return_policy is not None:
+    pybind_options.append(f'py::return_value_policy::{return_policy}')
+
+  # Keep alive values: avoid memory leaks
+  keep_alives = method_config.get('keep_alive')
+  keep_alive_strs = []
+  if keep_alives is not None:
+    def make_keep_alive_str(values) -> str:
+      assert len(values) == 2 and isinstance(values[0], int) and isinstance(values[1], int), 'Tried to make keep alive with incorrect values'
+      return f'py::keep_alive<{values[0]}, {values[1]}>()'
+    if not isinstance(keep_alives, list) or len(keep_alives) == 0:
+      raise RuntimeError(f'Keep alive value should be a list of int or a list of list of ints (multiple args kept alive), but got {keep_alives} for method {method_signature}')
+    if isinstance(keep_alives[0], int):
+      keep_alive_strs.append(make_keep_alive_str(keep_alives))
+    else:
+      for keep_alive in keep_alives:
+        keep_alive_strs.append(make_keep_alive_str(keep_alive))
+  pybind_options.extend(keep_alive_strs)
+
   # Get parameter names
   param_names = [param.name or 'arg' + str(i) for i, param in enumerate(method.parameters)]
   input_param_names = [param_names[i] for i in range(len(param_is_input)) if param_is_input[i]]
@@ -265,11 +292,19 @@ def define_method(method: types.Method, method_config: Dict, is_class_method, sp
     if method_doc is None:
       logging.warning(f'Could not find documentation for {bound_object.cpp_name}::{method_name}!')
     else:
-      py_arg_strs = [method_doc.documentation] + py_arg_strs
+      pybind_options = [method_doc.documentation] + pybind_options
+
+
 
   # If a function has refs to immutable params, we need to return them.
+  # Also true if user has specified input cpp params as output python params
   should_wrap_for_tuple_return = param_is_output is not None and any(param_is_output)
 
+  # Emit a warning when returnin a ref to an object:
+  # this can probably lead to memory leaks or unwanted object copies (and thus undesired behaviour down the line)
+  if not should_wrap_for_tuple_return and '&' in return_type and not (method_config.get('returns_ref_ok') or False):
+    header.submodule.report.add_method_returning_ref(bound_object.cpp_no_template_name, method, method_signature)
+
   # Arguments that are inputs to the lambda function that wraps the ViSP function
   input_param_types = [params_strs[i] for i in range(len(param_is_input)) if param_is_input[i]]
   params_with_names = [t + ' ' + name for t, name in zip(input_param_types, input_param_names)]
@@ -304,7 +339,7 @@ def define_method(method: types.Method, method_config: Dict, is_class_method, sp
   else:
     # When returning a tuple we need to explicitly convert references to pointer.
     # This is required since std::tuple will upcast the ref to its base class and try to store a copy of the object
-    # If a class is pure virtual, this is not possible and will a compilation error!
+    # If a class is pure virtual, this is not possible and will raise a compilation error!
     output_param_symbols.extend(['&' + name if is_ref else name for is_ref, name in zip(output_param_is_ref, output_param_names)])
     return_str = f'std::make_tuple({", ".join(output_param_symbols)})'
 
@@ -323,9 +358,9 @@ def define_method(method: types.Method, method_config: Dict, is_class_method, sp
   else:
     method_body_str = ref_to_function(bound_object.cpp_name + method_name, return_type, params_strs)
 
-  method_str = method_def(py_method_name, method_body_str, py_arg_strs, method.static if is_class_method else False)
+  method_str = method_def(py_method_name, method_body_str, pybind_options, method.static if is_class_method else False)
   method_str = f'{bound_object.python_ident}.{method_str};'
-  return method_str, MethodData(py_method_name, method, lambda_variant, py_arg_strs)
+  return method_str, MethodData(py_method_name, method, lambda_variant, pybind_options)
 
 def define_constructor(params: List[str], additional_args: List[str]) -> str:
   additional_args_str = ', '.join(additional_args)

modules/python/generator/visp_python_bindgen/submodule.py

@@ -174,6 +174,7 @@ def get_user_defined_headers(self) -> List[str]:
       header_names = self.config['user_defined_headers']
       return [f'#include "{header_name}"' for header_name in header_names]
     return []
+
   def get_required_headers(self) -> List[str]:
     if 'required_headers' in self.config:
       header_names = self.config['required_headers']
@@ -210,11 +211,14 @@ def get_enum_config(self, enum_name: str) -> Optional[Dict]:
   def get_method_config(self, class_name: Optional[str], method, owner_specs, header_mapping) -> Dict:
     res = {
       'ignore': False,
-      'use_default_param_policy': False,
+      'use_default_param_policy': False, # Handling
       'param_is_input': None,
       'param_is_output': None,
       'custom_name': None,
-      'custom_code': None
+      'custom_code': None,
+      'keep_alive': None,
+      'return_policy': None,
+      'returns_ref_ok': False,
     }
     functions_container = None
     keys = ['classes', class_name, 'methods'] if class_name is not None else ['functions']