GDScript: Issues with static variables

[dalexeev]

Godot version

v4.1.dev.custom_build [5c653c2]

System information

Kubuntu 23.04

Issue description

1. The setter is called when initializing a static variable (for both cases: a. if a static type is specified and b. if there is an initializer).

static var a: int = 1:
    set(value):
        prints("set a", value)

# For a non-static variable it's correct.
var b: int = 1:
    set(value):
        prints("set b", value)

set a 0
set a 1

This is because _make_static_initializer() uses codegen.generator->write_set_named() rather than codegen.generator->write_assign() since static variables belong to the script. We probably need a separate instruction that will not call the setter. I'm not familiar with the late stages, so I'm not sure how to properly fix this.

Also food for thought: does this make static variables slightly slower than non-static ones?

GDScript::callp at modules/gdscript/gdscript.cpp:866
GDScript::_set at modules/gdscript/gdscript.cpp:942
GDScript::_setv at modules/gdscript/gdscript.h:61
Object::set at core/object/object.cpp:299
Variant::set_named at core/variant/variant_setget.cpp:248
GDScriptFunction::call at modules/gdscript/gdscript_vm.cpp:1050
GDScript::_static_init at modules/gdscript/gdscript.cpp:657

MRP

77098_1.zip

2. An inner class static variable is falsely considered readonly.

class Inner:
    static var a

func _run() -> void:
    Inner.a = 1 # Error: Cannot assign a new value to a constant.

But you can work around this:

class Inner:
    static var a

func _run() -> void:
    var t: Variant = Inner
    t.a = 1
    print(Inner.a) # 1

This is an analyzer bug. I can try to fix it.

https://github.com/godotengine/godot/blob/5c653c27cdf779e1e70a16ec9514435537a01779/modules/gdscript/gdscript_analyzer.cpp#L2465C18-L2468

For scripts with class_name this already works.

MRP

77098_2.zip

3. You can apply the @export* and @onready annotations to static variables, but that doesn't make sense and may not work correctly (since one script can be attached to multiple objects). I can add the necessary checks.

MRP

77098_3.zip

4. Not a bug, but more of a design issue. Should static variables be accessed via an instance? Currently no, but within a class, you can access both static and non-static variables the same way, without the self prefix.

# my_class.gd
class_name MyClass

static var a

# test.gd
func test():
    var x = MyClass.new()
    MyClass.a # OK.
    x.a # Error.

Or perhaps we should think again about the keyword for accessing the current class (Self). See godotengine/godot-proposals#391.

static var a
var b

func test():
    a # OK.
    b # OK.
    self.a # Runtime error. Should be a compile-time error.
    self.b # OK.

    # Proposal:
    Self.a # OK.
    Self.b # Error.

EDIT: You can access any static member via self or instance, except for static variables. Probably for consistency and compatibility we should allow this for static variables as well.

By the way, the line 38 is incorrectly marked as safe.

MRP

77098_4.zip

5. Separated to #78195.

Old content

5. Static variables are not in the property list of both the object and the script. If we want the former, then we need a new PROPERTY_USAGE_STATIC flag, but I think we can just add it to the script's property list.

static var a: int
var b: int

func _ready() -> void:
    for property in get_property_list():
        if property.name in ["a", "b"]:
            print(property)
    print("---")
    for property in get_script().get_property_list():
        if property.name in ["a", "b"]:
            print(property)
    print("---")
    for property in get_script().get_script_property_list():
        if property.name in ["a", "b"]:
            print(property)

{ "name": "b", "class_name": &"", "type": 2, "hint": 0, "hint_string": "", "usage": 4096 }
---
---
{ "name": "b", "class_name": &"", "type": 2, "hint": 0, "hint_string": "", "usage": 4096 }

EDIT: I think this makes sense, since we have to add the static flag in the user documentation, and also because point 4.

Steps to reproduce

See above.

Minimal reproduction project

N/A
Bugsquad edit: add MRPs

[timpt0261]

I'm interested in this is there anything more you may need help with

[vnen]

EDIT: You can access any static member via self or instance, except for static variables. Probably for consistency and compatibility we should allow this for static variables as well.

Regarding this, calling static methods from instances is seen as a bad thing and generates a warning, so I assumed this wouldn't be wanted for static variables either. Since it's a new feature it can be forbidden from the start instead of just a warning. But static variables do not have the same potential issue as static methods, then maybe it does not matter. This is also looks inconsistent for me if calling static methods from instance generates warnings but accessing static variables do not.

[akien-mga]

EDIT: You can access any static member via self or instance, except for static variables. Probably for consistency and compatibility we should allow this for static variables as well.

Regarding this, calling static methods from instances is seen as a bad thing and generates a warning, so I assumed this wouldn't be wanted for static variables either. Since it's a new feature it can be forbidden from the start instead of just a warning. But static variables do not have the same potential issue as static methods, then maybe it does not matter. This is also looks inconsistent for me if calling static methods from instance generates warnings but accessing static variables do not.

That warning is a bit too eager though, as seen in #74397. So we probably shouldn't take it as design guidance, unless we can make sure it's only emitted in cases where it makes sense.

It was added mostly as bandaid for issues such as

var tex = ImageTexture.new()
tex.create_from_image(img)

which were valid code in 3.x but would suddenly do nothing in 4.x, as the method is now static and returns an instance.

That's arguably the only case which should really trigger a warning (calling a static method on an instance that returns an instance of the same type, i.e. a static constructor).

[dalexeev]

I agree that accessing static members via an instance is confusing and is not recommended. But given that it works for other types of static members (methods, constants, enums, inner classes), it would be strange not to support it only for variables. If warnings are needed, we can add them later.

Also inside a class you don't strictly separate instance properties and static variables, there are no mandatory prefixes (like $this-> and self:: in PHP).

At the moment self.static_var does not cause a compile error, but it does at runtime. I considered it a bug, and tried to fix it as described above. If in the future we add a keyword to access the current class (Self), then Self.non_static_var should of course raise an compile time error. However, self can be used to access both instance members and class members, at least in the current version of the language.

[L4Vo5]

If you can't access static variables from an instance, then when you're using an object from another script you won't be able to access its static variables at all unless you know its exact type, even though you can access instance variables without knowing the type either.
This is relevant for non-statically typed code, for example a function that receives an object and uses a variable from it. I don't think the function should worry about failing because the object has the required variable but it's static instead of an instance variable, in which case it has to figure out the type somehow.
It can be a problem in statically typed code too. Maybe you realize that a previously-instance variable is better off being made static. If you can't access static variables via an instance, you not only have to change that object's code but also every other piece of code in the project that referenced that variable, as opposed to it being as simple as typing static.