Implement access modifiers, private and protected, to GDScript to protect a class member from being accessed externally.

[Lazy-Rabbit-2001]

This is my first contribution to godot, also the first try sending a pull request. So if there is any mistake, please let me know and I'm here to hear that. :)

Trying implementing godot-proposal-#641.

This pr provides GDScript users a way to protect their members accessibility, and prevent any illegal access from external classes.
We will have @private and @protected annotations in GDScript soon:

Introduction of new annotations

@private

@private annotation allows a member (currently a constant, a variable or a method) to be accessible only within the current class scope:

# In Class A:
@private static var a = 1
# In Class B:
var b = A.a # Error: Could not access external class member "a" because it is private.
# In Class C that inherits from A:
var c = A.a # Error: Could not access external class member "a" because it is private.

@protected

@protected annotation allows a member (currently a constant, a variable or a method) to be accessible only within the current class scope, or the scope of derived classes:

# In Class A:
@protected static var a = 1
# In Class B:
var b = A.a # Error: Could not access external class member "a" because it is protected but accessed from a class that is not derived from "A".
# In Class C that inherits from A:
var c = A.a # OK

Common features:

* A member modified with @private or @protected cannot be modified with @export_* annotations, and vice versa.(This will be obsolescent and check this to know why)

• A member modified with @private or @protected will hide its documentation from auto-generated doc API, even if you use ## to document it.
• These two annotations CANNOT modify local members.

How these are done?

As is known, this two are trying to restrict the access to the member, so we have to modify gdscript_parser.cpp, gdscript_parser.h and gdscript_analyzer.cpp.
In the head file of the parser, since a Node there refers to a lingual element of GDScript, I added an enum AccessRestriction and an enum-ed variable access_restriction under it to control the member's accessibility. When @private or @protected modifies a member, its access_restriction will be modified to other values than AccessRestriction::ACCESS_RESTRICTION_PUBLIC.
Once analyzer starts analyzing external access, we detect the access of the member's access_restriction and figure out the relationship between the accessor class and the accessee class.

Remaining WIP

• Support protection for signals. (GDScript)
• Move protection in static analyzer into reduce_identifier() and reduce_call(). (GDScriptAnalyzer)
• Discussion about if @private and @protected should be keywords or annotations. Allow @export_* work with @private and @protected
• Hide GDScript intelliguess from providing hint of private and protected members.
• Test if Set() and Get() in C# can get access to the access-restricted members in GDScript. (Cross-language programming)
• Compiler, codegen and runtime stuff. (The most difficult part)
• Not sure whether an invalid access should pop an error or a warning. (Warning / error system)
• Unit tests in .gd and .out files. (Unit test)

Tracking after this PR gets merged:

• More optimizations (Such as speeding up, which needs other contributors' help as I'm just a freshman in C++ programming)

[FireCatMagic]

would @public for readability be a bad idea, even if its the default and wouldn't actually change anything?

[Lazy-Rabbit-2001]

would @public for readability be a bad idea, even if its the default and wouldn't actually change anything?

Making an extra @public annotation is not bad though, if you have been used to prefixing an accessibility predicate before an assignment. However, as by default all members are public, I don't think it's worth adding this to the pr since everyone will be and has been familiar with a potential rule that "everything goes public if no any access modifier prefixes a member"

P.S: Plus I think the omission of @public may save more time on declaring multiple public members, isn't it?

[tetrapod00]

I'm not sure it makes sense to tie export functionality to access functionality.

In C#, you can currently do all of the following, and they all have their uses:¹
[Export] public int PublicExport; is an exported public variable, accessible from both the editor and from other scripts. Useful for most export cases.
[Export] private int PrivateExport; is an exported private variable. Useful if you want to expose a value to editor configuration, but not to changes from scripting.
public int PublicVar; is an public variable. Used for values that are exposed to API access, but not to editor configuration.
private int PrivateVar; is an private variable. Used for internal state that don't need to be exposed to configuration or API access.

GDScript is its own specialized language, and it doesn't necessarily need to follow this example. But be aware that tying exporting and access together does not match the current C# implementation, and would prevent one of these four use cases.

Footnotes

1. Using variables instead of properties for simpler example syntax. ↩

[Dynamic-Pistol]

personal nitpick, make it @privacy(PRIVATE|PROTECTED|INTERNAL|PUBLIC) incase other privacy modifiers are added, this also lowers amount of annotations needed

[Lazy-Rabbit-2001]

personal nitpick, make it @privacy(PRIVATE|PROTECTED|INTERNAL|PUBLIC) incase other privacy modifiers are added, this also lowers amount of annotations needed

This does lower the number of annotations, but increases, on the other hand, the amount of constants and length of a modifier.

This is not a very good practice because a longer annotation means longer typing time and because access-protecting annotations will be being frequently used, this may make the efficiency lag down.

But anyways, thanks for your opinion :D

[Lazy-Rabbit-2001]

I'm not sure it makes sense to tie export functionality to access functionality.

In C#, you can currently do all of the following, and they all have their uses:1 [Export] public int PublicExport; is an exported public variable, accessible from both the editor and from other scripts. Useful for most export cases. [Export] private int PrivateExport; is an exported private variable. Useful if you want to expose a value to editor configuration, but not to changes from scripting. public int PublicVar; is an public variable. Used for values that are exposed to API access, but not to editor configuration. private int PrivateVar; is an private variable. Used for internal state that don't need to be exposed to configuration or API access.

GDScript is its own specialized language, and it doesn't necessarily need to follow this example. But be aware that tying exporting and access together does not match the current C# implementation, and would prevent one of these four use cases.

Footnotes

1. Using variables instead of properties for simpler example syntax. ↩

I just imagined some situations where you may use both @export_* and @private or @protected:

• Exported properties that can only be tweaked in the inspector -> @private
• Serialized properties that should be used in specific classes -> @private or @protected

Yep, maybe I need to allow this behavior in my future pushes

[Lazy-Rabbit-2001]

I just found a new place to do access protection, which also fixes a bug that a derived class is able to access private members of its super class.
But it's time for bed. I will continue my work when I get off my bed.

[kroketio]

I prefer the Python way of prefixing with _ to mark private members, while not enforcing that at runtime. In addition, these annotations don't help with the overall readability of GDScript.

[Lazy-Rabbit-2001]

I prefer the Python way of prefixing with _ to mark private members, while not enforcing that at runtime. In addition, these annotations don't help with the overall readability of GDScript.

This makes sense for constants, variables and signals, but not for functions. According to the discussion in the proposal, the single underline prefix may be confusing whether it means private or protected or virtual for a function.

While someone else have suggested me to use double or triple underlines prefix to distinguish them from being vague. Double underlines refers to private a member and triple ones to a protected one. However imo this is also not a good practice, because multiple-underlines prefixes breaks the programming habbit of single underline as a better programming practice in GDScript. Personally speaking, this is not what I'd prefer to do.

You may ask me, "Why not adding single underline suffix to declare a member as a private one?" I've thought of this, and the reason is similar to why not using multiple-underlines prefixes. The less underlines we use, the better and neater the code looks.

As for what you said that these would break the readability, first, the words private and protected are keywords in many programming languages like C++, C# and Java, and the annotations with the same names may help devs who move from these languages to get familiar with these two in GDScript faster. Second, from a perspective of new learners in GDScript, once they have learnt about the usage of the two annotations and wanted to learn some languages like C# or C++, they will get fast familiar with these two as keywords in these language as well. Third, the implementation of access protection tips the new learners in GDScript the importance of guarding external data access, and helping a team member from accessing, by mistake, a data that should not be accessed externally to avoid a disastrous issue.

Saying if it should be an error, which pauses the runtime, or a warning, it needs to be under discussion.

These are my own opinions on your suggestion and thought. Anyways thanks for your thought of this pr :)

[Lazy-Rabbit-2001]

I currently encountered a bug when trying accessing external function from other scripts:

# In Class A:
@private static var a = 1
# In Class B that inherits from A:
static func test():
    a = 10 # Will print error
# In Class C
var d = B.test() # However, this line will not be red; it will throw "Compile error" and "Could not resolve script ClassC.gd."

However, I tried many methods to figure out how to make the class C throws the error "Could not resolve script ClassC.gd" and prevent "Compile error" from being thrown, as well as making the line with var d = B.test() be highlighted in red. But nothing returned to me.
I'd like to hear your solution if you have any good idea :)

P.S. I seemed to use git rebase in an incorrect manner... I just searched how to make correct rebasing and found the answer. Luckily there are no conflicts with modules/gdscript, and I will use correct git rebase workflow next time.

[dalexeev]

Hi @Lazy-Rabbit-2001, thanks for your effort! I haven't reviewed this PR in detail, but I'd like to point out a few things:

1. GDScript is a gradually typed language, we support both dynamic and static sides of the language. This means that the feature can't be implemented on the static analyzer alone. It should at least involve the compiler/codegen and probably also the VM to remove unnecessary runtime checks.
2. I'm not sure that the current implementation of static checks is correct. I think you should use reduce_identifier() and reduce_call() instead of resolve_class_member().
3. There are some conceptual problems with implementing access modifiers with dynamic typing.
   • It's easy to determine what we are accessing, but hard to determine where from. In many programming languages, you can access private members not only from the current instance, but also from other instances of the same class (and its descendants, in case of protected).
   • Another problem is that sometimes we still want to access private fields from the outside, for example from the debugger. Also the problem with private export variables was mentioned above.
   • It might be simpler to have only private members that are effectively protected. This is how Haxe does it.
   • See also Add public/private access modifiers to GDScript and autocompletion improvements godot-proposals#641 (comment).
4. Maybe private/protected should be keywords, not annotations. See Scripting development - Annotation guidelines. Note that annotations are resolved and applied relatively late, so we should take this into account to avoid bugs.

This makes sense for constants, variables and signals, but not for functions. According to the discussion in the proposal, the single underline prefix may be confusing whether it means private or protected or virtual for a function.

I don't think there is any confusion between private and virtual methods. It's just that all built-in virtual methods are private (or protected), you are not supposed to call _ready() from the outside. Some of them have a "public final" counterpart, like _get() and get(). As for custom methods, I believe that you should not name your public virtual methods with an underscore, it should only be a sign of private methods (that may or may not be virtual). For more details, see godotengine/godot-proposals#10500 (comment) and godotengine/godot-proposals#10342 (comment).

I have nothing against access modifiers and consider them a useful language feature that provides more reliability. I would like to have them in GDScript, but we also need to consider the complexity of the implementation and maintainability of the feature.

In my opinion, Python's underscore approach solves the problem of distinguishing between external and internal interfaces quite well and cheaply, requiring only a little discipline and adherence to common conventions from the user. I don't recall ever encountering the problem of accidentally accessing the internals of a class, unlike the problems of the type system, null safety, etc.

[Lazy-Rabbit-2001]

Thanks for replying. I'd like to reply your response by each piece:

1. GDScript is a gradually typed language, we support both dynamic and static sides of the language. This means that the feature can't be implemented on the static analyzer alone. It should at least involve the compiler/codegen and probably also the VM to remove unnecessary runtime checks.

Yep, someone had mentioned this in some social media soft. However, it's my ceiling of contribution to this pr and very difficult for me to understand how these part works. Maybe an expert in this field can help with this problem.

2. I'm not sure that the current implementation of static checks is correct. I think you should use reduce_identifier() and reduce_call() instead of resolve_class_member().

Yes, @HolonProduction tipped this to me yesterday. Maybe I need to look through these parts...

3. There are some conceptual problems with implementing access modifiers with dynamic typing.

   • It's easy to determine what we are accessing, but hard to determine where from. In many programming languages, you can access private members not only from the current instance, but also from other instances of the same class (and its descendants, in case of protected).

Of course, this pr allows it. Because currently it only detects from static analyzer whether the access is from other classes or derived classes or not, it does not affect other instances of the same class to access the data of each other.
But having checked the comment you placed, maybe it will do be harder when it comes to compiling, runtime and debugging.

• Another problem is that sometimes we still want to access private fields from the outside, for example from the debugger. Also the problem with private export variables was mentioned above.

I'm not sure whether the current push may effect the access from the debugger, maybe not since it's currently analyzer-related. As for allowing @export_*-ed properties, this has been in the task list.

• It might be simpler to have only private members that are effectively protected. This is how Haxe does it.

I've thought of removing @protected and only leave @private here. However, when it comes to a data that can be share among the root super class and its all derived classes, @protected is still necessary imo.

• See also Add public/private access modifiers to GDScript and autocompletion improvements godot-proposals#641 (comment).

Understood. But it's okay to think of a better solution during available time after your hard work of one day.
I don't hurry on my way to making this pr request the prime or final review right now. I just upload and push what I thought and everyone can clone my branch, analyzing its mechanics, and share the idea under this pr.

4. Maybe private/protected should be keywords, not annotations. See Scripting development - Annotation guidelines. Note that annotations are resolved and applied relatively late, so we should take this into account to avoid bugs.

I've thought of this, but sadly I have no idea how to make a new keyword works. I only know how to register and make a new keyword (reserved word).

I don't think there is any confusion between private and virtual methods. It's just that all built-in virtual methods are private (or protected), you are not supposed to call _ready() from the outside. Some of them have a "public final" counterpart, like _get() and get(). As for custom methods, I believe that you should not name your public virtual methods with an underscore, it should only be a sign of private methods (that may or may not be virtual). For more details, see godotengine/godot-proposals#10500 (comment) and godotengine/godot-proposals#10342 (comment).

Having checked these two, say if a custom virtual method should not begin with _, based on your proposal, another annotation @virtual should be tagged before a method. But what I think better is to add a new documentational mark ## @virtual before a function to mark it as virtual. (A bit off-topic...)

In my opinion, Python's underscore approach solves the problem of distinguishing between external and internal interfaces quite well and cheaply, requiring only a little discipline and adherence to common conventions from the user. I don't recall ever encountering the problem of accidentally accessing the internals of a class, unlike the problems of the type system, null safety, etc.

If you do believe _ has to be a sign to distinguish between private and public members, then a cheaper solution is to add a new optinal warning named ACCESSING_PRIVATE_MEMBER_WARNING, with its error alternative to be implemented, which not only fits the philosophy of "introducing less, enhancing more", but also solve the major part of problem of external accessing and maintains the compatibility. If choosing this, you can also consider my reply in previous reply.

Anyways, thanks for your response to this pr again. :)

[Lazy-Rabbit-2001]

Due to the reply of dalexeev mentioned in #98136 (comment) that says that the @private and @protected may need to be keywords as annotations may cause potential bugs, I temporarily banned the function of @private and @protected, and introduced their keyword versions: private and protected. (Though these two guys worked very well in static analyzer. RIP these annotations 😢 )

However, keywords are operated harder and more complex than someone operating an annotation, this needs a lot of hard work and multiple tests. I, perhaps together with some experts in GDScript language parsing, will think of how to parse a private and protected members, and how to implement them in a balanced way.

[Zireael07]

Why close?