-
-
Notifications
You must be signed in to change notification settings - Fork 670
Implement calls to 'super()' #445
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…tructor when compiling them
Hmm, I'm not sure about first statement call rule. It make sense only if constructor(a: i32) {
var arg = a + 1;
super(arg);
} |
Yeah, there are the ALLOCATES and CONDITIONALLY_ALLOCATES flags in flows for that. Will probably need a CALLS_SUPER one as well then to do it this way. TS also has
Maybe we could use "'super' must be called before accessing 'this' in the constructor of a derived class.": {
"category": "Error",
"code": 17009
}, |
Yeah |
Seems constructor(a: i32) {
var arg = a + 1;
super(arg);
} is rejected by TS, but now legalized in AS with the flow changes above. Still needs a check when doing |
Hmm, really? That's interesting... |
Yeah, but PR for fixing this in TS already exists: microsoft/TypeScript#29374 |
Yeah, I remember that this limitation annoyed me in the past. Especially given the dynamic nature of JavaScript this seems like an arbitrary restriction (not directly related to AS, where such a restriction might actually make sense). |
As I know this legal in any OOP typed languages may be except C#. It seems typescript just missing this ability. But this should be fix soon |
One thing that's still particularly off is that derived classes currently inherit their super class's fields, and thus initialize super fields in each derived constructor, which is odd. Instead, each constructor should initialize its own class's fields right after bubbling up the entire prototype chain, calling all the overloaded constructors sequentially. One reason why this doesn't yet work is that classes that don't have an explicit constructor also don't have a wasm function for it (their field initialization is instead inlined), which we should change by generating actual constructor functions for them that we can call. |
Makes sense. But it look like not simple. May be better leave this for other PR? |
Sure, makes sense to leave that for a separate PR. One example what's not possible until then: class A {
a: i32 = 1;
constructor() {
assert(this.a == 1); // == 0, is uninitialized, and remains so
}
}
class B extends A {
a: i32 = 3;
constructor() {
super();
}
}
new B(); This is currently prevented by rejecting duplicate field names. |
Similarly: class A {
a: i32 = 1;
constructor() {}
}
class B extends A {
b: i32 = 2;
}
var b = new B();
assert(b.b == 2); // == 0, uninitialized due to A#constructor being used, with no B#constructor Not directly related to |
Merging, continuing in a separate PR. |
New to the assemblyscript project, and coming here because I hit this bug: #245 Does this mean that OO stuff in general isn't supported by assembly script at the moment? |
@robrohan Basic OO supports except virtual methods. Issue which you mentioned is not yet fixed. If you add some params to basic constructor it would work |
Passes a simple test and implements quite a few related checks. Still possible that not every single edge case is covered, but having this now and fixing any bugs as we go appears reasonable, considering that class support is still in the works anyway.