godot
godot copied to clipboard
godotengine
Reimplent soft access restriction for members prefixed with `_`
Rebuilt on #98557
- Closes: https://github.com/godotengine/godot-proposals/issues/641
- Closes: https://github.com/godotengine/godot-proposals/issues/12580
About the pr, see #98557
This time I removed __ naming convention, and left _ only.
- [x] Remaster the codes
- [x] Fix no warning when the current class has the same private member as the accessed class that is not the super class of the current class.
- [x] Unit test
I do really like this warning, but I wouldn't use it unless I could suppress the warning for a single line. Is that possible with the current code analysis system?
As for the bug, "no warning when the current class has the same private member as the accessed class that is not the super class of the current class", this is because the method check_access_private_member() only deals with the identifier passed in and a flag about if the pattern to be reduced or having been reduced is a call or not, in which there is a line:
if (parser->current_class->has_member(p_identifier->name)) {
return;
}
For example, if A and B has the same signatured function, while they are not derived from the other one:
class A:
var _t := 1
class B:
var a := A.new()
var _t := 2
func _init() -> void:
a._t = 3 # No warning as B has a member with the same name `_t`
Dunno how to deal with this as during reduction you cannot infer the details of the subscript base, except its type and name.
I do really like this warning, but I wouldn't use it unless I could suppress the warning for a single line. Is that possible with the current code analysis system?
You can ignore the warning with @warning_ignore("access_private_member"), or use @warning_ignore_start("access_private_member") (and ends with @warning_ignore_restore("access_private_member")) for access to multiple private members
I really like this change! Having the option for Godot to throw errors when accessing private members and methods helps a lot.
I thought we were genuinely championing for this PR, it was just not the time to merge it right now?
I thought we were genuinely championing for this PR, it was just not the time to merge it right now?
~~No, and please see https://github.com/godotengine/godot-proposals/issues/12685#issuecomment-3015107678~~
My brain was f**ked and forgot that this is something that would help users to avoid accessing private members. Ok, reopened
That's my personal opinion and was about hard access specifiers, not soft access specifiers like this
Ok, since vnen agreed the implementation of this pr, I'm gonna squash it tonight to make this ready to merge. Maybe it will be in 4.5, or in 4.6. But no matter in which version, this pr can be finally done.
Does this mean we will no longer get proper access modifiers?
Using an underscore as a private variable means every time I want to change a var from private to public, or vice versa, I now break save game compatibility with my next update. The options now are to either wrap each saveable variable in a getter/setter (Which are extremely slow in GDScript) or check each variable when I save or load and adjust the saved/loaded name.
These are both bad for a language specific to an environment where save game compatibility is extremely important as are save/load times. I have seen the arguments before 'But Python uses it', sure, but Python is not a language created specifically for game development.
Using an access modifier causes me or anyone else none of these problems.
Does this mean we will no longer get proper access modifiers?
We don't introduce the feature in a forced way (whether by adding private or @private) because of runtime check which will reduce extra performance during your gameplay. Maybe in GDScript 3.x this could get solved, hopefully.
Using an underscore as a private variable means every time I want to change a var from private to public, or vice versa, I now break save game compatibility with my next update. The options now are to either wrap each saveable variable in a getter/setter (Which are extremely slow in GDScript) or check each variable when I save or load and adjust the saved/loaded name.
Yeah, I understand your point, but as I said this does not break compatibility as possible and you can configure it and turn it off. So you can keep the current codestyle you've been using.
I have seen the arguments before 'But Python uses it', sure, but Python is not a language created specifically for game development.
How does Python has anything to do with this topic? I don't understand. :/
How does Python has anything to do with this topic?
They mentioned it because it's a common argument in favor of this, they're not making the argument, they're referencing it
Are there any plans to merge this in 4.6?
I've asked many times in the rocket chat but nobody has given this a look yet... sad Hope the members would notice this and get this a key approval and merge asap...
Could you double check whether you included unrelated changes? The diff contains some create dialog changes in editor interface, for which it's not directly obvious to me how they relate to this PR.
Yeah, I understand your point, but as I said this does not break compatibility as possible and you can configure it and turn it off. So you can keep the current codestyle you've been using.
This wasn't really the point I was trying to make. I do want a way to mark my variables as private or public, so I will use whatever is ultimately implemented. But imo having proper access modifiers is the correct way to go, for reasons previously mentioned. So if this is going to hinder or even halt that feature being merged in the future, then that is not good. Hence my question. It also helps me decide if I should use this feature at all if it ends up being merged, as I don't want to break all saves in the future when needing to change.
For some of us working on large projects, whom plan to release into Early access, its helpful to give us an idea of what's planned or likely to change/happen regarding certain features, so we can then make informed decisions within our own codebase.
Yeah, I understand your point, but as I said this does not break compatibility as possible and you can configure it and turn it off. So you can keep the current codestyle you've been using.
This wasn't really the point I was trying to make. I do want a way to mark my variables as private or public, so I will use whatever is ultimately implemented. But imo having proper access modifiers is the correct way to go, for reasons previously mentioned. So if this is going to hinder or even halt that feature being merged in the future, then that is not good.
The feature won't eliminate the solution to introduce either a keyword private or a modifying annotation @private, and you can easily upgrade your grammar from what it presents in this pr to what you have suggested for the future releases (perhaps) when needed because this doesn't take too much time to do grammar migration.
Hence my question. It also helps me decide if I should use this feature at all if it ends up being merged, as I don't want to break all saves in the future when needing to change.
This is implemented by warnings, so you can disable them in your project settings or enable them as you wish. The warnings will not break your current project as long as you do not switch the warnings to errors.
@produno Can you explain which feature you're using that changing variable names breaks compatibility? I assume you're passing some of your objects directly into one of the serialization functions, without specifying key names explicitly?
For some of us working on large projects, whom plan to release into Early access, its helpful to give us an idea of what's planned or likely to change/happen regarding certain features, so we can then make informed decisions within our own codebase.
While there was a consensus in a previous GDScript meeting that this PR was desirable, it is still up for debate. If we find it is not mergeable due to other concerns, it will not be merged. If you'd like to stay up to date with plans, the best place to find out is the dev releases' blogposts on the website. Changes are always up for debate until the moment they're merged (and rarely, they are reverted even after that).
@produno Can you explain which feature you're using that changing variable names breaks compatibility? I assume you're passing some of your objects directly into one of the serialization functions, without specifying key names explicitly?
I save the property name with its value, then upon load I use set() to restore the value where required. I'm sure I am not only person serialising in this way. It just means if this is added and proper access modifiers will likely to be added in the future, then I will refrain from using this feature.