search

godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Add access levels for class method and variable in GDScript #11039

Open zjin123 opened 3 weeks ago

zjin123 commented 3 weeks ago

Describe the project you are working on

A cross-platform app

Describe the problem or limitation you are having in your project

I think hiding class method/variable from outside may reduce potential bugs and makes code more clean.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Allow access level modifier on class method and variable.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Implemented in https://github.com/godotengine/godot/pull/98606

UPDATE:
internal changed to protected. pub changed to public. Project setting for global default access level is removed. Add @public annotation. Add warning when the script does not specify default access level explicitly.

Add keywords public protected private as access level modifier for class method and class variable and an additional keyword readonly for class variable.

public the method/variable can be accessed by anyone. protected the method/variable can be only accessed by the class defining it and its subclasses. private the method/variable can be only accessed by the class defining it. readonly the variable can be read by anyone but can be only assigned to by the class defining it.

All of the four keywords can be used as identifier.

class_name Foo
extends Node

public var a = 1
protected var b = 2
private var c = 3
readonly var d = 4

private func _init():
    pass

public func foo():
    pass

public static func Create() -> Foo:
    return Foo.new()

# the following are OK
public var public = 0
protected var protected = 0
private var private = 0
readonly var readonly = 0

For class method/variable without explicit access level modifier in one script, add three annotations @private @protected @public. If no such annotation is applied, default to public.

@private
extends Node
var a = 1 # a is private
@protected
extends Node
var a = 1 # a is protected
@public
extends Node
var a = 1 # a is public
extends Node
var a = 1 # a is public

Add IMPLICIT_DEFAULT_ACCESS_LEVEL warning when a script does not specify any of the three default access level annotations. Programmer may use this warning to prevent exposing script as public accidentally by forgetting setting default access level. This warning is ignored by default.

When IMPLICIT_DEFAULT_ACCESS_LEVEL is changed to warn/error,

# If IMPLICIT_DEFAULT_ACCESS_LEVEL is set to warn/error, 
# produce a warning/error because no default access level annotation is specified.
extends Node
var a = 1
# If IMPLICIT_DEFAULT_ACCESS_LEVEL is set to warn/error, this is OK.
@public
extends Node
var a = 1

Limitation

This feature works only at compiling time and rely on typed code. It has no effect on untyped code both at compiling time and at runtime. For example, the following untyped code will compile and run without triggering any access level check. It is up to the user to decide to opt-in this feature (by writing types and access level modifiers) or not.

extends Node
class Foo:
    private func foo():
        pass

func bar(obj):  # <-- Because obj is untyped, there is no access level check.
    obj.foo()

func test():
    bar(Foo.new())

If this enhancement will not be used often, can it be worked around with a few lines of script?

No.

Is there a reason why this should be core and not an add-on in the asset library?

Need to modify GDScript's compiler.

Chaosus commented 3 weeks ago

Seems like a duplicate of https://github.com/godotengine/godot-proposals/issues/641

Lazy-Rabbit-2001 commented 3 weeks ago

I think this consolidates #641 with some extra usages

Flynsarmy commented 3 weeks ago

Personally I'd prefer protected keyword over internal. Looking at all the related PRs and proposals they too all seem to use protected.

Lazy-Rabbit-2001 commented 3 weeks ago

Personally I'd prefer protected keyword over internal. Looking at all the related PRs and proposals they too all seem to use protected.

The more ambiguous reason is that inner may lead new gdscript users to the concept of private, which only allows a member to be accessed from the same class.

This also mentioned me of what dalexeev tipped under that pr of mine. In some languages like Haxe, private = protected. So if the author insist using private and inner as modifiers, it's better to swap their functions.

dalexeev commented 3 weeks ago

Limitation

This feature relys on static typing. So the following code works (and it should not):

extends Node
class Foo:
    private func foo():
        pass

func bar(obj):  # <-- obj's type is unknown
    obj.foo()

func test():
    bar(Foo.new())

because the type of obj cannot be determined at compiling time.

GDScript is a gradually typed language, i.e. it combines the qualities of both dynamically and statically typed languages. Type hints are optional and help with static analysis and performance. However, typed code must easily interoperate with untyped code. We don't want to limit users of untyped GDScript and there are no plans to make GDScript a fully statically typed language, as far as I know. So this limitation raises serious concerns about whether we should include the feature in this form.

HolonProduction commented 3 weeks ago

For new project, add a project setting gdscript/default_access_level which is an enum of Public, Internal and Private. It will change the default access level for all class methods and class variables without modifier for the whole project.

I really don't think we should make GDScript behaviour configurable through project settings.

zjin123 commented 3 weeks ago

Limitation This feature relys on static typing. So the following code works (and it should not):

extends Node
class Foo:
    private func foo():
        pass

func bar(obj):  # <-- obj's type is unknown
    obj.foo()

func test():
    bar(Foo.new())

because the type of obj cannot be determined at compiling time.

GDScript is a gradually typed language, i.e. it combines the qualities of both dynamically and statically typed languages. Type hints are optional and help with static analysis and performance. However, typed code must easily interoperate with untyped code. We don't want to limit users of untyped GDScript and there are no plans to make GDScript a fully statically typed language, as far as I know. So this limitation raises serious concerns about whether we should include the feature in this form.

Thanks for the reply. The feature is indeed optional. It is a pure static checking for typed code without involving any runtime change. For untyped code, this feature actually has no effect and those codes will work like usual. Only when a user decides to opt-in this feature by explicitly marking types and applying access level modifier, the feature can provide additional check at compiling time as a bonus. If a user chooses to stay with untyped code, then the feature appears as it does not exist (like the code in the limitation section will compile and run as usual without triggering any access level check at all). Hence I think it does not limit user of untyped code or force them to make any change. Purely optional. It is all up to user's decision.

zjin123 commented 3 weeks ago

For new project, add a project setting gdscript/default_access_level which is an enum of Public, Internal and Private. It will change the default access level for all class methods and class variables without modifier for the whole project.

I really don't think we should make GDScript behaviour configurable through project settings.

Indeed. I will remove it.

zjin123 commented 3 weeks ago

Personally I'd prefer protected keyword over internal. Looking at all the related PRs and proposals they too all seem to use protected.

Sure. I will change internal to protected. Actually I have no preference over those two :)

Flynsarmy commented 3 weeks ago

One other change I'd like to see is pub changed to public. You're using the full word for private and protected, but abbreviating public to pub which is inconsistent and may cause confusion.

zjin123 commented 3 weeks ago

One other change I'd like to see is pub changed to public. You're using the full word for private and protected, but abbreviating public to pub which is inconsistent and may cause confusion.

I just pick pub from Rust as it is short and convenient... But you are right, I will change it.

nbaum commented 3 weeks ago

Thanks for the reply. The feature is indeed optional. It is a pure static checking for typed code without involving any runtime change. For untyped code, this feature actually has no effect and those codes will work like usual. Only when a user decides to opt-in this feature by explicitly marking types and applying access level modifier, the feature can provide additional check at compiling time as a bonus. If a user chooses to stay with untyped code, then the feature appears as it does not exist (like the code in the limitation section will compile and run as usual without triggering any access level check at all). Hence I think it does not limit user of untyped code or force them to make any change. Purely optional. It is all up to user's decision.

While developers can opt-in to typing, they cannot opt another developer out of typing by "forgetting" the type of a variable. e.g.

class Foo:
  var x := "hello"

func _ready() -> void:
  var y : Variant = Foo.new()
  y.x = 3.0 # valid at compile time, but fails at runtime because I can't make Foo.x ignore its own type

But this proposal and associated PR do trivially allow me to bypass somebody else's access restrictions. e.g.

class Foo:
  private var x := "hello"

func _ready() -> void:
  var y : Variant = Foo.new()
  y.x = "world" # valid at compile time, and doesn't fail at runtime: Foo.x ignores its own access restriction

This behaviour may be surprising, especially for the author of Foo.

While I like the feature, I worry about this limitation.

girng commented 3 weeks ago

reduz's initial core values for gdscript were centered around being easy to learn and fast prototyping.

even though gdscript has changed over the years, i still think those values are integral to godot. if the dev needs access to variables/functions that are private in c++, they can compile from source.

vice versa if they need to implement access specifiers for classes in gdscript, they can make a module or use gdnative. imo, piling on extra keywords in gdscript can make it overly complex for the developer

Flynsarmy commented 3 weeks ago

if they need to implement access specifiers for classes in gdscript, they can make a module or use gdnative

This is currently the 11th most requested feature in godot-proposals and that's not even including all the duplicates of the same request. Suffice to say, the desire by the community is high enough for it to be added to the engine IMO.

girng commented 3 weeks ago

if they need to implement access specifiers for classes in gdscript, they can make a module or use gdnative

This is currently the 11th most requested feature in godot-proposals and that's not even including all the duplicates of the same request. Suffice to say, the desire by the community is high enough for it to be added to the engine IMO.

https://www.mtfca.com/discus/messages/506218/541602.png

core functionality shouldn't be added just because it's popular. it should be gauged whether it's truly needed. there's already two solutions the developer can do: compile from source or use gdnative. the more stuff that gets piled in gdscript, the more bugs that arise from it (which can negatively effect godot's development).

i've seen numerous threads in this section where users are wanting to turn gdscript into some god-like c++ language with STL support, access specifiers, etc. imo it goes against the entire purpose the language was designed for..

TX256 commented 3 weeks ago

I've grown to like the simple and succinct Python-style underscore for private stuff, and I believe GDScript manages just fine with that.

As a bonus, I like how explicit the underscore is, I can see from the function call-site and variable usage, whether the element is private.

Personally, regarding GDScript, I would simply make a single underscore account for both private & protected, and not introduce any additional keywords.

duckbanditdan commented 6 days ago

In my own GDScript code I am currently using _single_underscore for protected (as this seems consistent with GDScript's usage for _process(), ready() and so on), __double_underscore for private and no_underscore for public. It works but I do wish I had some way to enforce it for my project. I would much prefer having the option to use access modifiers rather than relying on my own consistency.

Developers can become aware of issues in their code in a number of ways:

The greater the percentage of issues that can be discovered at the early end of this list the better. At the extreme ends of the list you have:

In my view, access modifiers add minimal complexity to users of the language, are easy to explain and understand, and have the potential to push a large number of code issues from late discovery to early discovery (particularly when working in teams or using GDScript libraries written by others). I don't think they are as important as ensuring type safety is always available, but I do think they would be of great benefit to GDScript and its users.

I agree that popularity shouldn't be the only metric in determining the importance of a proposal, but issues like this one are popular for a reason - they have the potential to deliver enormous value to users of GDScript.

Brandbob commented 5 days ago

Another point in favor of access modifiers is that Godot's current nomenclature for identifying private functions is the same as identifying virtual functions. A private access modifier would resolve this issue, however another solution to the shared identification problem would be to instead change the nomenclature for identifying virtual functions. This would purely just be a change to the common practices of GDScript.

Perhaps something as simple as v_ which would look as such:

var my_public_value

var _my_private_value

func my_public_function():
  pass

func _my_private_function():
  pass

func v_my_virtual_function():
  pass

My personal vote however is to add a private access modifier used as such:

var my_public_value

private var my_private_value

func my_public_function():
  pass

private func my_private_function():
  pass

func _my_virtual_function():
  pass

Having the functionality of the private access modifier in GDScript would be great, the encapsulation, reduction of autofill junk, and of course the control of access are all very good reasons to add the optional keyword.

Overall, I think that at least the issue of shared nomenclature between virtual and private identification needs to be addressed, whether that be by adding new keywords or by using a different prefix for either private variables/functions or virtual functions, the latter being much easier.

Lazy-Rabbit-2001 commented 4 days ago

Another point in favor of access modifiers is that Godot's current nomenclature for identifying private functions is the same as identifying virtual functions. A private access modifier would resolve this issue, however another solution to the shared identification problem would be to instead change the nomenclature for identifying virtual functions. This would purely just be a change to the common practices of GDScript.

...

Having the functionality of the private access modifier in GDScript would be great, the encapsulation, reduction of autofill junk, and of course the control of access are all very good reasons to add the optional keyword.

Overall, I think that at least the issue of shared nomenclature between virtual and private identification needs to be addressed, whether that be by adding new keywords or by using a different prefix for either private variables/functions or virtual functions, the latter being much easier.

Access modifier would solve the naming concern and bring to real error-catching for invalid access, which should be pointed out and agreed. However, considering the performance on the analyzer and the flexibility of the feature, a warning system would be better, and since we have a precedent UNUSED_PRIVATE_CLASS_VARIABLE, which is triggered when there is any _-prefixed member that is never used anymore, a prefix + warning would be a more balanced solution imo. For virtual methods, at first I thought the same way as yours that _-prefixed functions will bring ambiguity when it comes to whether it should be private or virtual. However, I got the reply from one of the dev: In his opinion, a virtual method should be private (protected). Hence the naming is actually fit to what it should be accessed by. Then you may complain about calling it. Same, at first I was confused about this, but then I realized that a wrapper is a better solution:

# This is a virtual method that should be overridden by a derived class.
func _my_method() -> void:
    pass

# If you need to call the `_method()`, you should call this wrapper, because a wrapper may provide more encapsulation for calling your virtual method.
func call_virtual_my_method() -> void:
    <encapsulations>
    _my_method()

Since you have more encapsulations and you can even override the wrapper to customize your methods, I don't think _ is ambiguous anymore. Instead, it's reasonable, and more flexible and readable.

Note: Only personal view on this.

duckbanditdan commented 4 days ago

Access modifier would solve the naming concern and bring to real error-catching for invalid access, which should be pointed out and agreed. However, considering the performance on the analyzer and the flexibility of the feature, a warning system would be better, and since we have a precedent UNUSED_PRIVATE_CLASS_VARIABLE, which is triggered when there is any _-prefixed member that is never used anymore, a prefix + warning would be a more balanced solution imo. For virtual methods, at first I thought the same way as yours that _-prefixed functions will bring ambiguity when it comes to whether it should be private or virtual. However, I got the reply from one of the dev: In his opinion, a virtual method should be private (protected). Hence the naming is actually fit to what it should be accessed by. Then you may complain about calling it. Same, at first I was confused about this, but then I realized that a wrapper is a better solution: ~ @Lazy-Rabbit-2001

I don't really mind but I'm not sure why virtual would be conflated with access modifiers. Based on their usage, the current set of built in virtual methods should all be either private or protected but that doesn't mean that ALL virtual methods should be. I don't think all public virtual methods are anti-patterns, in many simple cases a wrapper only adds bloat.

I'd prefer access modifiers (and separate keywords for virtual and abstract) but if the analyzer warning is more "Godot" then that could also be a good solution. They are configurable and (so long as you remember to turn them on when you start a new project) do find errors in the editor while you write. It does make the language stricter however, as it enforces a naming convention. This is also a little inconsistent as other keywords, like "const" are used rather than enforcing constant names to ALL_CAPS. I'd be content with it all the same.

Side note: I've been considering most of the built in virtual methods as protected up until now, but on reflection maybe they should all be seen as private. The answer would be a lot more obvious if GDScript used access modifiers!

Brandbob commented 4 days ago

For virtual methods, at first I thought the same way as yours that _-prefixed functions will bring ambiguity when it comes to whether it should be private or virtual. However, I got the reply from one of the dev: In his opinion, a virtual method should be private (protected). Hence the naming is actually fit to what it should be accessed by.

  • @Lazy-Rabbit-2001

This makes sense, however as the current nomenclature stands, there is still an issue of having a private function having an identical prefix as a virtual function, where as it may not be intended for the private function to be overrode. If one were to see the _ prefixed function in the autofill options, they may believe it is meant to be overrode and override it, which isn't a terrible problem, but on the other hand one could see the function as private and abide by the naming conventions and not use it or override it.

I can think of some solutions that a user could do in order to mitigate this, such as commenting, or naming their functions better to convey their purpose regarding being private or virtual, but I believe a better solution without changing engine code would be to use the separate naming convention for virtual functions like the v_ I brought up in my original comment.

I don't really mind but I'm not sure why virtual would be conflated with access modifiers. Based on their usage, the current set of built in virtual methods should all be either private or protected but that doesn't mean that ALL virtual methods should be. I don't think all public virtual methods are anti-patterns, in many simple cases a wrapper only adds bloat.

  • @duckbanditdan

I agree that not all virtual functions should be private, this aligns with GDScript's everything is public mindset, and also points to the issue regarding the _ prefix used for identifying virtual functions. To this point I would like to append to my original idea of a different virtual identifier prefix: v_, another underscore to indicate whether it is meant to be private: _v_.

So here is my updated idea:

var my_public_value

var _my_private_value

func my_public_function():
  pass

func _my_private_function():
  pass

func v_my_virtual_function():
  pass

func _v_my_private_virtual_function():
  pass

Again, this is just changing the naming conventions for identifying the desired private or virtual attribute of functions.

As for internal virtual functions like ready, init, and the processes, here what they would look like, assuming they are private:

func _v_init():
  pass

func _v_ready():
  pass

func _v_process(delta):
  pass

func _v_physics_process(delta):
  pass

Not going to lie, I am not a huge fan of the _v_ for private virtual functions, it seems too lengthy, but I for sure believe that there needs to be some sort of distinction between private, virtual, and potentially private virtual functions within the function name that is standardized in GDScript documentation such as the _ prefix for private/virtual.

Also for the scope of what I am saying, I think it is related to this proposal but should probably be made into its own for further discussion.

Lazy-Rabbit-2001 commented 4 days ago

This makes sense, however as the current nomenclature stands, there is still an issue of having a private function having an identical prefix as a virtual function, where as it may not be intended for the private function to be overrode. If one were to see the _ prefixed function in the autofill options, they may believe it is meant to be overrode and override it, which isn't a terrible problem, but on the other hand one could see the function as private and abide by the naming conventions and not use it or override it.

I think we need to add a note under the documentation about saying that "a built-in method beginning with _ are virutal methods that you can override", as this sentence contains less information about it:

Note: In fact, these methods are accessibly protected, which means that these methods should not be called or overridden by any class not derived from the owner class of the methods. For example, an object that does not extend Node should not, by convention, call or override an _-prefixed method from Node (because it is not defined in the current class, or one of the ancestor classes of the current class). As Godot does not support private/protected members in GDScript, it can technically be accessed (This should be tweaked after one of the access pulls gets merged). but this is not encouraged and is unrecommended. Calling a virutal method from an external non-derived class is not recommended either, and it would be risky to call a virtual method directly. It would lead to an unexpected behavior unless you know what you are doing, or the caller is the wrapper of the virtual method, which is exposed to other class for safely calling this virtual function. If your script is very complex, it is recommended to encapsulate your algorithms from a built-in virtual method into public functions (methods), and call them when you need it.

And forgot to mention that if you looking into the source C++ code you will find that almost each virtual method is called via a wrapper method.

I agree that not all virtual functions should be private, this aligns with GDScript's everything is public mindset, and also points to the issue regarding the _ prefix used for identifying virtual functions. To this point I would like to append to my original idea of a different virtual identifier prefix: v_, another underscore to indicate whether it is meant to be private: _v_.

So here is my updated idea:

var my_public_value

var _my_private_value

func my_public_function():
  pass

func _my_private_function():
  pass

func v_my_virtual_function():
  pass

func _v_my_private_virtual_function():
  pass

Again, this is just changing the naming conventions for identifying the desired private or virtual attribute of functions.

As for internal virtual functions like ready, init, and the processes, here what they would look like, assuming they are private:

func _v_init():
  pass

func _v_ready():
  pass

func _v_process(delta):
  pass

func _v_physics_process(delta):
  pass

Not going to lie, I am not a huge fan of the _v_ for private virtual functions, it seems too lengthy, but I for sure believe that there needs to be some sort of distinction between private, virtual, and potentially private virtual functions within the function name that is standardized in GDScript documentation such as the _ prefix for private/virtual.

Also for the scope of what I am saying, I think it is related to this proposal but should probably be made into its own for further discussion.

Renaming built-in methods with the prefix would lead to compatibility issues, so imo this is not a good idea... :(

dalexeev commented 4 days ago

Note that unlike access modifiers, virtual methods have no runtime penalty[^1]. We always know the base class, no matter if static typing is used or not. So we can implement virtual/abstract/final annotations or keywords that will be resolved during static analysis and will always work properly.

The underscore would be only an indicator of a private/protected class member and would not be directly related to virtual methods. It just so happens that most native virtual methods are private/protected; you are not supposed to call these methods, at least not from the outside. Where this is not true, there are paired methods like _get() and get(). As for custom public virtual methods, you should not name them with a leading underscore.

See also:

[^1]: This is not true for languages ​​that have static call dispatch (virtual methods typically use vtables and this has overhead), but in GDScript all functions are already virtual by default.

duckbanditdan commented 4 days ago

@Brandbob & @Lazy-Rabbit-2001 Unfortunately I'd have to agree that the _v_ prefix is too long. I'd still prefer keywords anyway as they are:

I also agree with @dalexeev on the (lack of a) relationship between virtual and private methods. Personally, I think they should each be their own keyword.

edit: also typing _v_ in many web forms, like this one, ends up as v and then you have to do an edit to fix it.

Brandbob commented 4 days ago

Yeah it seems as though there are plenty of good proposals out there that resolve the issue I have for distinguishing private/virtual functions. And as @dalexeev said, the _ is not necessarily meant to be used as an indicator for virtual functions, it just happens to be used as such for all the built-in virtual functions as they are also private.

Therefore I strongly support the addition of either keywords/annotations regarding virtual functions. The proposals regarding the _ in current virtual functions and the ideas for keywords/annotations from @dalexeev 's comment are here as well for posterity: