Skip to content
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

Make object/class member variables public by default #12932

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 12 additions & 36 deletions runtime/doc/vim9class.txt
Original file line number Diff line number Diff line change
Expand Up @@ -114,31 +114,10 @@ naming convention makes the object members easy to spot. Also, when a
variable does not have the "this." prefix you know it is not an object member.


Member write access ~
Member accessibility ~

Now try to change an object member directly: >
By default object members are public and can be read and modified directly.

pos.lnum = 9
< *E1335*
This will give you an error! That is because by default object members can be
read but not set. That's why the TextPosition class provides a method for it: >

pos.SetLnum(9)

Allowing to read but not set an object member is the most common and safest
way. Most often there is no problem using a value, while setting a value may
have side effects that need to be taken care of. In this case, the SetLnum()
method could check if the line number is valid and either give an error or use
the closest valid value.
*:public* *E1331*
If you don't care about side effects and want to allow the object member to be
changed at any time, you can make it public: >

public this.lnum: number
public this.col: number

Now you don't need the SetLnum(), SetCol() and SetPosition() methods, setting
"pos.lnum" directly above will no longer give an error.
*E1334*
If you try to set an object member that doesn't exist you get an error: >
pos.other = 9
Expand Down Expand Up @@ -204,12 +183,12 @@ including "this.", as the argument to new() means the value provided in the
new() call is assigned to that object member. This mechanism comes from the
Dart language.

Putting together this way of using new() and making the members public results
in a much shorter class definition than what we started with: >
Putting together this way of using new() results in a much shorter class
definition than what we started with: >

class TextPosition
public this.lnum: number
public this.col: number
this.lnum: number
this.col: number

def new(this.lnum, this.col)
enddef
Expand Down Expand Up @@ -255,14 +234,13 @@ prefix: >
Since the name is used as-is, shadowing the name by a function argument name
or local variable name is not allowed.

Just like object members the access can be made private by using an underscore
as the first character in the name, and it can be made public by prefixing
"public": >
Just like object members the class members are public by default and the
access can be made private by using an underscore as the first character in
the name: >

class OtherThing
static total: number # anybody can read, only class can write
static total: number # anybody can read and write
static _sum: number # only class can read and write
public static result: number # anybody can read and write
endclass
<
*class-function*
Expand Down Expand Up @@ -482,7 +460,6 @@ Inside a class, in between `:class` and `:endclass`, these items can appear:
- An object member declaration: >
this._memberName: memberType
this.memberName: memberType
public this.memberName: memberType
- A constructor method: >
def new(arguments)
def newName(arguments)
Expand Down Expand Up @@ -864,9 +841,8 @@ you will have to track down and change. You may have to make it a "set"
method call. This reflects the real world problem that taking away access
requires work to be done for all places where that access exists.

An alternative would have been using the "private" keyword, just like "public"
changes the access in the other direction. Well, that's just to reduce the
number of keywords.
An alternative would have been using the "private" keyword. Well, that's just
to reduce the number of keywords.


No protected object members ~
Expand Down
2 changes: 0 additions & 2 deletions src/errors.h
Original file line number Diff line number Diff line change
Expand Up @@ -3405,8 +3405,6 @@ EXTERN char e_cannot_get_object_member_type_from_initializer_str[]
INIT(= N_("E1329: Cannot get object member type from initializer: %s"));
EXTERN char e_invalid_type_for_object_member_str[]
INIT(= N_("E1330: Invalid type for object member: %s"));
EXTERN char e_public_must_be_followed_by_this_or_static[]
INIT(= N_("E1331: Public must be followed by \"this\" or \"static\""));
EXTERN char e_public_member_name_cannot_start_with_underscore_str[]
INIT(= N_("E1332: Public member name cannot start with underscore: %s"));
EXTERN char e_cannot_access_private_member_str[]
Expand Down
20 changes: 10 additions & 10 deletions src/eval.c
Original file line number Diff line number Diff line change
Expand Up @@ -1573,19 +1573,19 @@ get_lval(
switch (om->ocm_access)
{
case VIM_ACCESS_PRIVATE:
semsg(_(e_cannot_access_private_member_str),
semsg(_(e_cannot_access_private_member_str),
om->ocm_name);
return NULL;
return NULL;
case VIM_ACCESS_READ:
if ((flags & GLV_READ_ONLY) == 0)
{
semsg(_(e_member_is_not_writable_str),
om->ocm_name);
return NULL;
}
break;
if ((flags & GLV_READ_ONLY) == 0)
{
semsg(_(e_member_is_not_writable_str),
om->ocm_name);
return NULL;
}
break;
case VIM_ACCESS_ALL:
break;
break;
}

lp->ll_valtype = om->ocm_type;
Expand Down
4 changes: 2 additions & 2 deletions src/structs.h
Original file line number Diff line number Diff line change
Expand Up @@ -1489,8 +1489,8 @@ typedef struct {
#define TTFLAG_SUPER 0x40 // object from "super".

typedef enum {
VIM_ACCESS_PRIVATE, // read/write only inside th class
VIM_ACCESS_READ, // read everywhere, write only inside th class
VIM_ACCESS_PRIVATE, // read/write only inside the class
VIM_ACCESS_READ, // read everywhere, write only inside the class
VIM_ACCESS_ALL // read/write everywhere
} omacc_T;

Expand Down
41 changes: 10 additions & 31 deletions src/testdir/test_vim9_class.vim
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,6 @@ def Test_class_object_member_access()
class Triple
this._one = 1
this.two = 2
public this.three = 3

def GetOne(): number
return this._one
Expand All @@ -729,27 +728,16 @@ def Test_class_object_member_access()
var trip = Triple.new()
assert_equal(1, trip.GetOne())
assert_equal(2, trip.two)
assert_equal(3, trip.three)
assert_fails('echo trip._one', 'E1333')

assert_fails('trip._one = 11', 'E1333')
assert_fails('trip.two = 22', 'E1335')
trip.three = 33
assert_equal(33, trip.three)
trip.two = 22
assert_equal(22, trip.two)

assert_fails('trip.four = 4', 'E1334')
END
v9.CheckScriptSuccess(lines)

# Test for a public member variable name beginning with an underscore
lines =<< trim END
vim9script
class A
public this._val = 10
endclass
END
v9.CheckScriptFailure(lines, 'E1332:')

lines =<< trim END
vim9script

Expand Down Expand Up @@ -828,23 +816,14 @@ def Test_class_object_member_access()
END
v9.CheckScriptSuccess(lines)

# Test for "public" cannot be abbreviated
lines =<< trim END
vim9script
class Something
pub this.val = 1
endclass
END
v9.CheckScriptFailure(lines, 'E1065:')

# Test for "public" keyword must be followed by "this" or "static".
# Test for using the "public" keyword
lines =<< trim END
vim9script
class Something
public val = 1
public this.val = 1
endclass
END
v9.CheckScriptFailure(lines, 'E1331:')
v9.CheckScriptFailure(lines, 'E1318:')

# Test for "static" cannot be abbreviated
lines =<< trim END
Expand Down Expand Up @@ -983,7 +962,7 @@ def Test_class_member()
this.col = 1
static counter = 0
static _secret = 7
public static anybody = 42
static anybody = 42

static def AddToCounter(nr: number)
counter += nr
Expand All @@ -1001,8 +980,8 @@ def Test_class_member()
assert_equal(3, GetCounter())

assert_fails('TextPos.noSuchMember = 2', 'E1337:')
assert_fails('TextPos.counter = 5', 'E1335:')
assert_fails('TextPos.counter += 5', 'E1335:')
TextPos.counter = 5
assert_equal(5, TextPos.counter)

assert_fails('echo TextPos._secret', 'E1333:')
assert_fails('TextPos._secret = 8', 'E1333:')
Expand Down Expand Up @@ -1522,14 +1501,14 @@ def Test_class_implements_interface()
vim9script

interface Result
public this.label: string
this.label: string
this.errpos: number
endinterface

# order of members is opposite of interface
class Failure implements Result
this.errpos: number = 42
public this.label: string = 'label'
this.label: string = 'label'
endclass

def Test()
Expand Down
45 changes: 7 additions & 38 deletions src/vim9class.c
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,13 @@
static int
parse_member(
exarg_T *eap,
char_u *line,
char_u *varname,
int has_public, // TRUE if "public" seen before "varname"
char_u **varname_end,
garray_T *type_list,
type_T **type_ret,
char_u **init_expr)
{
*varname_end = to_name_end(varname, FALSE);
if (*varname == '_' && has_public)
{
semsg(_(e_public_member_name_cannot_start_with_underscore_str), line);
return FAIL;
}

char_u *colon = skipwhite(*varname_end);
char_u *type_arg = colon;
Expand Down Expand Up @@ -122,16 +115,14 @@ add_member(
garray_T *gap,
char_u *varname,
char_u *varname_end,
int has_public,
type_T *type,
char_u *init_expr)
{
if (ga_grow(gap, 1) == FAIL)
return FAIL;
ocmember_T *m = ((ocmember_T *)gap->ga_data) + gap->ga_len;
m->ocm_name = vim_strnsave(varname, varname_end - varname);
m->ocm_access = has_public ? VIM_ACCESS_ALL
: *varname == '_' ? VIM_ACCESS_PRIVATE : VIM_ACCESS_READ;
m->ocm_access = *varname == '_' ? VIM_ACCESS_PRIVATE : VIM_ACCESS_ALL;
m->ocm_type = type;
if (init_expr != NULL)
m->ocm_init = init_expr;
Expand Down Expand Up @@ -1031,24 +1022,6 @@ ex_class(exarg_T *eap)
break;
}

int has_public = FALSE;
if (checkforcmd(&p, "public", 3))
{
if (STRNCMP(line, "public", 6) != 0)
{
semsg(_(e_command_cannot_be_shortened_str), line);
break;
}
has_public = TRUE;
p = skipwhite(line + 6);

if (STRNCMP(p, "this", 4) != 0 && STRNCMP(p, "static", 6) != 0)
{
emsg(_(e_public_must_be_followed_by_this_or_static));
break;
}
}

int has_static = FALSE;
char_u *ps = p;
if (checkforcmd(&p, "static", 4))
Expand All @@ -1065,7 +1038,6 @@ ex_class(exarg_T *eap)
// object members (public, read access, private):
// "this._varname"
// "this.varname"
// "public this.varname"
if (STRNCMP(p, "this", 4) == 0)
{
if (p[4] != '.' || !eval_isnamec1(p[5]))
Expand All @@ -1077,12 +1049,11 @@ ex_class(exarg_T *eap)
char_u *varname_end = NULL;
type_T *type = NULL;
char_u *init_expr = NULL;
if (parse_member(eap, line, varname, has_public,
&varname_end, &type_list, &type,
is_class ? &init_expr: NULL) == FAIL)
if (parse_member(eap, varname, &varname_end, &type_list, &type,
is_class ? &init_expr: NULL) == FAIL)
break;
if (add_member(&objmembers, varname, varname_end,
has_public, type, init_expr) == FAIL)
type, init_expr) == FAIL)
{
vim_free(init_expr);
break;
Expand Down Expand Up @@ -1174,17 +1145,15 @@ ex_class(exarg_T *eap)
// class members (public, read access, private):
// "static _varname"
// "static varname"
// "public static varname"
char_u *varname = p;
char_u *varname_end = NULL;
type_T *type = NULL;
char_u *init_expr = NULL;
if (parse_member(eap, line, varname, has_public,
&varname_end, &type_list, &type,
is_class ? &init_expr : NULL) == FAIL)
if (parse_member(eap, varname, &varname_end, &type_list, &type,
is_class ? &init_expr : NULL) == FAIL)
break;
if (add_member(&classmembers, varname, varname_end,
has_public, type, init_expr) == FAIL)
type, init_expr) == FAIL)
{
vim_free(init_expr);
break;
Expand Down
Loading