Calling a method on variable properties crashes with infinite recursion

[rsubtil]

Godot version

4.0.dev latest

System information

Arch Linux

Issue description

When doing some quick refactoring for a project in 3.2 to 4.0, I tried to cut corners and refactored the old setget behavior like this:

var my_var:
    get:
        return get_my_var()

func get_my_var():
    return my_var % 3

func _ready():
    my_var = 3
    print(my_var)

However when running this script it crashes with a segfault. Looking at gdb there seems to be infinite recursion:

#0  0x000055e9d0245f28 in Variant::Variant (this=0x7fffa38660b0, p_object=0x55e9d3edf330) at core/variant/variant.cpp:2511
#1  0x000055e9cce5f6e8 in GDScriptFunction::call (this=0x55e9d3ed25a0, p_instance=0x55e9d3ef5910, p_args=0x7fffa386b8e0, p_argcount=0, r_err=..., p_state=0x0) at modules/gdscript/gdscript_vm.cpp:519
#2  0x000055e9ccdcc74d in GDScriptInstance::call (this=0x55e9d3ef5910, p_method=..., p_args=0x7fffa386b8e0, p_argcount=0, r_error=...) at modules/gdscript/gdscript.cpp:1501
#3  0x000055e9d054c53a in Object::call (this=0x55e9d3edf330, p_method=..., p_args=0x7fffa386b8e0, p_argcount=0, r_error=...) at core/object/object.cpp:811
#4  0x000055e9d0260475 in Variant::call (this=0x7fffa386b880, p_method=..., p_args=0x7fffa386b8e0, p_argcount=0, r_ret=..., r_error=...) at core/variant/variant_call.cpp:1017
#5  0x000055e9cce693e7 in GDScriptFunction::call (this=0x55e9d3ede610, p_instance=0x55e9d3ef5910, p_args=0x7fffa38710a8, p_argcount=0, r_err=..., p_state=0x0) at modules/gdscript/gdscript_vm.cpp:1487
...
#1481 0x000055e9cce693e7 in GDScriptFunction::call (this=0x55e9d3ed25a0, p_instance=0x55e9d3ef5910, p_args=0x7fffa4058c60, p_argcount=0, r_err=..., p_state=0x0) at modules/gdscript/gdscript_vm.cpp:1487
#1482 0x000055e9ccdcc74d in GDScriptInstance::call (this=0x55e9d3ef5910, p_method=..., p_args=0x7fffa4058c60, p_argcount=0, r_error=...) at modules/gdscript/gdscript.cpp:1501
#1483 0x000055e9d054c53a in Object::call (this=0x55e9d3edf330, p_method=..., p_args=0x7fffa4058c60, p_argcount=0, r_error=...) at core/object/object.cpp:811
#1484 0x000055e9d0260475 in Variant::call (this=0x7fffa4058c00, p_method=..., p_args=0x7fffa4058c60, p_argcount=0, r_ret=..., r_error=...) at core/variant/variant_call.cpp:1017
#1485 0x000055e9cce693e7 in GDScriptFunction::call (this=0x55e9d3ede610, p_instance=0x55e9d3ef5910, p_args=0x7fffa405e428, p_argcount=0, r_err=..., p_state=0x0) at modules/gdscript/gdscript_vm.cpp:1487
#1486 0x000055e9ccdcc74d in GDScriptInstance::call (this=0x55e9d3ef5910, p_method=..., p_args=0x7fffa405e428, p_argcount=0, r_error=...) at modules/gdscript/gdscript.cpp:1501
#1487 0x000055e9d054c53a in Object::call (this=0x55e9d3edf330, p_method=..., p_args=0x7fffa405e428, p_argcount=0, r_error=...) at core/object/object.cpp:811
#1488 0x000055e9d0260475 in Variant::call (this=0x7fffa405e3b0, p_method=..., p_args=0x7fffa405e428, p_argcount=0, r_ret=..., r_error=...) at core/variant/variant_call.cpp:1017
#1489 0x000055e9cce693e7 in GDScriptFunction::call (this=0x55e9d3ed17e0, p_instance=0x55e9d3ef5910, p_args=0x0, p_argcount=0, r_err=..., p_state=0x0) at modules/gdscript/gdscript_vm.cpp:1487
#1490 0x000055e9ccdcc74d in GDScriptInstance::call (this=0x55e9d3ef5910, p_method=..., p_args=0x0, p_argcount=0, r_error=...) at modules/gdscript/gdscript.cpp:1501
#1491 0x000055e9ce51b9c0 in Node::_gdvirtual__ready_call (this=0x55e9d3edf330) at ./scene/main/node.h:236
#1492 0x000055e9ce4fd01e in Node::_notification (this=0x55e9d3edf330, p_notification=13) at scene/main/node.cpp:145
#1493 0x000055e9ccac9721 in Node::_notificationv (this=0x55e9d3edf330, p_notification=13, p_reversed=false) at ./scene/main/node.h:48
#1494 0x000055e9ccac95ec in CanvasItem::_notificationv (this=0x55e9d3edf330, p_notification=13, p_reversed=false) at ./scene/main/canvas_item.h:47
#1495 0x000055e9ce486c7c in Node2D::_notificationv (this=0x55e9d3edf330, p_notification=13, p_reversed=false) at ./scene/2d/node_2d.h:37
#1496 0x000055e9d05497c0 in Object::notification (this=0x55e9d3edf330, p_notification=13, p_reversed=false) at core/object/object.cpp:841
#1497 0x000055e9ce4fe929 in Node::_propagate_ready (this=0x55e9d3edf330) at scene/main/node.cpp:190
#1498 0x000055e9ce4fe8c5 in Node::_propagate_ready (this=0x55e9d3d35b70) at scene/main/node.cpp:182
#1499 0x000055e9ce504867 in Node::_set_tree (this=0x55e9d3d35b70, p_tree=0x55e9d3aaa0e0) at scene/main/node.cpp:2450
#1500 0x000055e9ce540575 in SceneTree::initialize (this=0x55e9d3aaa0e0) at scene/main/scene_tree.cpp:402
#1501 0x000055e9cc88c32f in OS_LinuxBSD::run (this=0x7fffa4063f10) at platform/linuxbsd/os_linuxbsd.cpp:330
#1502 0x000055e9cc8875f6 in main (argc=9, argv=0x7fffa40643f8) at platform/linuxbsd/godot_linuxbsd.cpp:58

I don't know if this usage of properties is valid per se, but it certainly shouldn't segfault 🙂

Steps to reproduce

• Have a script with a variable with a get property that calls another method:

var my_var:
    get:
        return get_my_var()

func get_my_var():
    return my_var % 3

func _ready():
    my_var = 3
    print(my_var)

• Run the game
• It crashes with a segfault

Minimal reproduction project

MRP.zip

[akien-mga]

This code is indeed invalid as it creates infinite recursion, but maybe the GDScript analyser could figure it out and throw a helpful error.

[rsubtil]

Tested with a simpler script:

func inf_recursion():
	return 1 + inf_recursion()

func _ready():
	print(inf_recursion())

On master this always crashes, but on 3.3.3 This stops with a "Stack overflow" error, but only when launching from the editor; when launching directly (godot --path ...) it crashes the same way as above. Judging from the code, the stack overflow protection is enabled only for debug builds:

godot/modules/gdscript/gdscript_vm.cpp

Lines 540 to 545 in 44b1143

	#ifdef DEBUG_ENABLED
	if (EngineDebugger::is_active()) {
	GDScriptLanguage::get_singleton()->enter_function(p_instance, this, stack, &ip, &line);
	}

godot/modules/gdscript/gdscript.h

Lines 365 to 370 in 44b1143

	if (_debug_call_stack_pos >= _debug_max_call_stack) {
	//stack overflow
	_debug_error = "Stack Overflow (Stack Size: " + itos(_debug_max_call_stack) + ")";
	EngineDebugger::get_script_debugger()->debug(this);
	return;
	}

Although this makes sense IMO, shouldn't the engine avoid crashing at all costs on normal builds? Either way, I'll bisect the regression on master in the meantime.

EDIT: This script doesn't crash on master after all

[KoBeWi]

set: has the same issue.

[MatteoPiovanelli-Laser]

As of 6b0b1a4 this is still an issue. It causes the editor on Windows 10 to crash if a script such as this is attached to a node in a Scene that is open in the editor:

@export
var my_property : int: # the type doesn't really matter
	set(value):
		set_my_property(value)
[...]
func set_my_property(value: int) -> void:
	my_property = value

[KoBeWi]

The proper way to use method setter is

@export var my_property: int: set = set_my_property

The only thing that could be improved here is leading the user to the correct syntax instead of hard crashing.

[cullumi]

In 3.5 setting/getting a variable from the same "class" as the variable doesn't call the setter/getter function; to intentionally do so simply required a self.variable syntax. This always seemed pretty intuitive to me, is there some reason for this to no longer be the case?

That is to say:

# outside the variable's parent object:

object .property = value       # triggers setter, no infinite recursion

# inside the variable's parent object:

property = value               # does not trigger setter, no infinite recursion
self.property = value          # triggers setter, infinite recursion

It seems to me like OP's original example shouldn't have led to an infinite recursion in the first place, therefore making this a bug or a missing feature. Just figured I'd bring that functionality up since I didn't see anyone mention it in the thread so far.

The way it is now I'm forced to mimic C#'s set/get properties with an extra variable to store values on the side... which feels a bit silly to be honest.

[dalexeev]

@cullumi This is intentional. The old different behavior of property and self.property is inconsistent and inconvenient.

The way it is now I'm forced to mimic C#'s set/get properties with an extra variable to store values on the side...

There was a suggestion/comment to add an annotation and my proposal. But they didn't get support.

[dalexeev]

There is no crash in 4.0.3.