Type is not referred correctly in the while loop

[mozesstumpf]

🔎 Search Terms

incorrect type while loop

🕗 Version & Regression Information

Worked correctly in version 4.2.3.
It doesn't work since 4.3.5.

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.5.4#code/MYGwhgzhAEDCAWBLEATaBTAHgF3QOxRgAkAVAWQBkBREdAW322gG8AoASAHN0mAHMAE6MAFAEoWHdsAD2eCE2bR+QvNhr1G0AL7QAvNGxIIAblaTEAM2jCAhMOWN1DVdERzsYPMHTSrABUFGUXE2djChbABXATxoPEiQEFMwrTNwnmjYh1UnRmTU1NZQSBgAlSYsXAJicmpaZwUObj5A1TEJMJl3FiVWtXrNHX1DRBM09ktrO2z+jRc3eU9vX2gAcQFPFDKgkMl2CMy4hKTJQvSomN7y3NV81kLiqDWNgm2XSvxCaFJKG8auHhXES7TqyeQ9GZ-bR6AxGUzmKy2ex9KELDxeHxWdboMDYdabN7YYIdc6HeKJZLsM77DKXSEDW4cApmR4wbG4-GvPoYHCfGq-Bn-ZpAtogmkXWLkk5U+4syLyaR0P4QAB0KHQFjc6GEACJgABaYBIVA6gA0cGNKFEpmA8uwiuVao1Wt1BpmZughOtRTtDsFqvVmrw2r1+s4LxQ+vd5s5Wz63ttCqV-qdQZDBvDOOwYYjUb6HvZeIjXvh2AAnrx0J6Uf6YYXY4ToAAfZ4E7ktwnwrrgo3IND6YMAdwtfbE8NoTFtAnKhIAXNXrrWW1KYb3UCqZvDB8aq8IpzP4x1u9JaCqQNJOHvogfytboAB6e-QCDwaQJNAAIyr9eL7dbXPKZsF0YNJ90YRt9DA1RCQ3PpTFSIA

💻 Code

class Child extends HTMLElement {
	get parent() {
		const { parentElement } = this;

		if (!(parentElement instanceof Parent)) {
			return null;
		}

		return parentElement;
	}
}

class Parent extends HTMLElement {
	get parent() {
		const { parentElement } = this;

		if (!(parentElement instanceof GrandParent)) {
			return null;
		}

		return parentElement;
	}
}

class GrandParent extends HTMLElement {
	get parent() {
		const { parentElement } = this;

		if (!(parentElement instanceof GreatGrandParent)) {
			return null;
		}

		return parentElement;
	}
}

class GreatGrandParent extends HTMLElement {
	get parent() {
		return null;
	}
}

// irrelevant code
customElements.define("c-child", Child);
customElements.define("c-parent", Parent);
customElements.define("c-grand-parent", GrandParent);
customElements.define("c-great-grand-parent", GreatGrandParent);

type ParentElements = GreatGrandParent | GrandParent | Parent;

const child = new Child();

let currentParent: ParentElements | null = child.parent;

while (currentParent) {
	console.log(currentParent); // should be GreatGrandParent | GrandParent | Parent

	currentParent = currentParent.parent;
}

🙁 Actual behavior

The curerntParent's type is not referred correctly. It became Parent | GrandParent.

🙂 Expected behavior

The currentParent's type should be Parent | GrandParent | GreatGrandParent.

Additional information about the issue

No response

[Andarist]

With removed redundant pieces (TS playground):

declare class Child {
  parent: Parent | null;
}

declare class Parent {
  parent: GrandParent | null;
}

declare class GrandParent {
  parent: GreatGrandParent | null;
}

declare class GreatGrandParent {
  parent: null;
}

declare const child: Child;

let currentParent: GreatGrandParent | GrandParent | Parent | null = child.parent;

// // the bug doesn't happen with this:
// declare let currentParent: GreatGrandParent | GrandParent | Parent | null;

while (currentParent) {
  // actual: GrandParent | Parent
  // expected: GreatGrandParent | GrandParent | Parent
  currentParent;

  currentParent = currentParent.parent;

  currentParent; // Parent | GrandParent | GreatGrandParent | null
}

[jcalz]

Isn't this just because GreatGrandParent is a subtype of Child, Parent, and Grandparent so it can get reduced out of unions with them? If you add more structure to Parent and Grandparent then the problem goes away, right?

[Andarist]

Same thing happens even if we make all of those classes nominal: TS playground

[Andarist]

1. TS starts to compute currentParent in while (currentParent), this has two antecedents
2. the first antecedent is the let currentParent declaration. Since the declared type is a union the assigned value is used as the initial type. Thanks to this antecedent TS gets Parent | null as the potential type
3. then it proceeds to the second antecedent, the assignment within the loop. Since the declared type of this variable is a union it tries to get the assigned type using getInitialOrAssignedType (that is meant to be passed to getAssignmentReducedType) here
4. this leads to checking what currentParent (since it needs to read its .parent) might be and that depends on the same flow loop that we are already in the middle of processing (the compiler starts to compute it at step 1).
5. in such a case, the compiler returns an incomplete flow type that contains what it got so far for this variable. It happens here.
6. since what we got so far as the potential currentParent is Parent | null we proceed to read .parent from it. This results in GranParent | null
7. the compiler gets back with this result to step 2 and adds that to the set of flow types for currentParent
8. the compiler visited all of the antecedents (just 2 of them)... and its creates a union of them: Parent | GrandParent | null. Later on, it passes that to checkTruthinessExpression (called by checkWhileStatement) so the null gets discarded.

This is 100% a bug but one that I don't know how to fix. I hope the above will help somebody to do it 😉

[Andarist]

@RyanCavanaugh shouldn't this really be classified as a bug if the CFA incorrectly drops one of the possible types? It's not that this isn't precise enough - it's plain incorrect. Take a look at this extension of the problem:

declare class Child {
  parent: Parent | null;
}

declare class Parent {
  parent: GrandParent | null;
  prop: number
}

declare class GrandParent {
  parent: GreatGrandParent | null;
  prop: number
}

declare class GreatGrandParent {
  parent: null;
}

declare const child: Child;

let currentParent: GreatGrandParent | GrandParent | Parent | null = child.parent;

while (currentParent) {
  // since `currentParent` doesn't include `GreatGrandParent` (even though it should),
  // this is allowed but it shouldn't!
  const num: number = currentParent.prop 

  currentParent = currentParent.parent;
}

[milkcask]

Last known good: 4.3.0-dev.20210319
Doesn't work since: 4.3.0-dev.20210322

Related #43183

[milkcask]

Some progress note.

I can confirm the change in #43183 was the root cause.

checkIdentifier is called with CheckMode.TypeOnly to get the type of second antecedent

Consider:

while (currentParent) {
  // actual: GrandParent | Parent
  // expected: GreatGrandParent | GrandParent | Parent
  currentParent;          <<<<< this line

  currentParent = currentParent.parent;

  currentParent; // Parent | GrandParent | GreatGrandParent | null
}

Call stack:

checkExpression()
  checkExpressionWorker()
    checkIdentifier()
      getFlowTypeOfReference()
        getTypeAtFlowNode()
          getTypeAtFlowLoopLabel() // in a loop, proceeds to the second antecedent
            getTypeAtFlowNode()
              getTypeAtFlowAssignment()
                getInitialOrAssignedType() // It actually goes on to the initialType branch. And interestingly, if we patch it to always return assignedType, it works as expected
                  getInitialType()
                    getInitialTypeOfVariableDeclaration()
                      getTypeOfInitializer()
                        getTypeOfExpression()
                          checkExpression(..., CheckMode.TypeOnly)
                            checkExpressionWorker(..., CheckMode.TypeOnly)
                              checkIdentifier(..., CheckMode.TypeOnly)
                                getNarrowedTypeOfSymbol()
                                  ...

I wonder where should we apply the fix. And what might be the edge cases.

[nmain]

The behavior before #43183 isn't significantly better, as it still fails if you add one more layer:

Playground

type All = A1 | A2 | A3 | A4;

class A1 {
	nom = "a1" as const
	next() {
		return new A2;
	}
}

class A2 {
	nom = "a2" as const
	next() {
		return new A3;
	}
}

class A3 {
	nom = "a3" as const
	next() {
		return new A4;
	}
}

class A4 {
	nom = "a4" as const
	next() {
		return null;
	}
}

function foo(a: A1) {
	for (let p: All | null = a; p != null; p = p.next()) {
		// In 4.2.3, `p` is A1 | A2 | A3
		// In 5.7.2, `p` is A1 | A2
		p;
		if (p.nom === "a4") {
			// `p` is never in all versions
			p;
			console.log("This is hit at runtime");
		}
	}
}

foo(new A1);

[milkcask]

It seems we can't bring back the behaviour before #43183. I'm thinking we might need to keep track of what the blocks/flows do with CheckMode. (Or perhaps a new flag?) Also, nmain's observation is interesting as let p: All | null is explicitly All | null, but the checker somehow determines it's better to infer it.