Skip to content

Private getters/setters #12204

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 21, 2025
Merged

Conversation

RblSb
Copy link
Member

@RblSb RblSb commented Apr 25, 2025

var prop(private get, private set) syntax implementation.

There is many places where this read/write check can be done, so not sure if i did it correctly.

Currently errors highlights full obj.prop expression instead of prop part.

I also added unification from (private get/set) to (get/set) for interfaces, so you can have read-only field in interface and implement it as read-write, like in other langs.

Closes #3053

@Simn
Copy link
Member

Simn commented Apr 25, 2025

Nice!

I still can't remember if we ever agreed to actually do this, but I'll just make the executive decision to merge it if CI agrees. I scrolled through the changes and it looks good, though it's not super easy to review something like this because it affects so many places and at the same time doesn't since AccCall = AccPrivateCall most of the time.

One question: should public var foo(private get, private set):Int; really be allowed? I can't tell what the public is supposed to do here, the intent clearly seems to be to have this property not-public, so I wonder if this should be an error.

Also, a follow-up I'd like to look into later: I hate that FProp still works with plain strings... it's the parser's job to turn strings into something meaningful, so having these strings here and then matching them in the typer is stupid. We already deprecated anything other than get and set a long time ago, so let's clean this up.

@skial skial mentioned this pull request Apr 25, 2025
1 task
@RblSb RblSb force-pushed the private-get-set branch from 26531cd to 1f41196 Compare May 6, 2025 15:35
@Simn
Copy link
Member

Simn commented May 21, 2025

Nobody protested so let's do this.

@Simn Simn merged commit 1c13016 into HaxeFoundation:development May 21, 2025
47 checks passed
@RblSb RblSb deleted the private-get-set branch May 21, 2025 08:22
kLabz added a commit that referenced this pull request Jun 10, 2025
@@ -1126,6 +1126,7 @@ and encode_var_access a =
| AccInline -> 5, []
| AccRequire (s,msg) -> 6, [encode_string s; null encode_string msg]
| AccCtor -> 7, []
| AccPrivateCall -> 8, []
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for the next time you deal with macro encode/decode: those values are linked to the order of the enum values (here, haxe.macro.Type.VarAccess) so changes need to reflect that.

Fixed with 54ce7ff

Copy link
Contributor

Choose a reason for hiding this comment

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

(and yeah it's on us for not properly code reviewing °°)

Copy link
Member

Choose a reason for hiding this comment

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

And by us he means me!

Copy link
Contributor

Choose a reason for hiding this comment

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

And by us he means me!

Well I should also have reviewed this properly 😅 I had a month (was not available for some of it, but not all)

Copy link
Member

Choose a reason for hiding this comment

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

It's ok I forgive you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Private getters/setters
3 participants