[vim/vim] Make object/class member variables public by default (PR #12932)
Yegappan Lakshmanan
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.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/12932
Commit Summary
- a255d33 Make object/class member variables public by default
File Changes
(6 files)
- M runtime/doc/vim9class.txt (48)
- M src/errors.h (4)
- M src/eval.c (16)
- M src/structs.h (3)
- M src/testdir/test_vim9_class.vim (41)
- M src/vim9class.c (49)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
codecov[bot]
Codecov Report
Merging #12932 (a255d33) into master (d809c0a) will decrease coverage by
5.46%.
The diff coverage is100.00%.
@@ Coverage Diff @@ ## master #12932 +/- ## ========================================== - Coverage 82.05% 76.59% -5.46% ========================================== Files 160 150 -10 Lines 194679 151995 -42684 Branches 43696 39171 -4525 ========================================== - Hits 159743 116421 -43322 - Misses 22073 23549 +1476 + Partials 12863 12025 -838
| Flag | Coverage Δ | |
|---|---|---|
| huge-clang-none | ? |
|
| linux | ? |
|
| mingw-x64-HUGE | 76.59% <100.00%> (-0.01%) |
⬇️ |
| mingw-x86-HUGE | ? |
|
| windows | 76.59% <100.00%> (-1.60%) |
⬇️ |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Files Changed | Coverage Δ | |
|---|---|---|
| src/eval.c | 91.80% <100.00%> (-0.65%) |
⬇️ |
| src/vim9class.c | 85.75% <100.00%> (-3.64%) |
⬇️ |
... and 154 files with indirect coverage changes
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
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 vim9script is performance.
=== assign micro-prof ===
50 100 200 : nLoops (usec/op)
0.044 0.044 0.043 : var local = n_value ###-A
0.036 0.035 0.034 : var local = C.static_var ###-D
0.050 0.047 0.046 : var local = a_class.this_var ###-G
0.173 0.170 0.164 : var local = a_class.Getter() ###-J
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
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 ofvim9scriptis performance.
If we want to keep the object method and member accessibility consistent,
then we need this PR. Otherwise we can keep the current behavior.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
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
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
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.
or the most complex one: not only change control but also access control, adding 'readonly', and all control with keyword.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
If we want to keep the object method and member accessibility consistent,
then we need this PR.
I don't understand that. Currently there is _ for private methods/members; public for for public. Nicely consistent. The issue revolves around what does it mean if there is no access modifier?
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
- Require an access modifier, currently
_andpublic
There is no default, and other access schemes, like introducing areadonlymodifier for member, can be implemented in a source compatible way. - Default is public, get rid of the
publicaccess modifier, it's not needed. If simplicity is the goal, it's clutter.
Members/methods are public by default. Can introduce areadonlyaccess modifier for current capability. - Keep the current.
Is it too complex/confusing that a method is not read only but by default a member is? members and methods are different; most languages don't have a concept of writing to a method.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Jesse Pavel
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.
errael's second option (default public, _ private, readonly keyword) feels like the cleanest way to retain all current capabilities, though I personally wouldn't mourn the loss of readonly either, given that the semantics of vim9script's read-only members is unique as well.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
If we want to keep the object method and member accessibility consistent,
then we need this PR.I don't understand that. Currently there is
_for private methods/members;
publicfor for public. Nicely consistent.
The public keyword is used only for object/class members and not for methods.
The methods are public by default and private if the name starts with an underscore.
The issue revolves around what does it mean if there is no access modifier?
Yes.
I thought read only member by default was elegant; it's simple to explain and
offers both safety and performance.
With the latest code, you will get this behavior (without this PR). If this is the
desired behavior, then we don't need to make any further changes.
Here's some ways to look at it
- Require an access modifier, currently
_andpublic
There is no default, and other access schemes, like introducing a
readonlymodifier for member, can be implemented in a source compatible
way.- Default is public, get rid of the
publicaccess modifier, it's not
needed. If simplicity is the goal, it's clutter. Members/methods are
public by default. Can introduce areadonlyaccess modifier for current
capability.
This PR introduces the behavior in 2 except for the readonly keyword.
- Keep the current.
Is it too complex/confusing that a method is not read only but by default
a member is? members and methods are different; most languages don't have
a concept of writing to a method.
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
methods and members), then we don't need any additional changes.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
- Default is public, get rid of the
publicaccess modifier, it's not
needed. If simplicity is the goal, it's clutter. Members/methods are
public by default. Can introduce areadonlyaccess modifier for current
capability.This PR introduces the behavior in 2 except for the
readonlykeyword.
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 readonly keyword, before asking how adding that would be received. If this PR left the infrastructure for readonly, it would be a lot simpler to provide that capability.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
@yegappan pushed 1 commit.
- beac7f2 Retain some parts of the read only member access code
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gianmaria Bajo
+1 for readonly and making members public by default (removing public altogether).
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 (extends, implements, etc.).
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
Retain some parts of the read only member access code
Thanks. Assuming you're not planning to do read only, I'll take a look tomorrow. (What other plans?).
Idea is basically put back public using readonly; has_readonly instead of has_public. Tweak as needed and flavor with tests.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
Oh, and readonly is the only one I've heard floated; haven't thought of anything else that's perfect. I could live with read or ronly. Once everything else is in place, it's simple to change the word.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
dkearns
readable...paint it green!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
When I build objmember, with no modifications, and run this I see two problems
- Able to access private static
- Compile error when trying to write public static
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
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
Oh, and this is what I'm running
hg glog -r ::objmember -l 2
@ changeset: 18285:bdb16ee51e4c
| bookmark: objmember
| tag: default/objmember
| tag: tip
| parent: 18253:025bc462c74c
| user: Yegappan Lakshmanan <yega...@yahoo.com>
| date: Sun Aug 27 07:59:56 2023 -0700
| summary: Make object/class member variables public by default
|
o changeset: 18253:025bc462c74c
| user: zeertzjq <zeer...@outlook.com>
~ date: Sun Aug 27 11:17:39 2023 +0200
summary: patch 9.0.1792: problem with gj/gk/gM and virtual text
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
Has readonly ever worked? I just ran some tests with vim I built yesterday, standard vim9 stuff. Happily writes to read only variable.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
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 (@yegappan 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);
}
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
After the shock of finding out that readonly has probably never worked, the VIM_ACCESS_PRIVATE is certainly simpler than readonly. I wonder if checking where the various STORE instructions generation is called would do the trick? Or maybe compile_assignment. Is there any other way to change a variable? I wonder how [ x, y ] = [ 1, 2] is handled. generate_loadvar looks interesting.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
errael
I meant generate_store_lhs
But wait, this is only for classmembers so: ISN_STORE_CLASSMEMBER might be all it takes.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
When I build objmember, with no modifications, and run this I see two problems
- Able to access private static
- Compile error when trying to write public static
vim-dev ML
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
>
> 2. Compile error when trying to write public static
>
>
I am able to reproduce the second issue even using a two months old image..
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
vim-dev ML
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)
> {
> + // 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
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
errael
Can you open a PR for this with a test for this condition?
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 compile_assign_unlet I believe that's the spot for the readonly check. The lhs_T looks like is has all the info needed for the check VIM_ACCESS_READ before any instructions are generated (3 for storing into the object when object is in local var).
A similar check is used before this block of code for
readonly object members
Assuming you meant private.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
upd & link & refer : #11827
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
readonly is working AFAICT. I still need to put together some tests. I can't try writing to a readonly static because of the compilation failure bug that prevents writing to class statics. One nice thing about readonly is that it doesn't affect anything unless you use it.
I'm going to put this on the shelf while I put together the PR for preventing writing to private members.
The readonly PR is based on this PR. So this PR, #12932, has to go in before I can open a PR for readonly. Is this PR ready to go?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
Is there some strong objection to requiring a keyword modifier? Why is a default needed at all?
We could require private and keep the required underscore as well. This is is already done for functions/methods which require an uppercase character.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Is there some strong objection to requiring a keyword modifier? Why is a default needed at all?
Are you suggesting private/public for functions, members, static.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
@errael before asking for this PR to be merged, can you show your code with the readonly implementation? Because I was also working on an implementation but it's incompatible with this PR. So I'd like to discuss how we should implement readonly first.
My take is here (WIP, not working).
Basically I wanted readonly to work as in C#:
- both private and public members can be readonly
- readonly means the member can still be changed (set) by the constructor, but not later
- readonly is only for object members (
this.), not for class members (static) because the constructor can change it
So it's different from const, that could be actually implemented to have readonly class (static) members.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
The implementation I'm working is the current read only semantics that you get by default and is described in the current documentation; see ":he vim9-class-member", it says "anybody can read, only class can write", no other rules. Note that even this has been considered by some as too complex. What you describe seems to go beyond the goal of simplicity.
The change I'm working on is about access control, and does not affect object constructin. It affects both members/statics.
There's a comment from Bram somewhere that a problem with using the readonly keyword is that it meant something else in another language (I'm guessing C#). Maybe readable would be a better keyword to avoid possible confusion. Fortunately changing the keyword is straightforward.
It seems like what you are working on is orthogonal to what I'm doing. It is a different mechanism entirely, which could be added at some point.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
The problem with the semantics Bram came up with, is that they're different from any other language (the use of public for example). The semantics for readonly you are proposing follow this line of thought. readonly should mean the member can't be written by anybody, otherwise it's not readonly. Just like public should mean publicly accessible, not publicly writable.
So I'm against your proposal. I think it doesn't matter what the current specification says, because mostly it doesn't work (do abstract classes and interfaces actually work?). The class specification should be considered a WIP all around so.
Rather than confusing simplicity, familiarity and consistence of semantics (on the base of what they already mean in other languages) is more important here. Then you say it's more complex but I don't even think it is.
Let's hear what @yegappan says.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
The same concept of anybody can read, only class can write, I don't think it exists in any other language. So it's not simple.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
I think it doesn't matter what the current specification says, because mostly it doesn't work
I have written a project using the current readonly semantics and I like them quite a bit. In particular I have a presentation class that presents a readonly view of some internal structures. This is easy to implement allowing concise code and doesn't require the much lower performance getters.
readonly should mean ...
OK, I guess the access modifier readable is the way to go.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
Also readable doesn't exist anywhere else. If simplicity is the ultimate goal, I'd say this PR is ok in just removing public. We can live without readonly but even more so without stuff invented for vimscript that doesn't exist anywhere else.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
so let me summarize: the current proposal is to remove the public keyword completely (?) and make all attributes public by default (unless they start with _)? So by removing this keyword, no-one complains that private keyword is also missing?
I like that approach, it makes it easier to spot private members directly in the code. But I am probably not the one going to use this a lot, so I am not having any saying here :)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
so let me summarize: the current proposal is to remove the
publickeyword completely (?) and make all attributes public by default (unless they start with_)? So by removing this keyword, no-one complains thatprivatekeyword is also missing?
vim-dev ML
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
readonlyis working AFAICT. I still need to put together some tests. I can't try writing to a readonly static because of the compilation failure bug that prevents writing to class statics. One nice thing aboutreadonlyis that it doesn't affect anything unless you use it.
I'm going to put this on the shelf while I put together the PR for preventing writing to private members.
The
readonlyPR is based on this PR. So this PR, #12932, has to go in before I can open a PR forreadonly. Is this PR ready to go?
vim-dev ML
On Tue, Aug 29, 2023 at 9:08 AM errael ***@***.***> wrote:
> readonly is working AFAICT. I still need to put together some tests. I
> can't try writing to a readonly static because of the compilation failure
> bug that prevents writing to class statics. One nice thing about readonly
> is that it doesn't affect anything unless you use it.
>
One of the goals of the Vim9 script is to be similar to other widely
scripting languages (like
typescript and python) and simple to use. I am not sure whether any other
OO language
supports the "readonly" keyword. Can we keep it simple and make the
methods/members
either private or public?
> I'm going to put this on the shelf while I put together the PR for
> preventing writing to private members.
>
> The readonly PR is based on this PR. So this PR, #12932
>
>
> This PR can be merged based on the outcome of these discussions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
>
> Is there some strong objection to requiring a keyword modifier? Why is a default needed at all?
>
in a member/method
declaration, then it will be too cumbersome to add this prefix to each
member/method.
A default makes it simpler.
Regards,
Yegappan
vim-dev ML
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Also
readabledoesn't exist anywhere else. If simplicity is the ultimate goal, I'd say this PR is ok in just removingpublic. We can live withoutreadonlybut even more so without stuff invented for vimscript that doesn't exist anywhere else.
vim-dev ML
On Tue, Aug 29, 2023 at 11:03 AM Gianmaria Bajo ***@***.***>
> Also readable doesn't exist anywhere else. If simplicity is the ultimate
> goal, I'd say this PR is ok in just removing public. We can live without
> readonly but even more so without stuff invented for vimscript that
> doesn't exist anywhere else.
>
>
> I agree. I think this will be simple and less confusing.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Gianmaria Bajo
Just to mention another alternative:
this.member # full access from everywhere
this._member # semi-private: read-only from outside the class
this.__member # fully private
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
I agree. I think this will be simple and less confusing.
Unlike the previous/current default, one would have to specify readable to get that behavior; it is a conscious effort to take on the burden of understanding it, otherwise it sits there quietly and doesn't intrude. If you look at code that must have Getters() versus direct access, I doesn't look simpler. And the idea that something new or innovative can't be simple is sad.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Able to access private static
Compile error when trying to write public static
There's another one (a real shocker).
It's OK to write to a private member.
vim9script
class C
this._m: number
#static _s: number
def Dump()
echo this._m
enddef
endclass
def F()
var o = C.new()
o.Dump()
o._m = 5
o.Dump()
#var s = C._s
enddef
F()
I put in the Dump() because I had to see it to believe it.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
Should the example here https://github.com/vim/vim/blob/d01a0df61d2a322645fb393ed704fcec38e0d0fe/runtime/doc/vim9class.txt#L186-L200
also be changed to use underscores? Otherwise we wouldn't need the and Set() function, right?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
There's another one (a real shocker).
It's OK to write to a private member.
Confirmed, but only inside a def, it works at script level.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Can you open a PR for this with a test for this condition?
I got everything wrapped up as a follow up to this PR, and it doesn't look like that will work out. So no.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
Should the example here https://github.com/vim/vim/blob/d01a0df61d2a322645fb393ed704fcec38e0d0fe/runtime/doc/vim9class.txt#L186-L200
also be changed to use underscores? Otherwise we wouldn't need the and Set() function, right?
vim-dev ML
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Yegappan Lakshmanan
Able to access private static
Compile error when trying to write public static
There's another one (a real shocker).
It's OK to write to a private member.
vim9script class C this._m: number #static _s: number def Dump() echo this._m enddef endclass def F() var o = C.new() o.Dump() o._m = 5 o.Dump() #var s = C._s enddef F()I put in the
Dump()because I had to see it to believe it.
vim-dev ML
On Tue, Aug 29, 2023 at 1:38 PM errael ***@***.***> wrote:
>
> -
> Able to access private static
> Compile error when trying to write public static
>
> There's another one (a real shocker).
>
> It's OK to write to a private member.
>
> vim9script
>
> class C
> this._m: number
> #static _s: number
> def Dump()
> echo this._m
> enddef
> endclass
>
> def F()
> var o = C.new()
> o.Dump()
> o._m = 5
> o.Dump()
> #var s = C._s
> enddef
> F()
>
> I put in the Dump() because I had to see it to believe it.
>
>
>
I have created the PR https://github.com/vim/vim/pull/12963 to address this.
Regards,
Yegappan
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Shane-XB-Qian
Just to mention another alternative:
this.member # full access from everywhere this._member # semi-private: read-only from outside the class this.__member # fully private
vim9 is for scripting mostly, i think better be making it simple.
// @yegappan double (or more) leading __ perhaps should be a err.
Yes. The only supported keyword for members/methods is "static" for declaring class members and methods. If a member or method name starts with an underscore, then it is private. Otherwise it is public.
ok, i think we can live with it (at least me), and actually this is the better one, leading underscore is acceptable if so.
// though i think should be not much people really would use vim9 to write OOP script.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
Is there some strong objection to requiring a keyword modifier? Why is a default needed at all?
Are you suggesting
private/publicfor functions, members, static.
Yes, all class features where the modifier is applicable. I was, however, assuming there'd be more than the two; there will be one day. :)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
dkearns
so let me summarize: the current proposal is to remove the
publickeyword completely (?) and make all attributes public by default (unless they start with_)? So by removing this keyword, no-one complains thatprivatekeyword is also missing?
I like that approach, it makes it easier to spot private members directly in the code. But I am probably not the one going to use this a lot, so I am not having any saying here :)
For the record, I have been complaining for a year about some of the quirky class definition syntax.
Limiting this to private underscore for the moment and since you have no dog in the fight, maybe you could take the hit and elaborate on why "spot private members in the code" is of such obvious value that we need a new identifier naming rule? My blind spot is huge.
It seems to me to be a completely arbitrary partial Hungarian notation with semantics. There are several attributes I'm interested in at the call site, accessibility is only one of them and not the most interesting.
In my experience the underscore idiom was developed in languages that didn't enforce accessibility and doesn't make much sense in those that do. I'm assuming this feels natural to people who already use the idiom in other languages. I find it extremely ugly in a language that I otherwise find consistent and quite pretty in its Vim 9 release state.
When you lump this in with required 'this' qualification one has to prefix every private field with six characters of extremely dubious value. A few variables on the same line and I'm already cobbling together a syn-conceal group to ease the pain.
This is largely bike-shedding, of course, but isn't that what surface level language design is all about? :)
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
why "spot private members in the code" is of such obvious value
I agree with most everything you've said (and although I've used (and like quite a bit) python, I've never used it in a larger project where _ was used.
But now that my beloved readable is in the trash, I will rarely have an unadorned field name; everything is private. I'd find it more useful to visually flag something public so the warning bells go off in my head.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
For the record, I am not insisting on any of this and the reason I have not yet merged is, I'd like to see what the dicussion results in :)
But since you seem to be asking me directly 😎
Code tends to be read more often then written and for me, being able to see while reading existing code, ah this must be a private member outweights the disadvantage that the _ brings as part of the changes in semantics of the language. (I think Bram brought this argument as well at some time). Yes you need to know that _foobar has a special meaning, but if you are writing code, you should know where to lookup the specifics of your language anyhow.
there is another disadvantage, that one cannot easily refactor code, but I suppose this is not done so often. So for me this is okay.
and finally whether one uses this._member or this.member doesn't make such a huge difference for me.
But then I am not a developer in my profession and I don't know what other languages tend to do, so my opinion is as good or bad as anybody elses here :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
errael
I'd like to see what the dicussion results in :)
In that case
I'm disappointed that there was no response to the observations/concerns with the performance hit of not having direct read access. Of the possible responses, e.g. "not that important", no response is the worst.
In the discussion on simplicity, there wasn't much discussion of the visual clutter introduced by requiring getters. Recall that the vim9class doc carefully shows how simple a class can look for common cases; vim9script is a scripting language. Safety was traded off in favor of not having public and adding visual clutter. And one of the "key" discussion points was that anything new or innovative could not be simple, I don't accept that.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
I'm disappointed that there was no response to the observations/concerns with the performance hit of not having direct read access. Of the possible responses, e.g. "not that important", no response is the worst.
I don't follow. By default we have public access, so that shouldn't have any negative performance impact.
In the discussion on simplicity, there wasn't much discussion of the visual clutter introduced by requiring getters. Recall that the vim9class doc carefully shows how simple a class can look for common cases; vim9script is a scripting language. Safety was traded off in favor of not having public and adding visual clutter. And one of the "key" discussion points was that anything new or innovative could not be simple, I don't accept that.
You need Getters only for private members, right? For common cases using public members you wouldn't need it, right?
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
I'm disappointed that there was no response to the observations/concerns with the performance hit of not having direct read access. Of the possible responses, e.g. "not that important", no response is the worst.
I don't follow. By default we have public access, so that shouldn't have any negative performance impact.
I don't use public, it is unsafe. The performance hit is that Getters are required.
#12932 (comment)
In the discussion on simplicity, there wasn't much discussion of the visual clutter introduced by requiring getters. Recall that the vim9class doc carefully shows how simple a class can look for common cases; vim9script is a scripting language. Safety was traded off in favor of not having public and adding visual clutter. And one of the "key" discussion points was that anything new or innovative could not be simple, I don't accept that.
You need Getters only for private members, right? For common cases using public members you wouldn't need it, right?
Public members are OK for tiny things, but public members are not safe. I think you are mistaken; for a stongly typed OOP, public members are not the common case. As a project grows, allowing arbitrary actors to modify internal state is not a common design pattern. Make everything private and add Getter() is what one needs to do with the proposed semantics.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
What @errael wants, is members that can be publicly read but not written, so that you don't need to set up Getters for them (with the associated performance hit). I think it's a valid case after all.
Other OOP languages have ways to define this behaviour (for example with properties) but we only want simple stuff here, certainly not properties, but it would be convenient to have here. I still think readable is a bad name, as is readonly for this. Maybe restrict or similar.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
I supposed this can reduce your safety concern, though I still think not much people really would use vim9 to wrote OOP script.
--
shane.xb.qian
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Found it, here's some comments from Bram about this topic
In the email thread
https://groups.google.com/g/vim_dev/c/yYpFNUHdRho/m/xjgrKqMoBQAJ?pli=1
The only comment I saw from Bram in the thread specifically about default public versus read-only has to do with simple versus safe. From the thread, in response to a suggestion that public should be the default:
About public/private/read-only: Making the default public is certainly
possible. I think most languages have it that way, even though it's not
a safe choice.
I do like the read-only access. Quite often you want to disallow any
code outside of the class to change a member, so that you can make sure
the value is valid. But making it private means you have add a
"getter" and call it. [...]
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
I'm disappointed that there was no response to the observations/concerns with
the performance hit of not having direct read access. Of the possible
responses, e.g. "not that important", no response is the worst.
Yes. Using getter functions to read member values will have an
performance impact.
In the discussion on simplicity, there wasn't much discussion of the visual
clutter introduced by requiring getters. Recall that the vim9class doc
carefully shows how simple a class can look for common cases; vim9script is
a scripting language. Safety was traded off in favor of not havingpublic
and adding visual clutter. And one of the "key" discussion points was that
anything new or innovative could not be simple, I don't accept that.
If we want to continue to support the existing readonly default access,
then I prefer using the existing "public" keyword to make members public
than using "readonly" or "readable" to make members readable (which is
non-standard). We then don't the changes in this PR and can keep the existing
behavior.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Other OOP languages have ways to define this behaviour...
I still think readable is a bad name, as is readonly for this. Maybe restrict or similar.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
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):
vim9script
class A
# object member: read-only access
this.val1 = 10
# object member: read/write access
public this.val2 = 20
# object member: private (no access)
this._val3 = 30
# class member: read-only access
static val4 = 40
# class member: read/write access
public static val5 = 50
# class member: private (no access)
static _val6 = 60
endclass
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
> But we are specifically talking about OOP. Someone using it should be assumed when considering a comment.
if so, not much getter() required.
--
shane.xb.qian
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Gianmaria Bajo
Currently public isn't even needed by the way. I learned of it with this PR.
vim9script
class C
this.str: string
endclass
def F()
var tvar = C.new()
echo tvar.str
tvar.str = "a"
echo tvar.str
enddef
F()
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
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?
I developed and maintain the following large and widely used Vim plugins:
https://github.com/yegappan/lsp
https://github.com/yegappan/taglist
https://github.com/yegappan/mru
Based on this experience, I can say that type safety and access control
are important. I 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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
Currently
publicisn't even needed by the way. I learned of it with this PR.
This is a bug. I will open a PR to address this.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
readonly access to the members will help.
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
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
okay, so I guess we should then leave it as it? Seems like that is prefered from the community point of view instead...
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
> so I guess we should then leave it as it?
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
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Currently
publicisn't even needed by the way. I learned of it with this PR.This is a bug. I will open a PR to address this.
@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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
so I guess we should then leave it as it?
- merger this pr 2. make
constandfinalwork 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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
errael
okay, so I guess we should then leave it as it? Seems like that is prefered from the community point of view instead...
@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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Currently
publicisn't even needed by the way. I learned of it with this PR.This is a bug. I will open a PR to address this.
And specifically, I've got a fix that handles ACCESS_PRIVATE/ACCESS_READ in compile_assign_unlet. Which I think covers this particular issue.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
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
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
________________________________
From: errael ***@***.***>
Sent: Thursday, August 31, 2023 1:34:17 AM
To: vim/vim ***@***.***>
Cc: Shane-XB-Qian ***@***.***>; Mention ***@***.***>
Subject: Re: [vim/vim] Make object/class member variables public by default (PR #12932)
so I guess we should then leave it as it?
…
-- shane.xb.qian
@Shane-XB-Qian<https://github.com/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.
—
You are receiving this because you were mentioned.Message ID: ***@***.***>
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
errael
it's not clear to me what you want.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
> > 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
> ```
>
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
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.
Yegappan Lakshmanan
Currently
publicisn't even needed by the way. I learned of it with this PR.This is a bug. I will open a PR to address this.
And specifically, I've got a fix that handles ACCESS_PRIVATE/ACCESS_READ
incompile_assign_unlet. Which I think covers this particular issue.
Please feel free to open a PR with this fix and a test.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
Currently
publicisn't even needed by the way. I learned of it with this PR.This is a bug. I will open a PR to address this.
And specifically, I've got a fix that handles ACCESS_PRIVATE/ACCESS_READ
incompile_assign_unlet. Which I think covers this particular issue.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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
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.. :smile:
--
shane.xb.qian
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Christian Brabandt
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.
Feel free to open as discussion. I am not in the position to test this currently.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
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.
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.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
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.
to continue to support the existing readonly default access,
[...]
We then don't need the changes in this PR and can keep
the existing behavior.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
JohnBeckett
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):
vim9script class A # object member: read-only access this.val1 = 10 # object member: read/write access public this.val2 = 20 # object member: private (no access) this._val3 = 30 # class member: read-only access static val4 = 40 # class member: read/write access public static val5 = 50 # class member: private (no access) static _val6 = 60 endclass
Yegappan Lakshmanan
Closed #12932.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Shane-XB-Qian
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
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
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.
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.
I have started a discussion (#12979) to capture the
access matrix. I have tested the various access controls for an object method and the
different ways it can be accessed. All of these works as expected. Let me know if I missed
any cases.
Accessing a class member variable from a def function currently doesn't work.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
errael
I have started a discussion (#12979) to capture the
access matrix.
Thank you. I'll take a look. I would have done something if you'd said "go ahead" .
—
Reply to this email directly, view it on GitHub.
You are receiving this because you commented.
Yegappan Lakshmanan
I have started a discussion (#12979) to capture the
access matrix.Thank you. I'll take a look. I would have done something if you'd said "go ahead" .
vim-dev ML
> I have started a discussion (#12979
>
> 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.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.