GDScript: Fix some methods still can use multi-level calls

[dalexeev]

In 4.0 we removed multi-level calls, users must now always explicitly use super() to call a base class method.

However, some methods are not implemented correctly, and a base class method may be implicitly called (if child class method returns false, null, or an error occurs). This is most apparent in _get_property_list() and _notification(), which are always multi-level called.

Outdated example

# 1.gd
extends Node

func _get_property_list() -> Array[Dictionary]:
    print("1.gd")
    return []

# 2.gd
extends "1.gd"

func _ready() -> void:
    var _t = get_property_list()

func _get_property_list() -> Array[Dictionary]:
    print("2.gd")
    # No `super()` here!
    return []

Prints:

2.gd
1.gd

Method list:

• _set()
• _get()
• _validate_property()
• _get_property_list() (!)
• _property_can_revert()
• _property_get_revert()
• _notification() (!)
• _to_string()

EDIT 1. Formally, this doesn't break compatibility, it's a bug fix. But some users can rely on this wrong behavior (I believe this is a bug), so it definitely deserves a mention in the release notes if this PR gets merged.

EDIT 2. _notification() looks problematic, since notification() is essentially intended for multi-level calls. See comment below.

[fire]

Can someone explain to me what bug this is solving in a game?

[dalexeev]

Can someone explain to me what bug this is solving in a game?

This fixes an inconsistency in GDScript. All other methods require an explicit super() in order for base class method to be called, otherwise you have no control over it. Multi-level calls in methods like _ready() have been removed in 4.0.

[dalexeev]

• See also Fix Object::notification order #78634.

CC @Sauermann

I think either we should treat all script classes as one or it should be explicitly documented that _notification() is an exception and does multi-level calls automatically, super() is not needed.

We could also add _notification2() like in GDExtension, but I don't think such a confusing manual call makes sense:

func _notification2(what: int, reversed: bool) -> void:
    if not reversed:
        super(what, reversed)
    # ...this level...
    if reversed:
        super(what, reversed)

[YuriSizov]

_get_property_list must be multi-level as well, because properties must be added in the correct order and by the class that holds them. I.e. there is never a situation when extending class can completely replace extended class' properties. I think both these are conscious exceptions.

[dalexeev]

_get_property_list must be multi-level as well, because properties must be added in the correct order and by the class that holds them.

But it can easily be achieved without multi-level calls, just one line needs to be changed. Unlike _notification(), which functionality includes multi-level calls by description and is more difficult to implement (see comment above). Don't we want to have as few exceptions as possible?

func _get_property_list() -> Array[Dictionary]:
    var result := super() # Instead of `var result: Array[Dictionary] = []`.
    result.append({name = "property", type = TYPE_INT})
    return result

Yes, _get_property_list() has a point in favor of multi-level calls due to compatibility. But if we want to add one more virtual method, which would also be "logical" (but not necessary) to be multi-level called, do we add an exception or prefer consistency? Or is the only argument here is compatibility?

[YuriSizov]

My argument is not about compatibility. The order of registering properties in the engine is important. It's not something that users should be able to break. This method is an exception because extending classes should not be able to override the behavior of the extended class. It's just something that should never happen.

I'm saying this about the logic of this call in the engine overall, not just in GDScript. These methods and the way they are called is important to the core. So yeah, nuance matters more than perceived consistency.

[dalexeev]

Okay, I got your point. Then I think all exceptions with multi-level calls (currently 2 methods) should be justified, commented in the code and documented.

[anvilfolk]

Went through it code and I believe all of this makes sense. Certain functions were not returning inside a while loop, meaning even after finding a result, superclass methods were being called.

This might actually end up fixing a few unexpected/awkward behaviors.

I have some minor requests, but nothing that fundamentally changes the opinion that this should be merged :)

[fire]

Does super() exist in c++ modules or gdextension? I think we have to do call("super") right? Not sure the syntax.

[dalexeev]

Does super() exist in c++ modules or gdextension? I think we have to do call("super") right? Not sure the syntax.

No, super() (and super.method()) is GDScript syntax. C# and C++ use a different syntax.

[dalexeev]

@YuriSizov I'm still unsure about _get_property_list(). It would be weird that it is multi-level called and _set(), _get(), _property_can_revert(), _property_get_revert() and _validate_property() are called as normal methods. In this case, it makes sense to keep the current behavior, but it's much more exceptions and all of them should be properly documented.

Also unusual is that if _set() and _property_can_revert() return false or _get() returns null, then the base class method is called and in other cases no. Conditional multi-level calls?

I also checked our codebase and found that most of the time we don't call the base class method (it called automatically?). However, there are multiple instances of a (wrong?) call to the base method:

\t+.*::(_set|_get|_get_property_list|_property_can_revert|_property_get_revert|_validate_property)\(
18 matches

scene/3d/camera_3d.cpp: 1
 84:  1: \tNode3D::_validate_property(p_property);
scene/3d/physics_body_3d.cpp: 14
2242:  1: \tif (JointData::_set(p_name, p_value, j)) {
2273:  1: \tif (JointData::_get(p_name, r_ret)) {
2291:  1: \tJointData::_get_property_list(p_list);
2299:  1: \tif (JointData::_set(p_name, p_value, j)) {
2342:  1: \tif (JointData::_get(p_name, r_ret)) {
2364:  1: \tJointData::_get_property_list(p_list);
2374:  1: \tif (JointData::_set(p_name, p_value, j)) {
2423:  1: \tif (JointData::_get(p_name, r_ret)) {
2447:  1: \tJointData::_get_property_list(p_list);
2458:  1: \tif (JointData::_set(p_name, p_value, j)) {
2531:  1: \tif (JointData::_get(p_name, r_ret)) {
2563:  1: \tJointData::_get_property_list(p_list);
2579:  1: \tif (JointData::_set(p_name, p_value, j)) {
2739:  1: \tif (JointData::_get(p_name, r_ret)) {
servers/audio/audio_stream.cpp: 3
655:  1: \tif (AudioStream::_get(p_name, r_ret)) {
679:  1: \tif (AudioStream::_set(p_name, p_value)) {
703:  1: \tAudioStream::_get_property_list(p_list); // Define the trivial scalar properties.

[YuriSizov]

@dalexeev So the thing is, all of these methods are ML calls in the engine. You can check the GDCLASS macro in object.h to see how that works, if you haven't already. All of them implicitly call inherited implementations at the best time that fits the logic of each methods. Object's own implementation is where we hook into extensions and scripting.

I think it's reasonable that extensions and scripting work in the exactly same way as the engine. So if the core method is ML, then GDScript should do the same thing in its class inheritance chain. Otherwise we have to deal with different expectations between the engine core and engine extensions. I was mainly advocating for get_property_list specifically because there are no circumstances where parent class' logic can be ignored. For other methods it wouldn't be ideal, but would be kind of okay.

Ideally, though, this should be consistent with core, IMO. Maybe @vnen can offer an opinion before we move forward with any specific changes, since he'd be well-versed in both core and GDScript.

[vnen]

IMO all of those make sense to be multilevel, except _to_string(). The ones that were removed were based on the confusion it generated and the inconsistent ordering of calls (some called the super class first, others called the sub class first).

For something like _set() and other property-related methods it makes sense that the script handles only its own properties and if not handled it defers to the super class instead of having to manually calling super() every time.

While dynamic properties are already tricky to deal with in OOP, it shouldn't break polymorphism. So a sub class should not just overwrite the properties of the super class by overriding this behavior, which would be the main reason to make those not multilevel and force the use of super() when needed.

Lastly, I think that if one of the property-related methods is ML, all of them should be, otherwise we'll have to keep remembering which ones are.

[adamscott]

Post-poned the target release of this PR to 4.3 as there's ongoing discussion.

[Mickeon]

Suggested change

	[b]Note:[/b] [code]_get_property_list()[/code] is automatically called for [b]all[/b] base classes. Unlike most other methods, you don't need to call [code]super()[/code] (in GDScript).
	[b]Note:[/b] [method _get_property_list] is automatically called for [b]all[/b] inherited classes. Unlike most other methods, you do not need to call [code]super()[/code] (in GDScript).

[Mickeon]

Suggested change

	[b]Note:[/b] [code]_notification()[/code] is automatically called for [b]all[/b] base classes. Unlike most other methods, you don't need to call [code]super()[/code] (in GDScript).
	[b]Note:[/b] [method _notification] is automatically called for [b]all[/b] inherited classes. Unlike most other methods, you do not need to call [code]super()[/code] (in GDScript).