vim
vim copied to clipboard
vim
[vim9class] problems with method dispatch: super -> abstract -> concrete
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
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.
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).
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
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())
This is the attached patch verbatim pasted:
# HG changeset patch
# User Ernie Rael <[email protected]>
# 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 <[email protected]>
# 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;
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.
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.
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?