[vim9class] problems with method dispatch: super -> abstract -> concrete

[errael]

Steps to reproduce

vim9script

class A
    def ToString()
        echo 'A'
    enddef
endclass

abstract class B extends A
    abstract def ToString()
endclass

class C extends B
    def ToString()
        super.ToString()
    enddef
endclass

C.new().ToString()

Run this and nothing happens.

Expected behaviour

Either A should be output or there should be an error.

Tried this on java, get an error

C.java ... abstract method ToString() in ... B cannot be accessed directly

Version of Vim

9.1.678

Environment

ubuntu/gtk3

Logs and stack traces

No response

[errael]

Tried this with the missing return fix, see #15512. In that case an error is generated

    Error detected while processing :source buffer=1[19]..function <SNR>39_C.ToString:
    line    1:
    E117: Unknown function: ToString

Not the best error message, but not too bad. See Java's message above.

@zzzyxwvut @chrisbra This is an example where the patch for "missing return" gives an error and without out things fail silently.

[zzzyxwvut]

Apparently, we missed a test case for the super patch.

I would expect here an error:
E1373: Abstract method "ToString" is not implemented.

The author of C is in the wrong and should rewrite its
implementation of ToString() not to use super. reaching for
an abstract method.

The author of B should have provided non-abstract methods
(possibly _protected) that access concrete implementations,
if any, of their supertypes for all overridden concrete
methods by abstract methods so that A's implementations can
be used in derived types (some examples).

[errael]

IMHO, this isn't a bug I introduced with #15477, but could have been fixed with it. But with that PR in place it's a quick fix.

Here's a fix if someone want to turn it into a PR

DispatchSuperAbstract.patch.zip

[zzzyxwvut]

A few more test cases that work with or without the super
part of the linked patch:

(1)

super and the initial implements clause

vim9script

interface I
    def ToString(): string
endinterface

# Note that A does not implement I.
class A
    def ToString(): string
        return 'A'
    enddef
endclass

class B extends A implements I
    def ToString(): string
        return super.ToString()
    enddef
endclass

def TestI(i: I): string
    return i.ToString()
enddef

echo 'A' == B.new().ToString()
echo 'A' == TestI(B.new())

(2)

super and an abstract class with no abstract methods

vim9script

class A
    def ToString(): string
        return 'A'
    enddef
endclass

# An abstract class with no abstract methods.
abstract class B extends A
endclass

class C extends B
    def ToString(): string
        return super.ToString()
    enddef
endclass

def TestA(a: A): string
    return a.ToString()
enddef

echo 'A' == C.new().ToString()
echo 'A' == TestA(A.new())
echo 'A' == TestA(C.new())

(3)

super and an abstract class with no abstract methods and the initial implements clause

vim9script

interface I
    def ToString(): string
endinterface

# Note that A does not implement I.
class A
    def ToString(): string
        return 'A'
    enddef
endclass

# An abstract class with no abstract methods.
abstract class B extends A implements I
endclass

class C extends B implements I
    def ToString(): string
        return super.ToString()
    enddef
endclass

# Note that A.ToString() is different from I.ToString().
def TestA(a: A): string
    return a.ToString()
enddef

echo 'A' == C.new().ToString()
echo 'A' == TestA(A.new())
echo 'A' == TestA(C.new())

[chrisbra]

This is the attached patch verbatim pasted:

# HG changeset patch
# User Ernie Rael <errael@raelity.com>
# Date 1723857152 25200
#      Fri Aug 16 18:12:32 2024 -0700
# Node ID 581eca46012a91f5bfb22d8c5f5cfbce0b1f3cef
# Parent  d0ec288fe429c438c27c92ce11384796116ea2de
Generate error for method dispatch: super -> abstract
Fix #15514

diff --git a/src/errors.h b/src/errors.h
--- a/src/errors.h
+++ b/src/errors.h
@@ -3616,8 +3616,10 @@
 	INIT(= N_("E1428: Duplicate enum value: %s"));
 EXTERN char e_class_can_only_be_used_in_script[]
 	INIT(= N_("E1429: Class can only be used in a script"));
-#endif
-// E1429 - E1499 unused (reserved for Vim9 class support)
+EXTERN char e_abstract_method_str_direct[]
+	INIT(= N_("E1430: Abstract method \"%s\" cannot be accessed directly"));
+#endif
+// E1431 - E1499 unused (reserved for Vim9 class support)
 EXTERN char e_cannot_mix_positional_and_non_positional_str[]
 	INIT(= N_("E1500: Cannot mix positional and non-positional arguments: %s"));
 EXTERN char e_fmt_arg_nr_unused_str[]
diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim
--- a/src/testdir/test_vim9_class.vim
+++ b/src/testdir/test_vim9_class.vim
@@ -3385,6 +3385,59 @@
     assert_equal(['E.SF()', 'D.SF()', 'B.SF()', 'A._H()', 'A.F()', 'F._G()'], call_chain)
   END
   v9.CheckSourceSuccess(lines)
+
+  # problems with method dispatch: super -> abstract
+  # https://github.com/vim/vim/issues/15514
+  lines =<< trim END
+    vim9script
+    abstract class B
+      abstract def ToString(): string
+    endclass
+
+    class C extends B
+      def ToString(): string
+        return super.ToString()
+      enddef
+    endclass
+
+    try
+      defcompile C.ToString
+      call assert_false(1, 'command should have failed')
+    catch
+      call assert_exception('E1430:')
+    endtry
+  END
+  v9.CheckSourceSuccess(lines)
+
+  # problems with method dispatch: super -> abstract -> concrete
+  lines =<< trim END
+    vim9script
+
+    class A
+      def ToString()
+        echo 'A'
+      enddef
+    endclass
+
+    abstract class B extends A
+      abstract def ToString()
+    endclass
+
+    class C extends B
+      def ToString()
+        super.ToString()
+      enddef
+    endclass
+
+    try
+      defcompile C.ToString
+      call assert_false(1, 'command should have failed')
+    catch
+      call assert_exception('E1430:')
+    endtry
+  END
+  v9.CheckSourceSuccess(lines)
+
 enddef
 
 def Test_class_import()
diff --git a/src/vim9expr.c b/src/vim9expr.c
--- a/src/vim9expr.c
+++ b/src/vim9expr.c
@@ -373,6 +373,11 @@
 		break;
 	    }
 	}
+	if (is_super && IS_ABSTRACT_METHOD(ufunc))
+	{
+	    semsg(_(e_abstract_method_str_direct), ufunc->uf_name);
+	    return FAIL;
+	}
 	ocmember_T  *ocm = NULL;
 	if (ufunc == NULL)
 	{
# HG changeset patch
# User Ernie Rael <errael@raelity.com>
# Date 1723823145 25200
#      Fri Aug 16 08:45:45 2024 -0700
# Node ID d0ec288fe429c438c27c92ce11384796116ea2de
# Parent  de5f583cd422620b84e4f11f223c74e03f8385f9
Fix 15512. Missing return. Was: fix #15432
During compilation, don't look for a return statement for an abstract method.

diff --git a/src/testdir/test_vim9_class.vim b/src/testdir/test_vim9_class.vim
--- a/src/testdir/test_vim9_class.vim
+++ b/src/testdir/test_vim9_class.vim
@@ -6264,6 +6264,27 @@
     assert_equal('foo', A.Foo())
   END
   v9.CheckSourceSuccess(lines)
+
+  # Invoke method returning a value through the abstract class. See #15432.
+  lines =<< trim END
+    vim9script
+
+    abstract class A
+        abstract def String(): string
+    endclass
+
+    class B extends A
+        def String(): string
+            return 'B'
+        enddef
+    endclass
+
+    def F(o: A)
+        assert_equal('B', o.String())
+    enddef
+    F(B.new())
+  END
+  v9.CheckSourceSuccess(lines)
 enddef
 
 " Test for calling a class method from a subclass
diff --git a/src/vim9compile.c b/src/vim9compile.c
--- a/src/vim9compile.c
+++ b/src/vim9compile.c
@@ -4120,8 +4120,9 @@
     ufunc->uf_args_visible = ufunc->uf_args.ga_len;
 
     // Compiling a function in an interface is done to get the function type.
-    // No code is actually compiled.
-    if (ufunc->uf_class != NULL && IS_INTERFACE(ufunc->uf_class))
+    // No code is actually compiled. Same goes for an abstract method.
+    if ((ufunc->uf_class != NULL && IS_INTERFACE(ufunc->uf_class))
+	|| IS_ABSTRACT_METHOD(ufunc))
     {
 	ufunc->uf_def_status = UF_NOT_COMPILED;
 	ret = OK;

[zzzyxwvut]

There are two patches included: the super patch (top) is
good, it works as expected (there are a few more test cases
posted above); the abstract patch (bottom) is the one that
ended up being reverted in #15441.

[errael]

As mentioned, oh so many times, the patch should never have been reverted, IMHO. In fact, I don't think the tests with the patch will run without the "missing return" patch.

[zzzyxwvut]

I see the same new E1430 in your test ‘as is’ and after
changing ToString(): void to ToString(): string and adding
returns.

We need interface methods invoked on their interface type
to be a factor for the abstract problem, right?