Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
CountBleck wants to merge2 commits intoAssemblyScript:main
base:main
Choose a base branch
Loading
fromCountBleck:interface-multiple-extension

Conversation

CountBleck
Copy link
Member

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

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

@CountBleckCountBleckforce-pushed theinterface-multiple-extension branch fromeb15e88 tof166f39CompareJune 21, 2023 05:42
@CountBleckCountBleck requested a review fromdcodeIOJune 21, 2023 06:09
@CountBleckCountBleck requested review fromdcodeIO and removed request fordcodeIOJune 30, 2023 03:45
@dcodeIOdcodeIO self-assigned thisJun 30, 2023
@CountBleck
Copy link
MemberAuthor

@dcodeIO How does it look?

@CountBleckCountBleckforce-pushed theinterface-multiple-extension branch 4 times, most recently from65ac5ee toca007deCompareJuly 23, 2023 17:43
@CountBleck
Copy link
MemberAuthor

Whoops, needed to rebase. It should work now.

@CountBleckCountBleckforce-pushed theinterface-multiple-extension branch fromca007de to94a2c28CompareJuly 24, 2023 06:18
@CountBleckCountBleck requested review fromdcodeIO and removed request fordcodeIOOctober 2, 2023 14:29
@CountBleckCountBleckforce-pushed theinterface-multiple-extension branch from94a2c28 tof0cd0daCompareJanuary 15, 2024 05:03
@HerrCai0907
Copy link
Member

HerrCai0907 commentedJan 15, 2024
edited
Loading

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.

@CountBleck
Copy link
MemberAuthor

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.

@HerrCai0907
Copy link
Member

interfaceA{f():void;}interfaceB{f():i32;}interfaceCextendsA,B{}classDimplementsC{f():void{}}

For this case, TS will diagnoseInterface '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.

@CountBleck
Copy link
MemberAuthor

CountBleck commentedJan 16, 2024
edited
Loading

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.

@HerrCai0907
Copy link
Member

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.

@CountBleck
Copy link
MemberAuthor

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.x andy are the conflicting properties. Forx,B comes first, so there are errors forB andC, as well asB andD. Fory,C comes first, so there are errors forC andD, as well asC andE.

HerrCai0907 reacted with thumbs up emoji

@HerrCai0907
Copy link
Member

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

@CountBleck
Copy link
MemberAuthor

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

@lebrunel
Copy link

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?

@CountBleck
Copy link
MemberAuthor

@lebrunel that would be great, thanks!

@lebrunel
Copy link

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.
@CountBleckCountBleckforce-pushed theinterface-multiple-extension branch fromf0cd0da to5abcad8CompareJune 1, 2024 20:00
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@HerrCai0907HerrCai0907HerrCai0907 left review comments

@dcodeIOdcodeIOAwaiting requested review from dcodeIO

Assignees

@dcodeIOdcodeIO

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@CountBleck@HerrCai0907@lebrunel@dcodeIO

[8]ページ先頭

©2009-2025 Movatter.jp