Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork669
Support interfaces having multiple base interfaces#2711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Support interfaces having multiple base interfaces#2711
Conversation
eb15e88
tof166f39
Compare@dcodeIO How does it look? |
65ac5ee
toca007de
CompareWhoops, needed to rebase. It should work now. |
ca007de
to94a2c28
Compare94a2c28
tof0cd0da
CompareHerrCai0907 commentedJan 15, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Could you explain more about the target behavior of extended interfaces and how to handle the conflict between different base interfaces. |
Uh oh!
There was an error while loading.Please reload this page.
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. |
interfaceA{f():void;}interfaceB{f():i32;}interfaceCextendsA,B{}classDimplementsC{f():void{}} For this case, TS will diagnose |
CountBleck commentedJan 16, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
By the way, interfaceA{m:Object|null}interfaceB{m:Object}interfaceXextendsA,B{// m: Object} 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. |
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'san example. |
So this PR still missing combination check when inheriting from difference base interfaces, right? |
Both of those things I described are part of that diagnostic, which I haven't implemented yet. |
lebrunel commentedJan 16, 2024
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 that would be great, thanks! |
lebrunel commentedJan 16, 2024
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.interfaceA{m():void;}classBimplementsA{m(n:u8):void{}} // implementing a different method signature from nested interfaces should fail// Types of property `m1` are incompatible.interfaceA{m1():void;}interfaceBextendsA{m2():void;}interfaceCextendsB{m3():void;}classDimplementsC{m1(n:u8):void{}m2():void{}m3():void{}} // implementing a different method signature with compatible types should passinterfaceA{m1():A;}interfaceBextendsA{m2():B;}classCimplementsA{m1():C{returnthis};}classDimplementsB{m1():D{returnthis};m2():B{returnthis};} // extending a different method signature should fail// Types of property `m` are incompatible.interfaceA{m():void;}interfaceBextendsA{m(n:u8):void;} // extending a different method signature from nested interfaces should fail// Types of property `m1` are incompatible.interfaceA{m1():void;}interfaceBextendsA{m2():void;}interfaceCextendsB{m1(n:u8):void;} // implementing a different field type with compatible types should passinterfaceA{a:A;b:A|null;}classBextendsJigimplementsA{a:B;b:B|null;constructor(a:B,b:B){super()this.a=athis.b=b}} |
This is a feature in TypeScript, and I didn't see much of a technicalreason to disallow it. By changing interface extension such thatimplementsTypes and interfacePrototypes are used for base interfacesinstead of extendsType and basePrototype in InterfacePrototype andInterface respectively, and by modifying the parser, existing codedoesn't seem to break, and multiple base interfaces are possible (if notworking already).There was also a small change to the instanceof helper generation, wherearrays are now used instead of Sets, since I needed to filter forinterfaces, and Set_values was used on the constructed Set regardless.However, the change also modified the order of instanceof checks as seenin instanceof.debug.wat. The instanceof.release.wat file underwent moredrastic changes, but it still appears to work anyway.
Without inspecting the resulting WAT, and instead concluding based onthe test compiling successfully and executing without any errors,everything seems to work properly.
f0cd0da
to5abcad8
Compare
Changes proposed in this pull request:
⯈Adding support for an interface extending multiple interfaces.