Skip to content

feat: add type narrow to support better type check #2352

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

Closed

Conversation

HerrCai0907
Copy link
Member

No description provided.

@HerrCai0907 HerrCai0907 force-pushed the feat/add-type-narrow branch from 1795f6f to df38498 Compare June 30, 2022 15:51
@dcodeIO
Copy link
Member

dcodeIO commented Jun 30, 2022

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?

@HerrCai0907
Copy link
Member Author

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?

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.

@HerrCai0907
Copy link
Member Author

HerrCai0907 commented Jul 1, 2022

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();
}

@MaxGraey
Copy link
Member

MaxGraey commented Jul 1, 2022

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

@dcodeIO
Copy link
Member

dcodeIO commented Jul 1, 2022

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
}

@HerrCai0907 HerrCai0907 marked this pull request as ready for review July 5, 2022 05:22
@MaxGraey
Copy link
Member

MaxGraey commented Aug 5, 2022

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

@MaxGraey MaxGraey requested a review from dcodeIO August 6, 2022 13:01
@HerrCai0907
Copy link
Member Author

@dcodeIO Could this feature merge?

@@ -2994,6 +2991,8 @@ export class Compiler extends DiagnosticEmitter {
);
pendingElements.delete(dummy);
flow.freeScopedDummyLocal(name);

initType = this.currentType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to compiling with Constraincts.CONV_IMPLICIT, I think initType is always the same as type here. Sure that the additional initType variable is needed?

Copy link
Member Author

@HerrCai0907 HerrCai0907 Aug 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider this testcase. Actually initType and type are not always the same in nullable

export function testInit(a: Ref): void {
  let c: Ref | null = requireNonNull(a);
  if (isNullable(c)) ERROR("should be non-nullable");
}

@MaxGraey MaxGraey requested a review from dcodeIO August 11, 2022 12:06
@HerrCai0907
Copy link
Member Author

@dcodeIO Could you kindly review this PR?

@dcodeIO
Copy link
Member

dcodeIO commented Sep 25, 2022

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?

@HerrCai0907 HerrCai0907 marked this pull request as draft September 26, 2022 01:37
@HerrCai0907
Copy link
Member Author

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😄

@HerrCai0907 HerrCai0907 deleted the feat/add-type-narrow branch March 2, 2023 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants