GDScript: Set has_type false if it is BUILTIN but Variant::NIL

[emre0altan]

This change fixes #87932 by preventing edge case for null value when used with a match. Even though edge case is happening with third check in IS_BUILTIN_TYPE(), m_var.type.builtin_type == m_type, there is no other default type other than Variant::NIL, so this check wouldn't change. But returning false with has_type in the first check for Variant::NIL makes more sense since it is also the default value in GDScriptDataType.

godot/modules/gdscript/gdscript_byte_codegen.cpp

Lines 430 to 434 in b4e2a24

	#define IS_BUILTIN_TYPE(m_var, m_type) \
	(m_var.type.has_type && m_var.type.kind == GDScriptDataType::BUILTIN && m_var.type.builtin_type == m_type)
	void GDScriptByteCodeGenerator::write_type_adjust(const Address &p_target, Variant::Type p_new_type) {

godot/modules/gdscript/gdscript_function.h

Lines 47 to 65 in b4e2a24

	class GDScriptDataType {
	public:
	Vector<GDScriptDataType> container_element_types;
	enum Kind {
	UNINITIALIZED,
	BUILTIN,
	NATIVE,
	SCRIPT,
	GDSCRIPT,
	};
	Kind kind = UNINITIALIZED;
	bool has_type = false;
	Variant::Type builtin_type = Variant::NIL;
	StringName native_type;
	Script *script_type = nullptr;
	Ref<Script> script_type_ref;

[dalexeev]

This makes sense since BUILTIN types cannot accept null value and there is no Nil type in GDScript. But in my opinion, this should be fixed first in the analyzer (GDScriptAnalyzer::type_from_variant()). In the compiler we can add a check in case of a bug in the analyzer, like here:

https://github.com/godotengine/godot/blob/2e684ef4f7e706ac3990e7431db84e30a4d81f3e/modules/gdscript/gdscript_compiler.cpp#L121-L126

Plus I think it's worth adding a test.

[emre0altan]

I have looked what is happening in GDScriptAnalyzer::type_from_variant() and can't really say anything that needs to be changed there. Btw, also I am fairly new to the codebase. So, It returns BUILTIN data type which is ANNOTATED_EXPLICIT and doesn't have has_type property yet, it will be added when converted to GDScriptDataType. There is no other type other than Variant::NIL that can represent null and it also stated as a built in type in here: https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html#null

I think the main problem is with IS_BUILTIN_TYPE() macro returning false except if the argument is Variant::NIL.

[dalexeev]

There is no other type other than Variant::NIL that can represent null

Sorry, I was thinking that we could use Variant::OBJECT, but I forgot that it is also not suitable for BUILTIN, and NATIVE/SCRIPT/CLASS is ambiguous for null, it can cause unexpected consequences. So I guess we can leave it as is, just add a comment and test.

Or, if look at it differently, GDScript has an implicit type Nil, it's just that the user can't specify it. Perhaps the bug can be fixed differently, in GDScriptByteCodeGenerator.

[emre0altan]

I think the one you referring is this one https://docs.godotengine.org/en/3.1/classes/class_nil.html and probably it's the default version of Variant with default NIL type. My gut feeling says that only difference from the other types is just null is more commonly used than nil and being used as null. But problem arises when NIL is treated the same way with the other types and also being used as the default type like in Variant::get_utility_function_argument_type(). Later if you used it to check something, then there will be an edge case for NIL.

godot/core/variant/variant_utility.cpp

Lines 1922 to 1929 in d335281

	Variant::Type Variant::get_utility_function_argument_type(const StringName &p_name, int p_arg) {
	const VariantUtilityFunctionInfo *bfi = utility_function_table.lookup_ptr(p_name);
	if (!bfi) {
	return Variant::NIL;
	}
	return bfi->get_arg_type(p_arg);
	}

[dalexeev]

I'd prefer to solve it here:

godot/modules/gdscript/gdscript_byte_codegen.cpp

Line 1086 in b4e2a24

if (!IS_BUILTIN_TYPE(p_arguments[i], Variant::get_utility_function_argument_type(p_function, i))) {

-			if (!IS_BUILTIN_TYPE(p_arguments[i], Variant::get_utility_function_argument_type(p_function, i))) {
+			const Variant::Type arg_type = Variant::get_utility_function_argument_type(p_function, i);
+			// `Variant::get_utility_function_argument_type()` returns `Variant::NIL` for `Variant` arguments.
+			if (arg_type == Variant::NIL || !IS_BUILTIN_TYPE(p_arguments[i], arg_type)) {

We have some problem distinguishing between Nil and Variant (there is PROPERTY_USAGE_NIL_IS_VARIANT). In some cases, Variant::NIL is used as the default value, in others Variant::VARIANT_MAX.

[dalexeev]

Looks good to me, but I'd appreciate a double check. We fixed the crash for now, but I'm not sure there aren't similar cases elsewhere. Perhaps we should change the IS_BUILTIN_TYPE macro or use Variant::VARIANT_MAX as the default value instead of Variant::NIL.

[emre0altan]

Thanks, I'll check it again. I agree, it could be better to add it to IS_BUILTIN_TYPE and I was mistaken about where the default value comes from. It returns NIL but it didn't change when I changed the return value to VARIANT_MAX here:

godot/core/variant/variant_utility.cpp

Lines 1922 to 1929 in d335281

	Variant::Type Variant::get_utility_function_argument_type(const StringName &p_name, int p_arg) {
	const VariantUtilityFunctionInfo *bfi = utility_function_table.lookup_ptr(p_name);
	if (!bfi) {
	return Variant::NIL;
	}
	return bfi->get_arg_type(p_arg);
	}

I tried to trace the registered function but it got complicated quickly. I'll have a second look there and try to find out if it's better to return VARIANT_MAX.

[emre0altan]

I think that if a method has a Variant parameter, this parameter returns Variant::NIL from get_arg_type() and that is where it is used as a default value. But it is not only for typeof method, it is like this for any method with a Variant parameter. So I don't think we can change the default value but I think it is better to fix this issue by changing IS_BUILTIN_TYPE macro.

https://github.com/godotengine/godot/blob/master/doc/classes/%40GlobalScope.xml#L1409-L1425

[vnen]

There's an extra problem here that we cannot properly validate the call if the argument is of type Variant, because the function itself might restrict which types are actually acceptable and there's no API to know that. In the case of typeof, which is what is being triggered here, it actually accepts any type, so it would be fine to do a validated call.

The issue is that the crash is not even because it's trying to do a validated call. It crashes only because of this:

godot/modules/gdscript/gdscript_byte_codegen.cpp

Lines 1097 to 1098 in 5cf0d77

	CallTarget ct = get_call_target(p_target, result_type);
	Variant::Type temp_type = temporaries[ct.target.address].type;

The target might not be always a temporary, so trying to access the temporaries' table might be bogus, which is exactly what happens with this crash. The code should check if it's actually a temporary or not before trying to get its type.

I'm approving this because it "solves" the first point (as it won't emit validated calls when the argument is Variant), but the other point should be checked as well.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[emre0altan]

Thank you🙂