Skip to content

Conversation

@Lazy-Rabbit-2001
Copy link
Contributor

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 commented Mar 7, 2025

Rebuilt on #98557

About the pr, see #98557
This time I removed __ naming convention, and left _ only.

  • Remaster the codes
  • 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.
  • Unit test

@Ivorforce
Copy link
Member

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?

@Lazy-Rabbit-2001
Copy link
Contributor Author

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.

@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Mar 7, 2025

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

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 marked this pull request as ready for review March 9, 2025 06:48
@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested review from a team as code owners March 9, 2025 06:48
@Lazy-Rabbit-2001
Copy link
Contributor Author

Everything is now ready for review :D

@nubels
Copy link
Contributor

nubels commented Apr 25, 2025

I really like this change! Having the option for Godot to throw errors when accessing private members and methods helps a lot.

return R"("@onready" will set the default value after "@export" takes effect and will override it.)";
case ACCESS_PRIVATE_MEMBER:
CHECK_SYMBOLS(1);
return vformat(R"(Trying to access a private member "%s" from an external place, which would cause problems during runtime.)", symbols[0]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice, but it's not exactly mandatory, to specify what it is instead of "member", similar to what's done in #104955 .

No need to change anything in this PR, it's for future reference

@Mickeon
Copy link
Member

Mickeon commented Jun 28, 2025

I thought we were genuinely championing for this PR, it was just not the time to merge it right now?

@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Jun 28, 2025

I thought we were genuinely championing for this PR, it was just not the time to merge it right now?

No, and please see godotengine/godot-proposals#12685 (comment)

My brain was f**ked and forgot that this is something that would help users to avoid accessing private members.
Ok, reopened

@AThousandShips
Copy link
Member

That's my personal opinion and was about hard access specifiers, not soft access specifiers like this

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 restored the private-remastered branch June 28, 2025 09:34
@Lazy-Rabbit-2001
Copy link
Contributor Author

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.

@produno
Copy link

produno commented Jul 16, 2025

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.

@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested review from a team as code owners July 16, 2025 14:08
@Ivorforce Ivorforce changed the title Reimplent soft access restriction for members prefixed with _ Reimplement soft access restriction for members prefixed with _ Jul 16, 2025
@Lazy-Rabbit-2001 Lazy-Rabbit-2001 requested a review from Mickeon July 16, 2025 15:03
@Lazy-Rabbit-2001
Copy link
Contributor Author

Lazy-Rabbit-2001 commented Jul 30, 2025

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. :/

@AThousandShips
Copy link
Member

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

@pleucell
Copy link

Are there any plans to merge this in 4.6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

8 participants