No warning when getting built in Callable from Dictionary

[nlupugla]

Tested versions

4.3.dev [b042636]

System information

Godot v4.2.stable - Windows 10.0.22621 - GLES3 (Compatibility) - Intel(R) Iris(R) Xe Graphics (Intel Corporation; 31.0.101.4255) - 12th Gen Intel(R) Core(TM) i5-1235U (12 Threads)

Issue description

A GDScript feature was recently added (#82264) that allows methods of built-in Variant types to be accessed as Callables. For example the feature makes the following code work.

var vec: Vector2 = Vector2(1.5, 2.5)
var callable: Callable = vec.ceil
print(callable.call()) # prints (2, 3)

This is a great feature, except there are cases where it could lead to potential confusion.

var dict: Dictionary = {&"clear" : randf}
var callable: Callable = dict.clear # does this refer to randf or the built in clear method?
callable.call()
print(dict) # does this print a blank dictionary or the original dictionary?

There is also potnential for confusion when using type annotations. In the following code

var dict: Dictionary = {&"clear" : "oops"}
var callable: Callable = dict.clear

the analyzer does not complain, but at runtime, I get "INTERNAL ERROR: Trying to assign value of type 'String' to a variable of type 'Callable'."

I'm not entirely sure what the best solution is, but as the title suggests, it might make sense to emit some kind of GDScript warning when the user creates or accesses a key that has the same name as a built in method.

Steps to reproduce

Run the script above.

Minimal reproduction project (MRP)

N/A

[dalexeev]

This is a great feature, except there are cases where it could lead to potential confusion.

var dict: Dictionary = {&"clear" : randf}
var callable: Callable = dict.clear # does this refer to randf or the built in clear method?

Note that the feature currently skips dictionaries for compatibility reasons. Both dict.clear and dict["clear"] refer not to the method, but to a key (regardless of whether it exists).

Problems:

1. Lack of a convenient way to construct a Callable for Dictionary methods in GDScript. The most expected syntax would be dict.clear, but this is ambiguous with keys and breaks compatibility. See also the Rocket Chat discussion starting from the comment. Possible options:

   1.1. (Breaks compatibility) Always treat dict.clear as Callable and dict["clear"] as a key (if missing, it produces an error). Both dict.not_a_method and dict["not_a_method"] are treated as keys. This is not the worst option, since the behavior does not depend on whether the key is in the dictionary or not. Also, this does not break compatibility for keys whose names do not match the methods.

   The only downside is that adding a new Dictionary method may break compatibility when using .attribute syntax (but not ["index] syntax). But for objects, we are constantly adding new properties/methods and do not consider this as breaking compatibility.

   1.2. (Breaks compatibility) Other ways with behavior depending on the presence of the key in the dictionary. I think that all the ways break compatibility, but also behavior depending on the key list of the dictionary is rather bad. Let me know what you think about it.

   1.3. Some new syntax. In my opinion this is unjustified.

   1.4. Leave as is.

2. Lack of any way to construct a Callable for Dictionary methods in GDScript. We could add an ad hoc solution like Dictionary.get_method(), but I think we should solve problems 1 and 3, or at least just 3, instead.

3. Lack of a universal way to construct a Callable for an arbitrary Variant (without checking the value type). I think one possible solution would be a Callable(Variant, StringName) constructor (currently there is only Callable(Object, StringName)). But this is problematic to bind since the existing system expects specific Variant types. Another option is a static method (like Callable.create()), this shouldn't be a problem.

[nlupugla]

First, a quick question about compatibility considerations. Although (https://github.com/godotengine/godot/pull/82264) has been merged, I wonder since it is only in a dev build whether the policy around compatibility breakage is less strict.

I definitely agree that the behavior shouldn't depend on what keys are in the dictionary. Of the options you listed, I like 1.1 the best. The only other alternative I can think of is to emit a warning whenever "clear" is used as a key in a dictionary, but I'm not sure it's much better than 1.1.

For problem 3. Callable(Variant, StringName) certainly makes the most sense to me semantically. I don't quite understand the issues with binding it. Also, is there really no universal way to construct a Callable for an arbitrary Variant?. Can't I do func(): my_variant.my_func()?

[dalexeev]

I don't quite understand the issues with binding it.

I looked at this issue. A suitable binder class is not implemented, it may be non-trivial to do as far as I remember. Also, I'm not sure whether the problem could be that the signature Callable(Variant, StringName) includes Callable(Object, StringName), i.e. call Callable(object, "method") will match two overloads at once.

[nlupugla]

Hmm, what about changing the signature in cpp to take a Variant instead of an Object and then have a branch in the code that checks whether the variant is an object or not? I wouldn't imagine that changing the signature like this would introduce any compatibility issues, but maybe there is something related to GDExtension bindings or something that would break if Object were replaced with Variant.