Make object/class member variables public by default #12932
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12932 +/- ##
==========================================
- Coverage 82.05% 78.18% -3.88%
==========================================
Files 160 150 -10
Lines 194679 152275 -42404
Branches 43696 39254 -4442
==========================================
- Hits 159743 119055 -40688
+ Misses 22073 20971 -1102
+ Partials 12863 12249 -614
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
No way to declare a member read only? It's a shame to sacrifice safety for marginal simplicity. And with a performance hit thrown it; it's almost 4x slower using a Getter(). The main goal of |
If we want to keep the object method and member accessibility consistent, |
|
If we want to keep the object method and member accessibility consistent
maybe adding another keyword 'readonly',
but make the default (if did not specify) then it is 'prevent' (vs not the 'public') means in pkg.
// if so, if we are working all with 'keyword', then i again recommend you use 'private' keyword too (vs not leading underscore)
…--
shane.xb.qian
|
|
simplest way was just basic class which none (get rid) of those keyword, i'm ok with that, it's good enough for scripting. |
I don't understand that. Currently there is I thought read only member by default was elegant; it's simple to explain and offers both safety and performance. Here's some ways to look at it
|
|
I would definitely weigh in in favor of changing the current system, where an unadorned member is read-only, given that it's different from most (all?) other mainstream languages.
|
The
Yes.
With the latest code, you will get this behavior (without this PR). If this is the
This PR introduces the behavior in 2 except for the
For 3, as described above, we don't need to make any further changes. If we all agree approach 3 is acceptable (even though it is not consistent between Regards, |
The PR also takes out the internal support for readonly, rather than just at the parsing/specification level. I was looking at what it would take to introduce a |
|
+1 for I don't think it's needed to make it an exclusive keyword, it can just be checked inside classes before member declaration, as other 'keywords' currently work ( |
Thanks. Assuming you're not planning to do read only, I'll take a look tomorrow. (What other plans?). Idea is basically put back |
|
Oh, and |
|
|
|
When I build objmember, with no modifications, and run this I see two problems
Here's a test |
|
Oh, and this is what I'm running |
|
Has readonly ever worked? I just ran some tests with vim I built yesterday, standard vim9 stuff. Happily writes to read only variable. |
|
I looked around to get a feel... The following fixes the "access to private static" var in the example, if you want to include it. Not tested much. |
|
After the shock of finding out that readonly has probably never worked, the I wonder if it is true that this can always be caught at compile time? Could a run time check ever be needed. The parser/variable-state was simple/straightforward to get working. But the compiler seems to ignore it. Sigh. |
|
I meant generate_store_lhs |
|
Hi,
On Mon, Aug 28, 2023 at 12:14 PM errael ***@***.***> wrote:
When I build objmember, with no modifications, and run this I see two
problems
1. Able to access private static
2. Compile error when trying to write public static
I am able to reproduce the second issue even using a two months old image.
It looks like writing to a class member from a def function never worked.
Regards,
Yegappan
…
Here's a test
vim9script
class C
this._mpfoo: number
static _spfoo: number
this.mfoo: number
static sfoo: number
def new()
this._mpfoo = 1
_spfoo = 2
this.mfoo = 201
sfoo = 202
enddef
endclass
def F()
var o = C.new()
#echo "_mpfoo" o._mpfoo
echo "_spfoo" C._spfoo ### <<<<<<<<< able to access private static
echo "mfoo" o.mfoo
echo "sfoo" C.sfoo
o.mfoo = 301
#C.sfoo = 302 ### <<<<<<<<< compilation fails
echo "mfoo" o.mfoo
echo "sfoo" C.sfoo
enddef
F()
finish
|
|
Hi,
On Mon, Aug 28, 2023 at 6:47 PM errael ***@***.***> wrote:
I looked around to get a feel... The following fixes the "access to
private static" var in the example, if you want to include it. Not tested
much.
diff --git a/src/vim9expr.c b/src/vim9expr.c
--- a/src/vim9expr.c
+++ b/src/vim9expr.c
@@ -434,14 +434,25 @@
{
// load class member
int idx;
+ ocmember_T *m;
for (idx = 0; idx < cl->class_class_member_count; ++idx)
{
- ocmember_T *m = &cl->class_class_members[idx];
+ m = &cl->class_class_members[idx];
if (STRNCMP(name, m->ocm_name, len) == 0 && m->ocm_name[len] == NUL)
break;
}
if (idx < cl->class_class_member_count)
{
+ // TODO: Would kindof prefer ***@***.*** what do you think)
+ // even though it's more likely to get a cache miss.
+ // if (m->ocm_access == VIM_ACCESS_PRIVATE
+ // && !inside_class(cctx, cl))
+ // Maybe not, *name == '_' is simple to look at but...
+ if (*name == '_' && !inside_class(cctx, cl))
+ {
+ semsg(_(e_cannot_access_private_member_str), m->ocm_name);
+ return FAIL;
+ }
*arg = name_end;
return generate_CLASSMEMBER(cctx, TRUE, cl, idx);
}
This is the proper fix for this issue. A similar check is used before this
block of code for
readonly object members. Can you open a PR for this with a test for this
condition?
- Yegappan
|
OK, but not tonight, tomorrow. I haven't looked at the "write public static compile error". For my current work area, I took your objmember PR, transplant to work area, rebased to current master, added the readonly parse, I'll need to unwind and stash. I just saw
Assuming you meant private. |
|
upd & link & refer : #11827 |
|
I'm going to put this on the shelf while I put together the PR for preventing writing to private members. The |
|
Is there some strong objection to requiring a keyword modifier? Why is a default needed at all? We could require The language uses strictly unnecessary keywords everywhere. In declarations, in particular, they're cheap and clear. This would also obviate the problem with initialised declarations looking the same as plain assignments. |
Are you suggesting |
The discussion should be about whether or not the semantics should be available in vim9script. I think it best to not discuss specific names, that derailed previous discussions. |
|
To summarize, we have the following ways to declare and control access to object and class member variables currently (without the changes from this PR): |
|
> can reduce your safety concern, though I still think not much people really would use vim9 to wrote OOP
But we are specifically talking about OOP. Someone using it should be assumed when considering a comment.
I mean you can have a bit confident to use that `public`, that default actually was pkg only, not much safety problem in vim9 scripting, even really someone to write huge project in vim9 OOP.
if so, not much getter() required.
…--
shane.xb.qian
|
|
@yegappan I appreciate that in your position you want to observe and let the discussion flow. But I would appreciate if you can find a way to express your opinions on the various topics. Not just state the facts. How useful are read-only semantics? How big are projects going to get (I've never written vimscript or looked into the ecosystem, but I've seen projects grow and grow no matter the language). How important is safety (it is strongly typed...) and how do the available syntax/semantics affect that. What do you think about the performance hit. Anything else? |
|
Currently |
I developed and maintain the following large and widely used Vim plugins: https://github.com/yegappan/lsp Based on this experience, I can say that type safety and access control |
This is a bug. I will open a PR to address this. |
|
have spent quite a bit of time optimizing these plugins and found that reducing functions calls definitely helps. So having a readonly access to the members will help.
I am there with you too specially that lsp plugin 😄
as my limit experience, readonly normally was not part of general OOP, but you may make `const` and `final` work on class var.
with the here `public` default and `const` or `final` then concern solved.
// anyway I supposed we were choosing the simplest way (vs that most complex) which I indicated two ways in above comment.
…--
shane.xb.qian
|
|
okay, so I guess we should then leave it as it? Seems like that is prefered from the community point of view instead... |
|
so I guess we should then leave it as it?
1. merger this pr
2. make `const` and `final` work on class var.
// then concern solved.
// P.S: double or more leading underscore may should be an err.
…--
shane.xb.qian
|
@yegappan If we get this PR, #12932, settled along with what semantics for access will be supported, and a list of outstanding access control features that are problems and need fixing. Then I can assist; I've got some work done, but stopped when it became unclear what was needed. Not sure the best way to enumerate the issues and at indicate who is working on what. |
@Shane-XB-Qian You might have a misunderstanding of the read-only semantics under discussion. In the vim9class docs it says The traditional |
@chrisbra Can you give a timeline on when this will be finalized. I'm unable/unwilling to work on issues until it's clear what needs to be done. |
And specifically, I've got a fix that handles ACCESS_PRIVATE/ACCESS_READ in |
|
it's not clear to me what you want. But to be clear, there is no rush, if you want something fixed, then just open an issue/PR and mention you are working on this. I am not in a hurry to release anything. We can tag this with the vim 9.1 Milestone and only when this is done get the release done. For the Vim9 oop semantics, I trust you and Yegappan to let me know once you thing this is in a state that is ready to be released :) And of course anybody else |
The first thing is a decision on what access semantics are going to be supported in vim9script for the 9.1 release. That is still an open issue AFAICT. The other problem is that access control was mostly not implemented at all as of last week. There's not been an issue opened. @yegappan and others have been addressing them as they come up; they have mostly been mentioned in unrelated PRs. There are/have-been to many specific cases to open an issue for each one (at least that's how it has been); but then again, I'm only saying what I've seen, accuracy might be off. Maybe a discussion, with the access matrix, what's been tested, what works, ..., If someone wants to address a specific case, they could say so. |
|
On 23/08/30 10:34AM, errael wrote:
> so I guess we should then leave it as it?
> 1. merger this pr 2. make `const` and `final` work on class var. // then concern solved. // P.S: double or more leading underscore may should be an err.
> […](#)
> -- shane.xb.qian
@Shane-XB-Qian You might have a misunderstanding of the read-only semantics under discussion. In the vim9class docs it says
```
anybody can read, only class can write
```
The traditional `const`/`final` do not meet this criteria.
maybe let's focus on your concern : 'safety' and 'getter()'
i donot understand that doc (your pasted) about 'read-only' what's that going to be (any OOP like that?)
or anyway, this part bram should be actually not impl yet, meant doc was just a rough design (in vim9 doc somewhere had said that too),
and `const` even different in legacy vim script and vim9, so supposed ok if a bit different in vim9 class var too (if really required).
…--
shane.xb.qian
|
Please feel free to open a PR with this fix and a test. |
Until there is a decision on access control syntax/semantics that's not going to happen (at least by me). So as long as you're not in a hurry... I'll do it when there's some decisions, otherwise it's potentially throw away code. How should co-ordination, specifically about access control, be handled? It's currently a bit of a hodge-podge. |
|
i am ok with current had made decision which is:
only `static` keyword, default is `public` and remove that keyword, and one leading underscore is private.
only left thing is your concern about 'readonly', if above explaination or suggestion cannot solve it, then i have no idea...
// then same like chrisbra, i left this to you guys to find a better or workable one.. 😄
…--
shane.xb.qian
|
Feel free to open as discussion. I am not in the position to test this currently. |
@yegappan You're the vim9script lead in this benign dictatorship. Whatever, if anything, we do must have your blessing or it's not worth doing. @chrisbra Is there a timeline for when a decision will be made? Nothing to do or work on if there's no target. |
|
Just in case there is a doubt. I would favor closing this PR without merging as described by @yegappan in the comment below extracted from #12932 (comment) To anyone who wishes to make clear their preference, keeping in mind that this is not a vote. Please be brief. There has been plenty of discussion.
|
|
keeping in mind that this is not a vote
this is a some kinds of 'vote' and discussion, otherwise where was this PR came from?
anyway, seems @yeggapan changed mind again which this was discarded vs #12978 opened.
// 'public' & 'readonly' & '_' looks odd, but if that's final version, then let it be~
…--
shane.xb.qian
|
I have started a discussion (#12979) to capture the Accessing a class member variable from a def function currently doesn't work. |
Thank you. I'll take a look. I would have done something if you'd said "go ahead" . |
|
On Thu, Aug 31, 2023 at 6:50 AM errael ***@***.***> wrote:
I have started a discussion (#12979
<#12979>) to capture the
access matrix.
Thank you. I'll take a look. I would have done something if you'd said "go
ahead" .
I have marked the failing cases with "BUG". Most of these are related to
writing to
a class member variable in a def function.
|
To be consistent and simple, make the default access for class/object member variables public.
If a member variable name starts with an underscore, then it is private.