Skip to content
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

Type is not referred correctly in the while loop #59715

Open
mozesstumpf opened this issue Aug 22, 2024 · 7 comments
Open

Type is not referred correctly in the while loop #59715

mozesstumpf opened this issue Aug 22, 2024 · 7 comments
Labels
Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@mozesstumpf
Copy link

mozesstumpf commented Aug 22, 2024

πŸ”Ž 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
Copy link
Contributor

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
Copy link
Contributor

jcalz commented Aug 22, 2024

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
Copy link
Contributor

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

@Andarist
Copy link
Contributor

  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 πŸ˜‰

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Aug 26, 2024
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 26, 2024
@Andarist
Copy link
Contributor

@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;
}

@RyanCavanaugh RyanCavanaugh added Bug A bug in TypeScript and removed Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases labels Aug 27, 2024
@milkcask
Copy link

milkcask commented Oct 5, 2024

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

Related #43183

@milkcask
Copy link

milkcask commented Oct 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

5 participants