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.

https://github.com/godotengine/godot/blob/b4e2a24c1f62088b3f7ce0197afc90832fc25009/modules/gdscript/gdscript_byte_codegen.cpp#L430-L434

https://github.com/godotengine/godot/blob/b4e2a24c1f62088b3f7ce0197afc90832fc25009/modules/gdscript/gdscript_function.h#L47-L65

[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.

https://github.com/godotengine/godot/blob/d3352813ea44447bfbf135efdec23acc4d1d3f89/core/variant/variant_utility.cpp#L1922-L1929

[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: https://github.com/godotengine/godot/blob/d3352813ea44447bfbf135efdec23acc4d1d3f89/core/variant/variant_utility.cpp#L1922-L1929 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: https://github.com/godotengine/godot/blob/5cf0d772bc9a48c807b2ac63fe75b29df4671ecc/modules/gdscript/gdscript_byte_codegen.cpp#L1097-L1098

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 :tada:

[emre0altan]

Thank you🙂