assemblyscript icon indicating copy to clipboard operation
assemblyscript copied to clipboard

Support interfaces having multiple base interfaces

Open CountBleck opened this issue 2 years ago • 13 comments

Changes proposed in this pull request: ⯈Adding support for an interface extending multiple interfaces.

  • [x] I've read the contributing guidelines
  • [x] I've added my name and email to the NOTICE file

CountBleck avatar Jun 21 '23 05:06 CountBleck

@dcodeIO How does it look?

CountBleck avatar Jul 23 '23 16:07 CountBleck

Whoops, needed to rebase. It should work now.

CountBleck avatar Jul 23 '23 17:07 CountBleck

Could you explain more about the target behavior of extended interfaces and how to handle the conflict between different base interfaces. Maybe you need to add more test cases for those.

HerrCai0907 avatar Jan 15 '24 05:01 HerrCai0907

Could you explain more about the target behavior of extended interfaces and how to handle the conflict between different base interfaces. Maybe you need to add more test cases for those.

Ah, I didn't see this. From my understanding, I don't think there should be any conflict, because the implementer needs to satisfy all of the requirements imposed by each interface, whereas the interface has no requirements imposed on it by the implementer, asides from a case being added to each of the interface's override (virtual) stubs.

I could of course be very wrong about this, but I don't know what else should be tested.

CountBleck avatar Jan 15 '24 07:01 CountBleck

interface A {
  f(): void;
}

interface B {
  f(): i32;
}

interface C extends A, B {}

class D implements C {
  f(): void {}
}

For this case, TS will diagnose Interface 'C' cannot simultaneously extend types 'A' and 'B'. Named property 'f' of types 'A' and 'B' are not identical.ts(2320) but AS will not.

HerrCai0907 avatar Jan 15 '24 08:01 HerrCai0907

By the way,

interface A {
    m: Object | null
}

interface B {
    m: Object
}

interface X extends A, B {
    // m: Object
}

isn't valid in TypeScript, but uncommenting that line will fix the error.

CountBleck avatar Jan 16 '24 03:01 CountBleck

isn't valid in TypeScript, but uncommenting that line will fix the error.

So at least we need to know what is correct and what is wrong firstly to avoid huge mismatch with TS. Normally type system is very complex module which has lots of corner cases. Maybe we can start from the most strict mode and forbidden each same name declaration and release limitation one by one to avoid publishing break change over and over.

HerrCai0907 avatar Jan 16 '24 05:01 HerrCai0907

I believe the error is only emitted if the property isn't explicitly defined on the interface. Maybe some more digging is warranted though.

Also, another thing about the "simultaneously extend" error: for each conflicting property, it chooses the first interface with that property to be the first interface mentioned in each error (for that property), and the second interface is each of the remaining interfaces.

I can't explain it that clearly, so here's an example. x and y are the conflicting properties. For x, B comes first, so there are errors for B and C, as well as B and D. For y, C comes first, so there are errors for C and D, as well as C and E.

CountBleck avatar Jan 16 '24 05:01 CountBleck

So this PR still missing combination check when inheriting from difference base interfaces, right?

HerrCai0907 avatar Jan 16 '24 05:01 HerrCai0907

Both of those things I described are part of that diagnostic, which I haven't implemented yet.

CountBleck avatar Jan 16 '24 05:01 CountBleck

FWIW we're actually running this PR in our fork of assemblyscript, and we added a small number of extra diagnostics messages to our custom compiler for different interface scenarios. I can share our test cases if that would be helpful?

lebrunel avatar Jan 16 '24 11:01 lebrunel

@lebrunel that would be great, thanks!

CountBleck avatar Jan 16 '24 15:01 CountBleck

The following is by no means exhaustive (we don't have a test for the simultaneous extending case mentioned above), but hopefully these are in someway helpful. At the very least you'll be able to write some tests that fail :P

// implementing a different method signature should fail
// Types of property `m` are incompatible.
interface A {
  m(): void;
}

class B implements A {
  m(n: u8): void {}
}
// implementing a different method signature from nested interfaces should fail
// Types of property `m1` are incompatible.
interface A {
  m1(): void;
}

interface B extends A {
  m2(): void;
}

interface C extends B {
  m3(): void;
}

class D implements C {
  m1(n: u8): void {}
  m2(): void {}
  m3(): void {}
}
// implementing a different method signature with compatible types should pass
interface A {
  m1(): A;
}

interface B extends A {
  m2(): B;
}

class C implements A {
  m1(): C { return this };
}

class D implements B {
  m1(): D { return this };
  m2(): B { return this };
}
// extending a different method signature should fail
// Types of property `m` are incompatible.
interface A {
  m(): void;
}

interface B extends A {
  m(n: u8): void;
}
// extending a different method signature from nested interfaces should fail
// Types of property `m1` are incompatible.
interface A {
  m1(): void;
}

interface B extends A {
  m2(): void;
}

interface C extends B {
  m1(n: u8): void;
}
// implementing a different field type with compatible types should pass
interface A {
  a: A;
  b: A | null;
}

class B extends Jig implements A {
  a: B;
  b: B | null;

  constructor(a: B, b: B) {
    super()
    this.a = a
    this.b = b
  }
}

lebrunel avatar Jan 16 '24 17:01 lebrunel