Fix uneeded use of destructor after converting builtins from/into pyobject/variant

Author: touilleMan

src/godot/_lang_resource_format_loader.pxi

@@ -160,7 +160,7 @@ cdef class PythonResourceFormatLoader:
         script._py_cls = klass
         script._script_instance_info = generate_instance_info()
         # script._set_source_code(source_code.into_gd_data())
-        # # `into_gd_data()` steal the underlying Godot string, so `source_code`
+        # # `into_gd_data()` steals the underlying Godot string, so `source_code`
         # # ends up containing nothing and we'd rather destroy it early to avoid
         # # confusions.
         # del source_code

src/godot/builtins_pxd/method.pxd.j2

@@ -82,7 +82,7 @@ cdef inline {{ render_method_signature(m) }}:
 {%       if m.return_type.is_object %}
     return BaseGDObject.cast_from_object(ret)
 {%       elif m.return_type.is_variant %}
-    # Note the "steal" means we don't need a final `gdapi.gd_variant_del(&ret)`
+    # Note `gd_variant_steal_into_pyobj(&ret)` calls `ret`'s destructor
     return gd_variant_steal_into_pyobj(&ret)
 {%       else %}
     return ret
@@ -147,7 +147,7 @@ cdef inline {{ render_method_signature(m) }}:
 {%       if m.return_type.is_object %}
     return BaseGDObject.cast_from_object(ret)
 {%       elif m.return_type.is_variant %}
-    # Note the "steal" means we don't need a final `gdapi.gd_variant_del(&ret)`
+    # Note `gd_variant_steal_into_pyobj(&ret)` calls `ret`'s destructor
     return gd_variant_steal_into_pyobj(&ret)
 {%       else %}
     return ret

src/godot/builtins_pyx/conversion.pyx.j2

@@ -66,6 +66,8 @@ cdef gdextension_interface.GDExtensionVariantType py_to_gd_type(object pytype):
 #########################################################################
 
 
+# Consume the variant (i.e. the caller shouldn't call destructor on it) and
+# convert in into a proper builtin (e.g. `int`, `Vector2`, `StringName` etc.).
 cdef object gd_variant_steal_into_pyobj(gd_variant_t *gdvar):
     cdef gdextension_interface.GDExtensionVariantType gdtype = gdptrs.gdptr_variant_get_type(gdvar)
     if gdtype == gdextension_interface.GDEXTENSION_VARIANT_TYPE_OBJECT:
@@ -96,12 +98,8 @@ cdef object gd_variant_steal_into_pyobj(gd_variant_t *gdvar):
 cdef inline object _gd_variant_steal_into_pyobj_{{ builtin.cy_type }}(gd_variant_t *gdvar):
     cdef {{ builtin.cy_type }} ret = {{ builtin.cy_type }}.__new__({{ builtin.cy_type }})
     ret._gd_data = gdapi.gd_{{ builtin.snake_name }}_from_variant(gdvar)
-{% if builtin.is_stack_only %}
-    # Skip `gdapi.gd_variant_del(gdvar)` since this variant contains a builtin
-    # that doesn't have destructor.
-{% else %}
-    gdapi.gd_variant_del(gdvar)
-{% endif %}
+    # Note `gdapi.gd_{{ builtin.snake_name }}_from_variant(gdvar)` has already
+    # called `gdvar`'s destructor.
     return ret
 {% endfor %}
 
@@ -117,7 +115,17 @@ cdef object gd_variant_copy_into_pyobj(const gd_variant_t *gdvar):
 #########################################################################
 
 
-# TODO rename given stealing is not possible with GDExtension C++ orientated API
+# Consuming the Python object is a bit peculiar here: since a Python object is
+# refcounted, there is no guarantee we are the only one referencing it.
+# So this function "consumes" the Python object by:
+# - Doing nothing for Python scalar types such as `int` or `str`
+# - Doing nothing for builtin types that don't have a destructor (e.g. `Vector2`)
+# - For builtin types with a destructor (e.g. `StringName`), the destructor is called.
+#   The trick is that the destructor makes sure the object is still valid once destructed
+#   (typically freed pointer are set to NULL, and all code accessing pointers first
+#   does a null pointer check).
+#   Hence we end up with a Python object containing an empty (but valid !) Godot builtin
+#   object that will be destroyed a second time once the Python object refcount reaches zero.
 cdef bint gd_variant_steal_from_pyobj(object pyobj, gd_variant_t *gdvar):
     if pyobj is None:
         gdptrs.gdptr_variant_new_nil(gdvar)
@@ -143,6 +151,5 @@ cdef bint gd_variant_steal_from_pyobj(object pyobj, gd_variant_t *gdvar):
 cdef inline void _gd_variant_steal_from_pyobj_pystr(object pyobj, gd_variant_t *gdvar):
     cdef gd_string_t gdstr = gdapi.gd_string_from_unchecked_pystr(pyobj)
     gdvar[0] = gdapi.gd_string_into_variant(&gdstr)
-    gdapi.gd_string_del(&gdstr)
-    # into conversion steals the ownership, so don't need to call `gdapi.gd_string_del(gdstr)`
+    # Note `gd_string_into_variant(&gdstr)` has already called `gdstr`'s destructor.
 {% endmacro %}

src/godot/classes.pyx

@@ -113,7 +113,7 @@ cdef object _load_class(str name):
     cdef StringName gdname_classdb = StringName("ClassDB")
     cdef gd_object_t classdb = gdptrs.gdptr_global_get_singleton(&gdname_classdb._gd_data)
 
-    gd_name = GDString(name)
+    gd_name = StringName(name)
     parent = spec[0]
     items_spec = iter(spec[1:])
     if parent:
@@ -179,7 +179,7 @@ cdef object _load_class(str name):
                 _prop_setter,
                 _prop_index,
             ):
-                gd_prop_name = GDString(prop_name)
+                gd_prop_name = StringName(prop_name)
                 # TODO: ptrcall on getter/setter
                 @property
                 def _property(self):
@@ -205,7 +205,7 @@ cdef object _load_class(str name):
                 signal_name,
                 signal_arguments_count,
             ):
-                gd_signal_name = GDString(signal_name)
+                gd_signal_name = StringName(signal_name)
                 for _ in range(signal_arguments_count):
                     _arg_name = next(items_spec)
                     _arg_type = next(items_spec)
@@ -230,7 +230,7 @@ cdef object _load_class(str name):
                 _method_return_type,
                 method_arguments_count,
             ):
-                gd_method_name = GDString(method_name)
+                gd_method_name = StringName(method_name)
                 for _ in range(method_arguments_count):
                     _arg_name = next(items_spec)
                     _arg_type = next(items_spec)
@@ -280,21 +280,20 @@ cdef object _object_call(gd_object_t obj, str meth, list args):
     cdef gdextension_interface.GDExtensionMethodBindPtr Object_call = gdptrs.gdptr_classdb_get_method_bind(&gdname_object._gd_data, &gdname_call._gd_data, 3400424181)
 
     cdef gdextension_interface.GDExtensionInt args_with_meth_len = len(args) + 1
-    if args_with_meth_len > 9:
+    # Currently the worst method takes 14 arguments, so this hack should be enough...
+    if args_with_meth_len > 15:
         # TODO: handle this
-        gdptrs.gdptr_print_error("more than 8 params is not supported !", "_object_call", "", 0, False)
+        gdptrs.gdptr_print_error("Calling with more than 14 parameters is not supported (wtf are you calling ? :/)", "_object_call", "", 0, False)
         return None
-    cdef gd_variant_t[9] variant_args
-    cdef (gd_variant_t*)[9] variant_args_ptrs
+    cdef gd_variant_t[15] variant_args
+    cdef (gd_variant_t*)[15] variant_args_ptrs
     for i in range(args_with_meth_len):
         variant_args_ptrs[i] = &variant_args[i]
 
     # TODO: provide a helper for string name from Python str creation
     cdef gd_string_name_t meth_gdstrname = gdapi.gd_string_name_from_unchecked_pystr(meth)
     variant_args[0] = gdapi.gd_string_name_into_variant(&meth_gdstrname)
-    gdapi.gd_string_name_del(&meth_gdstrname)
-    # TODO: rename !
-    # Into conversion steals the owneship, so no need to delete meth_gdstrname
+    # Note `gd_string_name_into_variant(&meth_gdstrname)` already calls `meth_gdstrname`'s destructor
 
     for i, arg in enumerate(args, 1):
         variant_args[i] = ensure_is_gdany_and_borrow_ref(arg)
@@ -309,11 +308,10 @@ cdef object _object_call(gd_object_t obj, str meth, list args):
         &ret,
         &call_error,
     )
-    for i in range(args_with_meth_len):
-        gdapi.gd_variant_del(&variant_args[i])
-    # gdapi.gd_variant_del(&variant_args[0])  # Only param we created without stealing ownership
+    # In Godot the callee is responsible to destroy the provided parameters.
+    # Hence we don't have to call `gd_variant_del()` on `variant_args`.
     if call_error.error == gdextension_interface.GDEXTENSION_CALL_OK:
-        # No need to destroy ret given the conversion has stolen ownership on data !
+        # Note `gd_variant_steal_into_pyobj(&ret)` already calls `ret`'s destructor
         return gd_variant_steal_into_pyobj(&ret)
 
     # TODO: improve ret error raised exception type ?