Calling a static method from a static method in autoload, emits unnecessary warnings

[Chaosus]

Godot version

6916349697a4339216469e9bf5899b983d78db07

System information

Windows 11

Issue description

Consider placing the static call in static mathod in autoloaded script (ex Globals):

and later call it in the common script:

emits the warning on the project startup:

I think it's unnecessary and incorrect.

Steps to reproduce

^^

Minimal reproduction project

Test.zip

[k0T0z]

@Chaosus I think by modifying line 4 to be Globals.Node.boxv(Vector3i(0, 0, 0), 10, 10), the warning goes away. I am working on it right now.

[Chaosus]

@k0T0z Nope, such code is incorrect:

[k0T0z]

hmmm, okay just give me some time to figure things out.

[k0T0z]

@Chaosus I think the docs are unclear in unit tests running part? What is this ./bin/<godot_binary>? Which binary is the meant one? I have these binaries only:

Can you help?

[Chaosus]

Well, I guess it's godot.windows.editor.x86_32 since it has been recently modified, according Date modified column. If you compile the engine using a 32-bit version of msvc, of course.

[k0T0z]

@Chaosus Thanks to you actually, it's working that way. Every time I forgot about my MSVC's architecture 😅.

[k0T0z]

I don't know if this will help, but it is there if anyone will review it: #8202.

[dalexeev]

I'm not sure if this is actually a bug. The singleton name is an instance, not a class. You can use non-static methods and call them directly on the singleton. Static singleton methods don't make much sense since you are not expected to create new instances of the singleton class.

However, if you give a class_name to the singleton script or load it via const-preload, then you can call static methods without warnings:

# globals.gd
class_name GlobalsClass
extends Node

static func static_func():
    pass

func non_static_func():
    pass

# other.gd
func _ready():
    GlobalsClass.static_func()
    Globals.non_static_func()

[k0T0z]

@dalexeev If I understand correctly these singletons collect global functions (You can call global functions using these singletons) and they are built-in, they are not created by the user which means this warning is not necessary as Globals.static_meth() is the same as Node.static_meth(). Also if it's an instance why there is no parenthesis Globals().static_meth(), I need to practice this special kind of calling.

I need to read about these godot singletons anyway because where I am from, this concept is new I guess.

[dalexeev]

@k0T0z For global function collections you can use a regular class, no need for a singleton. For example:

class_name Globals

static func sum(a, b):
    return a + b

print(Globals.sum(1, 2)) # No warning.

Singletons (autoload) are unique instances that are added to the root of the scene tree.

[LunaticInAHat]

It's true that singletons are instances, but you also aren't permitted to overload autoload names with class_name. That is, if you have Globals.gd, autoloaded as Globals, you can't give that script class_name Globals. If it has static functions, then your only options are to access those functions through the autoload name, or to give the script a class name that is different than its autoload name.

Having to refer to the same script through two different names (one of which necessarily differing from the filename), depending on which particular function you're trying to call, is pretty awkward.

It seems to me that, while singletons are technically instances, that's almost an implementation detail, in the context of this particular usecase. When the user refers to a static function of a script that is also an autoload, I don't think they're confused about whether they're accessing an object or not, because singletons often don't behave very "objecty". So it's not clear to me what value the warning is adding, in those cases, and maybe it ought to just be suppressed. There's no elegant way to get to those functions otherwise.

[dalexeev]

I don't see the point in using static functions with singletons. Typically, singletons are used for a single instance that is always accessible from anywhere. You are not expected to create new instances of the singleton class. That is, it is not the warning that is incorrect, but the use of static functions with singletons. Static functions should be used with regular classes, for which Name denotes the class rather than the instance.

That is, if you have Globals.gd, autoloaded as Globals, you can't give that script class_name Globals.

Perhaps we should think about how to fix this. See also godotengine/godot-proposals#8441.

[LunaticInAHat]

I don't see the point in using static functions with singletons. Typically, singletons are used for a single instance that is always accessible from anywhere. You are not expected to create new instances of the singleton class. That is, it is not the warning that is incorrect, but the use of static functions with singletons. Static functions should be used with regular classes, for which Name denotes the class rather than the instance.

I completely agree with the first half of what you say: Singletons are used to collect stuff that is always accessible from anywhere, and you shouldn't create new instances of them. Where I disagree, is the conclusion that therefore your singletons shouldn't have static functions in them. There are both conceptual, and practical reasons to do this:

Practically, static functions can be used in at least one context where singleton object methods cannot: Tool scripts. Trying to call a singleton method from a tool script just results in: Invalid call. Nonexistent function 'Foo' in base 'Node (Global.gd)'.

On the conceptual side, we split our code into files and classes, so that we can bundle related things together, and separate them from unrelated things. If a function does not require any object state, then it should be static, and if it is conceptually related to the other things going on in Foo.gd, then it should be in Foo.gd. Whether Foo happens to also be a singleton object or not, doesn't seem very relevant.

It's not really clear to me why putting static functions into a script that happens to be an autoload would somehow be objectively "incorrect". That doesn't really seem true to me on the surface. Code should go where it makes sense for it to go, in the user's mind.

[k0T0z]

@dalexeev If the use-case is wrong, then the question becomes: what is the right action that the analyzer should perform? In my opinion, the syntax is pretty strange as Globals.some_function() is more likely to be a static calling syntax although Globals is actually an instance. I think it should be Globals().some_function() and then the warning will be obvious, but for Globals.some_function(), I think the syntax shouldn't be allowed (the analyzer tracks a syntax error) or the analyzer should output a different warning message indicating a bad/wrong use-case.

@LunaticInAHat I didn't get this part:

If a function does not require any object state, then it should be static,

I thought that a function could be global or global static or static inside a class!

So in short there is no contradiction between what we are saying here, which is the warning message doesn't need to be eliminated otherwise it needs to be more descriptive or we just refactor the analyzer to cover that use-case part.

[kleonc]

I think it's unnecessary and incorrect.

Autoloaded nodes are instances like any other nodes. Even if an autoload is set to be obtainable via a globally registered name it's still an instance, not a class. Hence the "static function called on instance" part of the warning is absolutely correct.

What's incorrect is the suggestion that it can be called like Node.boxv(). This part is indeed a bug. It could probably be detected whether the given class is available via some name and add hint like that only in such cases? :thinking: Or the hint could be changed to something like load("path/to/script_defining_static_func").static_func() (not sure that's a good idea though :upside_down_face:).

---

(...) these singletons (...)

Technically an autoload obtainable via a registered global name is not really a singleton. This is mentioned in the docs:

Note Godot won't make an Autoload a "true" singleton as per the singleton design pattern. It may still be instanced more than once by the user if desired.

Meaning currently e.g. something like this is possible:

# some_script.gd
extends Node

static func func_static():
    pass

func func_non_static():
    pass

# some_other_script.gd

func foo():
    GlobalInstance1.func_non_static()
    GlobalInstance2.func_non_static()

    GlobalInstance1.func_static() # Static called on instance.
    GlobalInstance2.func_static() # Static called on instance.

    load("path/to/some_script.gd").func_static() # Static called on class (no warning/error).

In the example above neither GlobalInstance1 nor GlobalInstance2 refer to the class defined by the some_script.gd, each of them refer to a seperate instance of that script.

---

Note there's already a debug/gdscript/warnings/static_called_on_instance project setting which can be changed to disable the warning (or to make it be treated as an error):

I don't see a reason why such warning should potentially be never shown in case a static call is performed using a globally registered name of an autoload (regardless of the project settings). For me adding such exception makes no sense, autoloaded instance is not a class, it's as simple as that.

What would be fine I think is e.g. extending/modifying the current warning/error message in case the static call was made on an autoload. In such case an additional sentence/hint could be added, e.g something like:

The function is a static function but was called from an instance. Note that global name refers to an autoloaded node instance, it is not a class name.

[LunaticInAHat]

From my perspective, the warning is only objectionable because it currently isn't possible to give autoloaded scripts a class name matching their autoload name. If that limitation is removed, then all of my reservations about the warning go away. I've never been debating its correctness, only its utility (in the sense of steering users toward good code structure) in the specific case of autoloads. If the users haven't been given a right way to do the (IMO quite reasonable) thing they're trying to do, then not much is gained by scolding them for doing it the wrong way. (As you say, load("path/to/script_defining_static_func").static_func() is quite a mouthful, and not something I would characterize as being an improvement to the user's code)

With the current behavior, I feel that expanding the warning message to clarify the distinction between an "autoloaded node instance" and a "class name" is going to lead people in the correct-seeming direction of trying to give their scripts a class name, only to discover that it isn't actually possible to give it the name they want. I think that the suggestion before from @dalexeev about trying to allow class_name and autoloads of the same name to coexist peacefully was quite right.

[kleonc]

From my perspective, the warning is only objectionable because it currently isn't possible to give autoloaded scripts a class name matching their autoload name. If that limitation is removed, then all of my reservations about the warning go away. (...) I think that the suggestion before from @dalexeev about trying to allow class_name and autoloads of the same name to coexist peacefully was quite right.

I disagree as I don't see how they could coexist peacefully. Let's suppose it's allowed and Global is both autoload global name and class_name. Then e.g. what the results of Global is Node and Global is Script would suppose to be?:thinking:

• Both true? Now that would be at least confusing. Like would it mean that multiple inheritance is allowed or what? Seems like we would need to introduce "autoload-class duality" term. :smile:
• One over the other? Which one and why?
• Both false? Also doesn't make sense.

I see it as introducing inconsistencies. The same name shouldn't be allowed to refer to both an instance (autoload) and to the script. Allowing doing so would lead to ambiguity, needing to handle such special cases/exceptions, and to new bugs caused by this. I don't see it as a viable solution (but maybe there's a way for them to coexist peacefully, would love to be surprised).

With the current behavior, I feel that expanding the warning message to clarify the distinction between an "autoloaded node instance" and a "class name" is going to lead people in the correct-seeming direction of trying to give their scripts a class name, only to discover that it isn't actually possible to give it the name they want.

Meaning this would potentially lead to discovering the difference between the autoload and the script. This I see as a good thing. If such difference exists (it does) then I see no point in trying to camouflage it.

---

I'd want to point out again that this warning can already be turned off in the project settings.

If a separate (sub)setting specifically for the "static called on instance using the autoload's global name" case is wanted so it would be possible to disable the warning only in this specific case and still be warned in other "static called on instance" cases - this would be fine for me as long as it would be an opt-in (you know you don't want a warning in this specific case hence you deliberately choose to not be warned about it). I wouldn't use it but if that's desired then sure (as it wouldn't be anyhow harmful in general).