patch 9.0.1945: Vim9: missing support for ro-vars in interfacev9.0.1945

Problem:  Vim9: missing support for ro-vars in interface
Solution: Support only read-only object variables in an interface,
          add additional checks when parsing class definitions.

closes: #13183
cloess: #13184
cloess: #13185.
closes: #13188

Signed-off-by: Christian Brabandt <cb@256bit.org>
Co-authored-by: Yegappan Lakshmanan <yegappan@yahoo.com>

8 files changed, 190 insertions, 102 deletions

diff --git a/runtime/doc/tags b/runtime/doc/tags
index daf59a90d9..918e767721 100644
--- a/runtime/doc/tags
+++ b/runtime/doc/tags
@@ -4479,6 +4479,9 @@ E1380	vim9class.txt	/*E1380*
 E1381	vim9class.txt	/*E1381*
 E1382	vim9class.txt	/*E1382*
 E1383	vim9class.txt	/*E1383*
+E1387	vim9class.txt	/*E1387*
+E1388	vim9class.txt	/*E1388*
+E1389	vim9class.txt	/*E1389*
 E139	message.txt	/*E139*
 E140	message.txt	/*E140*
 E1400	builtin.txt	/*E1400*
diff --git a/runtime/doc/vim9class.txt b/runtime/doc/vim9class.txt
index a27f7319a7..b230cb8007 100644
--- a/runtime/doc/vim9class.txt
+++ b/runtime/doc/vim9class.txt
@@ -464,10 +464,10 @@ The interface name can be used as a type: >
 	   echo $'the surface is {shape.Surface()}'
 	endfor
 <
-						*E1378* *E1379* *E1380*
-An interface can have only instance variables (read-only and read-write
-access) and methods.  An interface cannot contain private variables, private
-methods, class variables and class methods.
+					*E1378* *E1379* *E1380* *E1387*
+An interface can contain only object methods and read-only object variables.
+An interface cannot contain read-write and private object variables, private
+object methods, class variables and class methods.
 
 An interface can extend another interface using "extends".  The sub-interface
 inherits all the instance variables and methods from the super interface.
@@ -563,7 +563,7 @@ will be added automatically.
 
 
 A class implementing an interface ~
-						*implements* *E1346* *E1347*
+					*implements* *E1346* *E1347* *E1389*
 A class can implement one or more interfaces.  The "implements" keyword can
 only appear once *E1350* .  Multiple interfaces can be specified, separated by
 commas.  Each interface name can appear only once. *E1351*
@@ -577,7 +577,7 @@ interface, which is often done in many languages, especially Java.
 
 
 Items in a class ~
-						*E1318* *E1325*
+						*E1318* *E1325* *E1388*
 Inside a class, in between `:class` and `:endclass`, these items can appear:
 - An object variable declaration: >
 	this._privateVariableName: memberType
diff --git a/src/errors.h b/src/errors.h
index 0361d71345..7260c30cbf 100644
--- a/src/errors.h
+++ b/src/errors.h
@@ -3523,6 +3523,12 @@ EXTERN char e_class_method_str_accessible_only_using_class_str[]
 	INIT(= N_("E1385: Class method \"%s\" accessible only using class \"%s\""));
 EXTERN char e_object_method_str_accessible_only_using_object_str[]
 	INIT(= N_("E1386: Object method \"%s\" accessible only using class \"%s\" object"));
+EXTERN char e_public_member_not_supported_in_interface[]
+	INIT(= N_("E1387: Public variable not supported in an interface"));
+EXTERN char e_public_keyword_not_supported_for_method[]
+	INIT(= N_("E1388: Public keyword not supported for a method"));
+EXTERN char e_missing_name_after_implements[]
+	INIT(= N_("E1389: Missing name after implements"));
 #endif
 EXTERN char e_cannot_mix_positional_and_non_positional_str[]
 	INIT(= N_("E1400: Cannot mix positional and non-positional arguments: %s"));
@@ -3538,4 +3544,4 @@ EXTERN char e_invalid_format_specifier_str[]
 	INIT(= N_("E1405: Invalid format specifier: %s"));
 EXTERN char e_aptypes_is_null_nr_str[]
 	INIT(= "E1408: Internal error: ap_types or ap_types[idx] is NULL: %d: %s");
-// E1387 - E1399 unused
+// E1390 - E1399 unused
diff --git a/src/eval.c b/src/eval.c
index eaca2f5fa0..5edda68206 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -1220,8 +1220,7 @@ get_lval(
 	int r = OK;
 	if (v_type == VAR_LIST && lp->ll_tv->vval.v_list == NULL)
 	    r = rettv_list_alloc(lp->ll_tv);
-	else if (v_type == VAR_BLOB
-					     && lp->ll_tv->vval.v_blob == NULL)
+	else if (v_type == VAR_BLOB && lp->ll_tv->vval.v_blob == NULL)
 	    r = rettv_blob_alloc(lp->ll_tv);
 	if (r == FAIL)
 	    return NULL;
diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim
index 37303297c0..2fd66c937f 100644
--- a/src/testdir/test_vim9_class.vim
+++ b/src/testdir/test_vim9_class.vim
@@ -291,17 +291,80 @@ def Test_class_basic()
 
   # Use a multi-line initialization for a member variable
   lines =<< trim END
-  vim9script
-  class A
-    this.y = {
-      X: 1
-    }
-  endclass
-  var a = A.new()
+    vim9script
+    class A
+      this.y = {
+        X: 1
+      }
+    endclass
+    var a = A.new()
   END
   v9.CheckSourceSuccess(lines)
 enddef
 
+" Tests for object/class methods in a class
+def Test_class_def_method()
+  # Using the "public" keyword when defining an object method
+  var lines =<< trim END
+    vim9script
+    class A
+      public def Foo()
+      enddef
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1331: Public must be followed by "this" or "static"', 3)
+
+  # Using the "public" keyword when defining a class method
+  lines =<< trim END
+    vim9script
+    class A
+      public static def Foo()
+      enddef
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1388: Public keyword not supported for a method', 3)
+
+  # Using the "public" keyword when defining an object private method
+  lines =<< trim END
+    vim9script
+    class A
+      public def _Foo()
+      enddef
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1331: Public must be followed by "this" or "static"', 3)
+
+  # Using the "public" keyword when defining a class private method
+  lines =<< trim END
+    vim9script
+    class A
+      public static def _Foo()
+      enddef
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1388: Public keyword not supported for a method', 3)
+
+  # Using a "def" keyword without an object method name
+  lines =<< trim END
+    vim9script
+    class A
+      def
+      enddef
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1318: Not a valid command in a class: def', 3)
+
+  # Using a "def" keyword without a class method name
+  lines =<< trim END
+    vim9script
+    class A
+      static def
+      enddef
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1318: Not a valid command in a class: static def', 3)
+enddef
+
 def Test_class_defined_twice()
   # class defined twice should fail
   var lines =<< trim END
@@ -697,36 +760,6 @@ def Test_assignment_with_operator()
       assert_equal(23, f.x)
   END
   v9.CheckSourceSuccess(lines)
-
-  # do the same thing, but through an interface
-  lines =<< trim END
-      vim9script
-
-      interface I
-        public this.x: number
-      endinterface
-
-      class Foo implements I
-        public this.x: number
-
-        def Add(n: number)
-          var i: I = this
-          i.x += n
-        enddef
-      endclass
-
-      var f =  Foo.new(3)
-      f.Add(17)
-      assert_equal(20, f.x)
-
-      def AddToFoo(i: I)
-        i.x += 3
-      enddef
-
-      AddToFoo(f)
-      assert_equal(23, f.x)
-  END
-  v9.CheckSourceSuccess(lines)
 enddef
 
 def Test_list_of_objects()
@@ -1743,7 +1776,6 @@ func Test_interface_garbagecollect()
 
     interface I
       this.ro_obj_var: number
-      public this.rw_obj_var: number
 
       def ObjFoo(): number
     endinterface
@@ -1753,7 +1785,6 @@ func Test_interface_garbagecollect()
       public static rw_class_var: number = 20
       static _priv_class_var: number = 30
       this.ro_obj_var: number = 40
-      public this.rw_obj_var: number = 50
       this._priv_obj_var: number = 60
 
       static def _ClassBar(): number
@@ -1769,16 +1800,16 @@ func Test_interface_garbagecollect()
       enddef
 
       def ObjFoo(): number
-        return this.ro_obj_var + this.rw_obj_var + this._ObjBar()
+        return this.ro_obj_var + this._ObjBar()
       enddef
     endclass
 
     assert_equal(60, A.ClassFoo())
     var o = A.new()
-    assert_equal(150, o.ObjFoo())
+    assert_equal(100, o.ObjFoo())
     test_garbagecollect_now()
     assert_equal(60, A.ClassFoo())
-    assert_equal(150, o.ObjFoo())
+    assert_equal(100, o.ObjFoo())
   END
   call v9.CheckSourceSuccess(lines)
 endfunc
@@ -1938,8 +1969,7 @@ def Test_interface_basics()
   var lines =<< trim END
       vim9script
       interface Something
-        this.ro_var: string
-        public this.rw_var: list<number>
+        this.ro_var: list<number>
         def GetCount(): number
       endinterface
   END
@@ -2127,14 +2157,15 @@ 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
+        public this.lnum: number = 5
         this.errpos: number = 42
-        public this.label: string = 'label'
+        this.label: string = 'label'
       endclass
 
       def Test()
@@ -2142,10 +2173,6 @@ def Test_class_implements_interface()
 
         assert_equal('label', result.label)
         assert_equal(42, result.errpos)
-
-        result.label = 'different'
-        assert_equal('different', result.label)
-        assert_equal(42, result.errpos)
       enddef
 
       Test()
@@ -2251,14 +2278,14 @@ def Test_class_implements_interface()
     vim9script
 
     interface I1
-        public this.mvar1: number
-        public this.mvar2: number
+        this.mvar1: number
+        this.mvar2: number
     endinterface
 
     # NOTE: the order is swapped
     class A implements I1
-        public this.mvar2: number
-        public this.mvar1: number
+        this.mvar2: number
+        this.mvar1: number
         public static svar2: number
         public static svar1: number
         def new()
@@ -2303,20 +2330,20 @@ def Test_class_implements_interface()
     vim9script
 
     interface I1
-        public this.mvar1: number
-        public this.mvar2: number
+        this.mvar1: number
+        this.mvar2: number
     endinterface
 
     interface I2
-        public this.mvar3: number
-        public this.mvar4: number
+        this.mvar3: number
+        this.mvar4: number
     endinterface
 
     class A implements I1
         public static svar1: number
         public static svar2: number
-        public this.mvar1: number
-        public this.mvar2: number
+        this.mvar1: number
+        this.mvar2: number
         def new()
             svar1 = 11
             svar2 = 12
@@ -2326,10 +2353,10 @@ def Test_class_implements_interface()
     endclass
 
     class B extends A implements I2
-        public static svar3: number
-        public static svar4: number
-        public this.mvar3: number
-        public this.mvar4: number
+        static svar3: number
+        static svar4: number
+        this.mvar3: number
+        this.mvar4: number
         def new()
             svar3 = 23
             svar4 = 24
@@ -2368,6 +2395,36 @@ def Test_class_implements_interface()
     assert_equal([[131, 132], [133, 134]], [F2(oc), F4(oc)])
   END
   v9.CheckSourceSuccess(lines)
+
+  # Using two interface names without a space after the ","
+  lines =<< trim END
+    vim9script
+    interface A
+    endinterface
+    interface B
+    endinterface
+    class C implements A,B
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1315: White space required after name: A,B', 6)
+
+  # No interface name after a comma
+  lines =<< trim END
+    vim9script
+    interface A
+    endinterface
+    class B implements A,
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1389: Missing name after implements', 4)
+
+  # No interface name after implements
+  lines =<< trim END
+    vim9script
+    class A implements
+    endclass
+  END
+  v9.CheckSourceFailure(lines, 'E1389: Missing name after implements', 2)
 enddef
 
 def Test_call_interface_method()
@@ -3558,19 +3615,6 @@ def Test_private_object_method()
   END
   v9.CheckSourceFailure(lines, 'E1366: Cannot access private method: _Foo')
 
-  # Try to use "public" keyword when defining a private method
-  lines =<< trim END
-    vim9script
-
-    class A
-      public def _Foo()
-      enddef
-    endclass
-    var a = A.new()
-    a._Foo()
-  END
-  v9.CheckSourceFailure(lines, 'E1331: Public must be followed by "this" or "static"')
-
   # Define two private methods with the same name
   lines =<< trim END
     vim9script
@@ -4257,10 +4301,10 @@ def Test_change_interface_member_access()
   var lines =<< trim END
     vim9script
     interface A
-      public this.val: number
+      this.val: number
     endinterface
     class B implements A
-      this.val = 10
+      public this.val = 10
     endclass
   END
   v9.CheckSourceFailure(lines, 'E1367: Access level of variable "val" of interface "A" is different')
@@ -5101,7 +5145,7 @@ def Test_extend_empty_class()
 enddef
 
 " A interface cannot have a static variable or a static method or a private
-" variable or a private method
+" variable or a private method or a public variable
 def Test_interface_with_unsupported_members()
   var lines =<< trim END
     vim9script
@@ -5125,12 +5169,20 @@ def Test_interface_with_unsupported_members()
       public static num: number
     endinterface
   END
-  v9.CheckSourceFailure(lines, 'E1378: Static member not supported in an interface')
+  v9.CheckSourceFailure(lines, 'E1387: Public variable not supported in an interface')
+
+  lines =<< trim END
+    vim9script
+    interface A
+      public static num: number
+    endinterface
+  END
+  v9.CheckSourceFailure(lines, 'E1387: Public variable not supported in an interface')
 
   lines =<< trim END
     vim9script
     interface A
-      public static _num: number
+      static _num: number
     endinterface
   END
   v9.CheckSourceFailure(lines, 'E1378: Static member not supported in an interface')
@@ -5177,14 +5229,14 @@ def Test_extend_interface()
       def Foo()
     endinterface
     interface B extends A
-      public this.var2: dict<string>
+      this.var2: dict<string>
       def Bar()
     endinterface
     class C implements A, B
       this.var1 = [1, 2]
       def Foo()
       enddef
-      public this.var2 = {a: '1'}
+      this.var2 = {a: '1'}
       def Bar()
       enddef
     endclass
@@ -5197,10 +5249,10 @@ def Test_extend_interface()
       def Foo()
     endinterface
     interface B extends A
-      public this.var2: dict<string>
+      this.var2: dict<string>
     endinterface
     class C implements A, B
-      public this.var2 = {a: '1'}
+      this.var2 = {a: '1'}
     endclass
   END
   v9.CheckSourceFailure(lines, 'E1349: Method "Foo" of interface "A" is not implemented')
@@ -5211,7 +5263,7 @@ def Test_extend_interface()
       def Foo()
     endinterface
     interface B extends A
-      public this.var2: dict<string>
+      this.var2: dict<string>
     endinterface
     class C implements A, B
       def Foo()
diff --git a/src/testdir/test_vim9_disassemble.vim b/src/testdir/test_vim9_disassemble.vim
index 4cf4dfe5b0..206908b79c 100644
--- a/src/testdir/test_vim9_disassemble.vim
+++ b/src/testdir/test_vim9_disassemble.vim
@@ -3052,15 +3052,15 @@ def Test_disassemble_interface_static_member()
   var lines =<< trim END
     vim9script
     interface I
-      public this.o_var: number
-      public this.o_var2: number
+      this.o_var: number
+      this.o_var2: number
     endinterface
 
     class C implements I
       public static s_var: number
-      public this.o_var: number
+      this.o_var: number
       public static s_var2: number
-      public this.o_var2: number
+      this.o_var2: number
     endclass
 
     def F1(i: I)
diff --git a/src/version.c b/src/version.c
index ddf9a745ee..aef4b1a641 100644
--- a/src/version.c
+++ b/src/version.c
@@ -700,6 +700,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    1945,
+/**/
     1944,
 /**/
     1943,
diff --git a/src/vim9class.c b/src/vim9class.c
index b5e11ca7fb..8b07d76498 100644
--- a/src/vim9class.c
+++ b/src/vim9class.c
@@ -1430,11 +1430,19 @@ ex_class(exarg_T *eap)
 	    {
 		char_u *impl_end = find_name_end(arg, NULL, NULL,
 							      FNE_CHECK_START);
-		if (!IS_WHITE_OR_NUL(*impl_end) && *impl_end != ',')
+		if ((!IS_WHITE_OR_NUL(*impl_end) && *impl_end != ',')
+			|| (*impl_end == ','
+			    && !IS_WHITE_OR_NUL(*(impl_end + 1))))
 		{
 		    semsg(_(e_white_space_required_after_name_str), arg);
 		    goto early_ret;
 		}
+		if (impl_end - arg == 0)
+		{
+		    emsg(_(e_missing_name_after_implements));
+		    goto early_ret;
+		}
+
 		char_u *iname = vim_strnsave(arg, impl_end - arg);
 		if (iname == NULL)
 		    goto early_ret;
@@ -1539,6 +1547,11 @@ early_ret:
 		semsg(_(e_command_cannot_be_shortened_str), line);
 		break;
 	    }
+	    if (!is_class)
+	    {
+		emsg(_(e_public_member_not_supported_in_interface));
+		break;
+	    }
 	    has_public = TRUE;
 	    p = skipwhite(line + 6);
 
@@ -1664,7 +1677,20 @@ early_ret:
 	    exarg_T	ea;
 	    garray_T	lines_to_free;
 
-	    // TODO: error for "public static def Func()"?
+	    if (has_public)
+	    {
+		// "public" keyword is not supported when defining an object or
+		// class method
+		emsg(_(e_public_keyword_not_supported_for_method));
+		break;
+	    }
+
+	    if (*p == NUL)
+	    {
+		// No method name following def
+		semsg(_(e_not_valid_command_in_class_str), line);
+		break;
+	    }
 
 	    CLEAR_FIELD(ea);
 	    ea.cmd = line;