[Core] Fix Variant::construct of Object

[AThousandShips]

Variant type was not updated correctly causing leaks in ref-counted

This caused errors as objects constructed this way would be typed NIL and therefore not be freed, even though they went through reference counting, might have caused further issues elsewhere, like extensions, but haven't tested, this should be a safe fix as it just clears and initializes the type

• Fixes: Memory leak when using weakref() #90086
• Fixes: AnimationPlayer: Call Method track, unable to select Object type in Args of AnimationTrackKeyEdit inspector #91289

[AThousandShips]

Moving this below matches the rest of the cases, where errors mean we don't touch it, can move it back above if desired, but take VariantConstructorNilObject for example

[AThousandShips]

The reason it breaks GDScript in this case is that this code passes fine, as value is NIL, which is valid for that check, a NIL variant is a valid type for an object here, now it will correctly error, as the resulting variant is now a valid OBJECT so it will go through the checks and correctly determine the type is wrong:

godot/modules/gdscript/gdscript.cpp

Line 1609 in 29b3d9e

if (err.error != Callable::CallError::CALL_OK || !member->data_type.is_type(value)) {

[AThousandShips]

There's still a strange leak when using set directly, but unsure what is causing that, it would be in some part of the code unrelated to this, will dig further but seems a different case

Edit: Got a fix for this, part too, but will look at some other potential areas as well

Edit 2: Realized why, will improve

This issue was caused by that Variant::construct didn't clear properly with just change as it doesn't do anything if the types match, so when Variant value in GDScriptInstance::set was already assigned with p_value the change operation did nothing, so value was never cleared, and therefore leaked

[AThousandShips]

There, new fix, this is perhaps not exactly the cleanest, but I prefer to use the methods available there instead of assigning directly to the type, an alterative solution would be to instead (diff on top of this PR):

diff --git a/core/variant/variant_construct.h b/core/variant/variant_construct.h
index 40e0d37360..b5cd7ff1e7 100644
--- a/core/variant/variant_construct.h
+++ b/core/variant/variant_construct.h
@@ -154,12 +154,10 @@ class VariantConstructorObject {
 public:
        static void construct(Variant &r_ret, const Variant **p_args, Callable::CallError &r_error) {
                if (p_args[0]->get_type() == Variant::NIL) {
-                       VariantInternal::clear(&r_ret);
                        VariantTypeChanger<Object *>::change(&r_ret);
                        VariantInternal::object_assign_null(&r_ret);
                        r_error.error = Callable::CallError::CALL_OK;
                } else if (p_args[0]->get_type() == Variant::OBJECT) {
-                       VariantInternal::clear(&r_ret);
                        VariantTypeChanger<Object *>::change(&r_ret);
                        VariantInternal::object_assign(&r_ret, p_args[0]);
                        r_error.error = Callable::CallError::CALL_OK;
@@ -171,7 +169,6 @@ public:
        }

        static inline void validated_construct(Variant *r_ret, const Variant **p_args) {
-               VariantInternal::clear(r_ret);
                VariantTypeChanger<Object *>::change(r_ret);
                VariantInternal::object_assign(r_ret, p_args[0]);
        }
@@ -201,13 +198,11 @@ public:
                        r_error.expected = Variant::NIL;
                }

-               VariantInternal::clear(&r_ret);
                VariantTypeChanger<Object *>::change(&r_ret);
                VariantInternal::object_assign_null(&r_ret);
        }

        static inline void validated_construct(Variant *r_ret, const Variant **p_args) {
-               VariantInternal::clear(r_ret);
                VariantTypeChanger<Object *>::change(r_ret);
                VariantInternal::object_assign_null(r_ret);
        }
diff --git a/core/variant/variant_internal.h b/core/variant/variant_internal.h
index dbd4a6a7ad..d867d08b2c 100644
--- a/core/variant/variant_internal.h
+++ b/core/variant/variant_internal.h
@@ -1493,13 +1493,13 @@ struct VariantDefaultInitializer<PackedColorArray> {
 template <typename T>
 struct VariantTypeChanger {
        static _FORCE_INLINE_ void change(Variant *v) {
-               if (v->get_type() != GetTypeInfo<T>::VARIANT_TYPE || GetTypeInfo<T>::VARIANT_TYPE >= Variant::PACKED_BYTE_ARRAY) { //second condition removed by optimizer
+               if (v->get_type() != GetTypeInfo<T>::VARIANT_TYPE || GetTypeInfo<T>::VARIANT_TYPE == Variant::OBJECT || GetTypeInfo<T>::VARIANT_TYPE >= Variant::PACKED_BYTE_ARRAY) { //second condition removed by optimizer
                        VariantInternal::clear(v);
                        VariantInitializer<T>::init(v);
                }
        }
        static _FORCE_INLINE_ void change_and_reset(Variant *v) {
-               if (v->get_type() != GetTypeInfo<T>::VARIANT_TYPE || GetTypeInfo<T>::VARIANT_TYPE >= Variant::PACKED_BYTE_ARRAY) { //second condition removed by optimizer
+               if (v->get_type() != GetTypeInfo<T>::VARIANT_TYPE || GetTypeInfo<T>::VARIANT_TYPE == Variant::OBJECT || GetTypeInfo<T>::VARIANT_TYPE >= Variant::PACKED_BYTE_ARRAY) { //second condition removed by optimizer
                        VariantInternal::clear(v);
                        VariantInitializer<T>::init(v);
                }

And just use the change method without clear like the other cases use, this would be slightly more invasive, but accomplish the same goal, either way works for me (though the current PR changes are cheaper in rebuilds)

Will see if I can add a test for leaks as well

Edit: Added a test for the leak, will push when CI is done

[vnen]

LGTM. Should be safe to merge too.

[akien-mga]

Thanks!

[AThousandShips]

Thank you!