assemblyscript
assemblyscript copied to clipboard
AssemblyScript
feat: add type narrow to support better type check
When I thought about narrowing in the past, the mechanism I had in mind was to add a new Flow#localTypes in addition to Flow#localFlags, that would include narrowed types if available. Checks would then first look if there is a narrowed type in Flow#localTypes and otherwise fall back to the annotated type from before (say through a Flow#getLocalType(index) helper). Wondering if going this route would have advantages over a separate concept as proposed here?
When I thought about narrowing in the past, the mechanism I had in mind was to add a new
Flow#localTypesin addition toFlow#localFlags, that would include narrowed types if available. Checks would then first look if there is a narrowed type inFlow#localTypesand otherwise fall back to the annotated type from before (say through aFlow#getLocalType(index)helper). Wondering if going this route would have advantages over a separate concept as proposed here?
I have not found localTypes in current flow. However it thinks like a good idea that we can reuse current inherit process. I can try to add this feature.
By the way, we should not support narrow type for variant like a.b.
Although TS support this grammar, but it will cause something strange during running.
class A {}
class B extends A {
foo(): void {}
}
class C {
v: A = new A();
}
let c = new C();
let d = c;
c.v = new B();
if (c.v instanceof B) {
d.v = new A(); // c.v has changed, but compiler don't know
c.v.foo();
}
d.v = new A(); // c.v has changed, but compiler don't know
well. that's problem on TS. I guess they are fixed this later. I see no reason to refuse it if we can handle this case correctly
Quite similar to why nullability checks are limited to locals. Also applies to narrowing if we don't want to run into unsoundness.
...
let cv = c.v;
if (cv instanceof B) {
// works
}
Still thinking whether the nullable flag should be combined with this, so the following can become one
thenFlow.inheritNonnullIfTrue(condExpr);
thenFlow.inheritNarrowedTypeIfTrue(condExpr);
Right now this looks like unnecessary duplication of basically the same concept (flag before, non-nullable narrowed type after).
@dcodeIO Nonnull check has been merged into type narrow logic. Could you kindly review it?
Btw such flow mechanics is quite typical:
var thenFlow = flow.fork();
this.currentFlow = thenFlow;
thenFlow.inheritNarrowedTypeIfTrue(condExpr);
I'm wondering could we add some method for Flow which include this routine?
class Flow {
...
toBranch(condExpr: Expression, ifTrue: bool): Flow {
var other = this.fork();
if (ifTrue) {
other.inheritNarrowedTypeIfTrue(condExpr);
} else {
other.inheritNarrowedTypeIfFalse(condExpr);
}
return other;
}
}
And later simplify to:
this.currentFlow = flow.toBranch(condExpr, true);
Not sure about naming. Probably you could call all of this better
I'm having a hard time reviewing the PR, and would like to suggest to split it up a little. As a start, there has been the refactoring from fork + X to forkTrueBranch and forkFalseBranch. Can you make this a separate PR, so we have this in already as it seems useful independently? There also appear to be a few fixes here, which would be good to merge independently as well? Unless I missed another refactoring, afterwards we'd be left with just the narrowing changes.
From there on it gets a little tricky, as I am not confident about having an out-of-line src/narrow.ts. Typically, what we do there is to place utility data structures into util/, which here could be an util/narrowing.ts with the specialized data structure. However, the data structure now absorbs the logic from the inheritXY methods that previously lived in Flow, but I'd like to keep the logic in Flow. Suggesting to do the other ones first, so I can look at this again separately and see. Would this be OK for you?
I'm having a hard time reviewing the PR, and would like to suggest to split it up a little.
Yes, I will split it into several PR. But it needs some time because I am preparing my graduation thesis now😄