Statically typed function argument silently converted to null if the type doesn't match

[ttencate]

Godot version:
3.2.1.stable.official

OS/device including version:
Arch Linux (installed godot-bin from AUR)

Issue description:
Static typing on function arguments can under some circumstances result in an argument silently being converted to null when the type doesn't match, rather than throwing an error.

Steps to reproduce:

tool
extends EditorScript

func _run():
	var bar = "hello"
	print("bar outside foo: ", bar) # prints "hello"
	foo(bar)

func foo(bar: Node):
	print("bar inside foo: ", bar) # prints "Null"

Just paste the above code into a new GDScript file and hit Ctrl+Shift+X to run it.

Expected result:
Because bar has no type, the error can't be detected at compile time. So I would expect an error at runtime like this:

bar outside foo: hello
_ready; Invalid type in function 'foo' in base '...'. Cannot convert argument 1 from String to Node.

Alternatively, the bar variable should have the String value of "hello" inside the function foo, as would happen if there weren't any type annotations present:

bar outside foo: hello
bar inside foo: hello

This would be a worse outcome than a clear error, but I could understand it from a performance point of view if dynamic types aren't checked at every possible occasion.

Actual result:
No errors are reported; the function is called successfully but the incoming argument is mutilated into null:

bar outside foo: hello
bar inside foo: Null

Interestingly, the error is reported correctly if I change the function to take Array instead of Node:

bar outside foo: hello
_ready: Invalid type in function 'foo' in base '...'. Cannot convert argument 1 from String to Array.

Minimal reproduction project:
Copy-pasting the one snippet above is sufficient.

[dalexeev]

This is working correctly. The bar variable can be Node, so there are no warnings.

[ttencate]

@dalexeev Maybe you misunderstood. I'm not trying to fix my example code. I'm trying to say there's an obvious bug in the example code, and not only does the static typing fail to catch it, it's actually altering behaviour of the code at runtime.

[dalexeev]

Yes, you are right, at runtime this should cause an error. Just the issue title is a bit confusing.

[Zylann]

at runtime

Ideally, at runtime, when the script gets parsed (as opposed to being run), to be even more precise ;)

[dalexeev]

at runtime, when the script gets parsed (as opposed to being run)

I am not sure that this analysis is possible even for local variables (and even more so for class members).

func _run():
    var bar = "hello"
    
    if randi() % 2:
        bar = Node.new()
    
    foo(bar) # What type of `bar` is it now?

func foo(bar: Node):
    ...

The only way is to check the type at the time of calling foo().

Another thing is if the bar variable is statically typed - in this case the error will be visible already when writing the code.

[Zylann]

Oh, I was referring to a situation like this:

func _run():
	var bar := "hello"
	print("bar outside foo: ", bar) # prints "hello"
	foo(bar)

If not typed then yeah it can only defer to runtime (and induce a cost).

[dalexeev]

In this situation, everything seems to work correctly, the line with the error is highlighted in red.

(I wrote about this in my first comment.)

[ttencate]

I'm going to look at making a PR now, and if it turns out to be over my head I'll post back here.

[ttencate]

Colour me surprised: this behaviour is coded explicitly.

godot/modules/gdscript/gdscript_function.cpp

Lines 339 to 349 in a2da99f

	if (!argument_types[i].is_type(*p_args[i], true)) {
	if (argument_types[i].is_type(Variant(), true)) {
	memnew_placement(&stack[i], Variant);
	continue;
	} else {
	r_err.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT;
	r_err.argument = i;
	r_err.expected = argument_types[i].kind == GDScriptDataType::BUILTIN ? argument_types[i].builtin_type : Variant::OBJECT;
	return Variant();
	}
	}

Paraphrasing: whenever the actual argument isn't convertible to the expected type, rather than immediately giving up with an error, we first check whether null is a valid value for the expected type. If that is the case, we replace the argument by null and proceed. So this also happens also in cases where you pass e.g. a Node where a Node2D is expected, or a Node2D where a Node3D is expected, and similar with custom classes. I'm baffled why nobody has reported this before...

This was added in f0efc75 because "Previous version resulted in confusing (but actually right) errors". Fair enough, but now it results in confusing (and actually wrong) runtime behaviour.

But there is some good news: the error message seems to have improved since that commit was written, so it may no longer be necessary. Here's my testing code:

extends Node

func _ready():
	# Using call_deferred to avoid stopping at the first error.
	call_deferred("foo", Node.new())   # Correct type (Node)
	call_deferred("foo", Node2D.new()) # Correct type (Node subclass)
	call_deferred("foo", RegEx.new())  # Object type, but not a Node
	call_deferred("foo", "hello")      # String type
	call_deferred("foo", [])           # Array type
	call_deferred("foo", null)         # null type

# Using an extra level of indirection through this untyped `foo` because
# call_deferred seems to silently ignore invocations with the wrong signature.
func foo(bar):
	print("bar inside foo: ", bar)
	foo_impl(bar)

func foo_impl(bar: Node):
	print("bar inside foo_impl: ", bar)

And the resulting output, when I run this in a build with f0efc75 reverted:

bar inside foo: [Node:1132]
bar inside foo_impl: [Node:1132]
bar inside foo: [Node2D:1133]
bar inside foo_impl: [Node2D:1133]
bar inside foo: [RegEx:1134]
SCRIPT ERROR: foo: Invalid type in function 'foo_impl' in base 'Node (nullify.gd)'. The Object-derived class of argument 1 (RegEx) is not a subclass of the expected argument class.
   At: res://nullify.gd:14.
bar inside foo: hello
SCRIPT ERROR: foo: Invalid type in function 'foo_impl' in base 'Node (nullify.gd)'. Cannot convert argument 1 from String to Object.
   At: res://nullify.gd:14.
bar inside foo: []
SCRIPT ERROR: foo: Invalid type in function 'foo_impl' in base 'Node (nullify.gd)'. Cannot convert argument 1 from Array to Object.
   At: res://nullify.gd:14.
bar inside foo: Null
bar inside foo_impl: Null

Those look like fine error messages to me! @bojidar-bg Would you agree to a pull request to revert that commit?

[KoBeWi]

So this also happens also in cases where you pass e.g. a Node where a Node2D is expected, or a Node2D where a Node3D is expected, and similar with custom classes. I'm baffled why nobody has reported this before...

This is actually convenient. In my case I have a Player class and in various methods I do this:

func on_body_entered(body: Player):
    if body:
        #collided with player

I would be sad if this gave an error :I
(I know I could just use is though...)

[ttencate]

https://xkcd.com/1172/ is just too true, isn't it... But seriously, this auto-nulling behaviour is undocumented, inconsistent (it doesn't happen if you have Array instead of Player) and extremely surprising to anyone who's ever worked with another language (which is a large number of people). It may make the code slightly easier to write in some very specific cases like this, but it makes it much harder to understand. I would strongly recommend not to rely on it.