GDScript: Assume freed object to be of any Object type

[vnen]

Since we can't check the actual type, we assume the type is correct if the expected type is Object-derived, the same as what happen with null values.

Fix #86609

Edit: added an exception for is to also allow it on freed objects, but in this case it's always false. Also added a few test cases.

[dalexeev]

Makes a lot of sense to me, but we should check other places as well. For example, the following hack is probably no longer needed, at least replacing Freed Object with null (see #81605 for details):

godot/modules/gdscript/gdscript_lambda_callable.cpp

Lines 95 to 101 in 107f296

	if (captures[i].get_type() == Variant::OBJECT) {
	bool was_freed = false;
	captures[i].get_validated_object_with_check(was_freed);
	if (was_freed) {
	ERR_PRINT(vformat(R"(Lambda capture at index %d was freed. Passed "null" instead.)", i));
	static Variant nil;
	args.write[i] = &nil;

The old behavior (error when passing Freed Object as a parameter) was convenient, perhaps people rely on it:

It seems that the useful error will not appear with this PR, since it does not appear now if you remove the type hint from the function parameter. Probably a similar check should be done separately, since we check this in many cases:

Incomplete list

godot/modules/gdscript/gdscript_utility_functions.cpp

Line 550 in 107f296

*r_ret = RTR("Type argument is a previously freed instance.");

godot/modules/gdscript/gdscript_utility_functions.cpp

Line 567 in 107f296

*r_ret = RTR("Value argument is a previously freed instance.");

godot/modules/gdscript/gdscript_vm.cpp

Line 825 in 107f296

err_text = "Left operand of 'is' is a previously freed instance.";

godot/modules/gdscript/gdscript_vm.cpp

Line 847 in 107f296

err_text = "Left operand of 'is' is a previously freed instance.";

godot/modules/gdscript/gdscript_vm.cpp

Line 1362 in 107f296

err_text = "Trying to assign invalid previously freed instance.";

godot/modules/gdscript/gdscript_vm.cpp

Line 1399 in 107f296

err_text = "Trying to assign invalid previously freed instance.";

godot/modules/gdscript/gdscript_vm.cpp

Line 2249 in 107f296

err_text = "Trying to await on a freed object.";

godot/modules/gdscript/gdscript_vm.cpp

Line 2560 in 107f296

err_text = "Trying to return a previously freed instance.";

godot/modules/gdscript/gdscript_vm.cpp

Line 2602 in 107f296

err_text = "Trying to return a previously freed instance.";

godot/modules/gdscript/gdscript_vm.cpp

Line 2970 in 107f296

err_text = "Trying to iterate on a previously freed object.";

godot/modules/gdscript/gdscript_vm.cpp

Line 3301 in 107f296

err_text = "Trying to iterate on a previously freed object.";

By the way, I noticed an error, was_freed is not used:

godot/modules/gdscript/gdscript_vm.cpp

Lines 1721 to 1730 in 107f296

	if (!call_async && ret->get_type() == Variant::OBJECT) {
	// Check if getting a function state without await.
	bool was_freed = false;
	Object *obj = ret->get_validated_object_with_check(was_freed);
	if (obj && obj->is_class_ptr(GDScriptFunctionState::get_class_ptr_static())) {
	err_text = R"(Trying to call an async function without "await".)";
	OPCODE_BREAK;
	}
	}

[vnen]

What I think is that is if a value is immediately used it should give an error. For instance, passing it to a utility function should raise the error because the user does not have control of its contents, but passing to a user-made function (like in this PR) should not trigger it automatically. Because sometimes the user don't have a lot of control of the source of data. For most cases being freed or null is pretty much the same, so while the error is somewhat useful in some situations, it does not really exclude argument validation.

I do agree that it should be changed in a few other places to be consistent with this if needed.

[vnen]

Updated this to remove the check in lambda captures, since that is not needed anymore. I also added a change to allow is to be performed on a freed object, which will always return false since the type cannot be checked.

I considered doing the same for casting (as) but I think it's better to keep it an error. If you're casting a freed object you're most likely doing something wrong.

Also added a few test cases to validate the behavior.

[Macksaur]

I disagree with as staying an error. is/as are supposed to be non-throwing and safe operators to write type-safe code, not randomly crash to the editor operators. I was very surprised they worked like this.

If you're casting a freed object you're most likely doing something wrong.

Can you describe which scenarios this helps?

Assuming you deliberately have a freed object you cannot store/access/move it safely elsewhere or type-check with the is/as operators since it will break, despite the logic being functionally correct otherwise. With the current design you are burdened to micromanage every resource intricately (or ignore types entirely and use Variant everywhere freed objects may go). It basically makes freed objects radioactive, when they could be graceful.

I think that all of the VM breaks should be removed and replaced with gentle linting (that can be toggled off) rather than hard errors that prevent the user from progressing with simpler code.