[vim9class] Unexpected method dispatch when type is interface

[errael]

Steps to reproduce

Note this is vim-9.1 behavior.

Came across this sorting out some recent issues. I was surprised, wrote the test in Java and got the expected result. @dkearns what do you think? @zzzyxwvut @chrisbra @yegappan note.

vim9script

interface I
    def F(): string
endinterface

class A implements I
    def F(): string
        return 'A'
    enddef
endclass

class C extends A
    def F(): string
        return 'C'
    enddef
endclass

# Unexpected: following outputs 'A'. Expect 'C'
def TestI(i: I)
    echo i.F()
enddef
TestI(C.new())

outputs

A

Was looking atAbstract method overrides concrete method, but that's not related to this (maybe loosely related).

Expected behaviour

Expect dispatch of function F() on an object to be "dynamic dispatch". For example, wrote the above test in Java and it works as expected.

Another example, this works as expected.

# Expected: following outputs 'C'.
def TestA(a: A)
    echo a.F()
enddef
TestA(C.new())

Version of Vim

9.1.661

Environment

ubuntu/gtk3

Logs and stack traces

No response

[errael]

And in the example if class C extends A is changed to

class C extends A implements I

Then it dispatches correctly.

[errael]

And in the example if class C extends A is changed to

class C extends A implements I

Then it dispatches correctly.

And BTW, with the OP example, doing

echo 'instanceof(C.new(), I): ' instanceof(C.new(), I)

outputs

instanceof(C.new(), I):  1

Aha, so there's some magic during parse that makes class C more than just an instance of interface I if implments I is present.

[zzzyxwvut]

It is good to have this issue formally fleshed out. I have
already broached the subject of it here:

Then there is also a wrong-result success: comment out the stipulated by (3) implements clauses, BUT make A concrete, or keep A abstract and implement Odd(), then observe the uniform output of TestOdd*().

Now it remains to be seen how this requirement for the
repetitive use of implements can be addressed:

• Always repeat implements (enforce it during compilation of
  derived classes by failing to compile any class that does
  not repeat implements, and document it; because sooner or
  later a method member of an interface will be invoked on
  the said interface type via an instance of a class that
  should have implemented it).
• Treat the repetition of implements as a matter of style
  (adjust the current implementation to allow for only the
  initial use of implements and ensure the correct method
  dispatch for any derived class instances regardless of
  whether implements is repeated for their types or not).

[errael]

It is good to have this issue formally fleshed out. I have already broached the subject of it here:

Then there is also a wrong-result success: comment out the stipulated by (3) implements clauses, BUT make A concrete, or keep A abstract and implement Odd(), then observe the uniform output of TestOdd*().

I must admit that I see that post as TL;DR. I do a lot better with short pass/fail tests.

Now it remains to be seen how this requirement for the repetitive use of implements can be addressed:

* Always repeat `implements` 

* Treat the repetition of `implements` as a matter of style

I certainly believe the 2nd is the way to go. It's natural and compatible with currently written code (assuming the developer wants dynamic dispatch).

[dkearns]

Yes, I'd strongly prefer not to kludge around the bug by requiring the redundant implements clauses.

P.S. Sorry for my tardiness in responding to all of these issues, I'll try and get back to it next week.

[errael]

I've got a partially working fix for this.

I'm beginning to think that the current interface mechanisms will not allow a full fix. Still investigating.

[errael]

There's a simple patch which fixes the problem when extending a class A implements I in the same file. But it fails with imports. The patch can probably be fixed for a simple import case, but I'd guess it would fail when the extended class and the interface it implements are in different files. So either the fix needs to be made somewhere else or (more likely) more information needs to be available/saved about the implemented interfaces.

Using the test from #15484

Following without the patch

:class: created 'I'
:class: created 'A'
validate_implements: adding interface'I'
:class: created 'C'
object_index_from_itf_index: itf: 'I', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'A', is_method: 1                    <<<<<<<<<<<<<<<<<<<<<<<<
         class 'A', is_method: 1
      method_offset: 1
      super: 'A'
      === inner for
         class 'A', is_method: 1
         break-found
      method_offset: 1
object_index_from_itf_index: itf: 'A', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'C', is_method: 1
         break-found
      method_offset: 0

Following with the patch

:class: created 'I'
:class: created 'A'
validate_implements: adding interface'I'
:class: created 'C'
validate... inherited: check 'I', missing 1
   Adding inherited interface 'I'.
   ... ADDED 'I'.                                   <<<<<<<<<<<<<<<<<<<<<<<<
object_index_from_itf_index: itf: 'I', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'C', is_method: 1                    <<<<<<<<<<<<<<<<<<<<<<<<
         break-found
      method_offset: 0
object_index_from_itf_index: itf: 'A', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'C', is_method: 1
         break-found
      method_offset: 0

A slight modification of the test to use imports. Fails as expected.

vim9script
import '/home/err/work/bugs/vim/concrete-abstract-method/interface_dispatch_import_2.vim' as i_imp

class C extends i_imp.A # implements i_imp.I
    def F(): string
        return 'C'
    enddef
endclass

# Unexpected: following outputs 'A'. Expect 'C'
def TestI(i: i_imp.I): string
    return i.F()
enddef
echo TestI(C.new())

/home/err/work/bugs/vim/concrete-abstract-method/interface_dispatch_import_2.vim

vim9script

export interface I
    def F(): string
endinterface

export class A implements I
    def F(): string
        return 'A'
    enddef
endclass

trace when extending imported class

:class: created 'I'
:class: created 'A'
validate_implements: adding interface'I'
:class: created 'C'
validate... inherited: check 'I', missing 0
object_index_from_itf_index: itf: 'I', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'A', is_method: 1
         class 'A', is_method: 1
      method_offset: 1
      super: 'A'
      === inner for
         class 'A', is_method: 1
         break-found
      method_offset: 1
object_index_from_itf_index: itf: 'A', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'C', is_method: 1
         break-found
      method_offset: 0

[errael]

Opened #15495 which fixes the failure in the OP. It is not a complete fix when imports are involved.

[errael]

There are some decisions that have to made related to this problem. @zzzyxwvut discusses two behaviors that can be summarized as

1. Always repeat implements
2. Treat the repetition of implements as a matter of style

(2) is superior, but might be complicated as discussed. (1), at first look, seems simpler but but is very cumbersome; consider extending an imported class hierarchy which implements some interfaces; with (1) if any superclass gets a change in interfaces, adding or removing, then the importing class would have to change.

Note that the traces in the previous comment can be turned on/off with a #define in patch #15495.

[zzzyxwvut]

If we shall end up with (1), its silver lining is that this
implements repetition imposed by it can still be backward
compatible with (2) any time that improvement is up for
consideration at a later date.

[zzzyxwvut]

I'm trying out your example from above, with LOG_CLASS
defined, and, after uncommenting implements i_imp.I,
instead of posted:

object_index_from_itf_index: itf: 'A', is_method 1, idx 0, cl 'C'

I see:

object_index_from_itf_index: itf: 'I', is_method 1, idx 0, cl 'C'

Is it, i.e. 'A' not 'I', a typo?

[errael]

Oops, the description should read trace when extending imported class. I updated the post. (the trace looks OK)

For the case where the implements is used, I see the trace

:class: created 'I'
:class: created 'A'
validate_implements: adding interface'I'
:class: created 'C'
validate_implements: adding interface'i_imp.I'
validate... inherited: check 'I', missing 0
object_index_from_itf_index: itf: 'I', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'C', is_method: 1
         break-found
      method_offset: 0
object_index_from_itf_index: itf: 'A', is_method 1, idx 0, cl 'C'
   method_offset: 0
   === outer for
      super: 'C'
      === inner for
         class 'C', is_method: 1
         break-found
      method_offset: 0

[errael]

If we shall end up with (1)

To get (1) you can look around

https://github.com/errael/vim/blob/27a30246e2d51549e0249faa9753c5f1d6975391/src/vim9class.c#L880

If something's missing, then give an error.

But that code isn't quite right for checking the full hierarchy. In particular

• The full hierarchy of all interfaces from any/all superclasses must be check
• probably should check new class against extends_cl->class_interfaces_cl (and all its hierarchy) and not do eval_variable_import (ignore the names)

[errael]

Here's a zip of the experimental patch in case it doesn't survive cleanup.

15495.patch.zip