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

Array filter() by element constructor returns never[] #58987

Closed
iknowcss-invenco opened this issue Jun 24, 2024 · 12 comments
Closed

Array filter() by element constructor returns never[] #58987

iknowcss-invenco opened this issue Jun 24, 2024 · 12 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@iknowcss-invenco
Copy link

πŸ”Ž Search Terms

array never filter

πŸ•— Version & Regression Information

  • This changed between versions 5.4.5 and 5.5.2

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.5.2#code/IYIwzgLgTsDGEAJYBthjAg4gUwHbagEtYBhVdBAbwCgEEAHKAewm3mwBMkndIoBXeEygAKevxDJiCAG7Bk-bAC4EfQrgDmASioBfavuoo0GEj1hRsrMiYTYAHq1wcMOfEVLkMNOrB59BCGEROQVlVWh1bSpaOlV+egIRAAMzXAsrcIASSlDFXWStAG5Y-UM-XkRsZGwAWzwIMBU3AmIbdABtAF0EAF4EDvwAdwQ0jOsvEQByYCmtLpKjf0QAM0JkVksOAFEa+txGvrs9hrAAOjWNpJFquoadXoA+Y7uDs4qAoSg+3v6xywmJmK1Gol02nF2r0aHQADF0znlsGcggBlSKaETAoA

πŸ’» Code

abstract class GenericClass {
  protected constructor(public value: string) {}
}

class ConcreteClass extends GenericClass {
  constructor(value: string) {
    super(`Concrete: ${value}`);
  }
}

const elements: GenericClass[] = [new ConcreteClass('a')];

const filteredElements = elements.filter((element) => element.constructor === ConcreteClass);

filteredElements[0].value.toString();
//                  ~~~~~ 
// Property 'value' does not exist on type 'never'.

πŸ™ Actual behavior

The return type from the .filter() expression is never[]

πŸ™‚ Expected behavior

The return type from the .filter() expression is GenericClass[]

Additional information about the issue

I searched through the open issues in the days since 5.5.2 were released but I could not find any which matched this case. Apologies if I missed anything. I've tried to reduce this down to the most minimal example that I can.

@fatcerberus
Copy link

fatcerberus commented Jun 24, 2024

Changed in 5.2 5.5 so probably a result of automatic type predicate inference

@whzx5byb
Copy link

whzx5byb commented Jun 24, 2024

The return type of filter callback (element) => element.constructor === ConcreteClass is inferred as element is never in 5.5.2 because of #57465. I would suggest using (element) => element instanceof ConcreteClass instead.

@jcalz

This comment was marked as resolved.

@IllusionMH
Copy link
Contributor

Related #16035

@fatcerberus
Copy link

fatcerberus commented Jun 24, 2024

Why are we calling this β€œ5.2”?

Oops, typo and/or me being in too much of a rush. Fixed now.

@iknowcss-invenco
Copy link
Author

I would suggest using (element) => element instanceof ConcreteClass instead.

Yes, I agree this works and is more idiomatic. I will change this in my code to fix the problem. I wanted to report it in case other projects unexpectedly break while trying to do a minor bump. I'm not sure if 5.5.2 fixes a long standing bug or not, but regardless it was a surprise to have a broken build after this minor upgrade. Thanks for your work on it πŸ™

@fatcerberus
Copy link

@whzx5byb Thats weird though because #16035 suggests this condition doesn’t narrow

@whzx5byb
Copy link

@fatcerberus Actually the .constructor narrowing works only when the target is a union type and it must be compared using === (or ==, but not for switch clause) operator. I'm very surprised that it even works for the switch (true) pattern as long as there is a === comparison!

class A {
  a!: number;
}

class B {
  b!: number;
}


function fn(input: A | B) {
  switch (input.constructor) {
    case A:
      input.a // <- not work
  }

  switch (true) {
    case input.constructor === A:
      input.a; // work!?
  }
}

But anyway, in the OP's case the narrowing target is not a union type, and .constructor === comparison will always narrow it to never.

@ahejlsberg
Copy link
Member

ahejlsberg commented Jun 24, 2024

Seems to me the real issue is this:

class GenericClass {
    constructor(public value: string) { }
}

class ConcreteClass extends GenericClass {
    constructor(value: string) {
        super(`Concrete: ${value}`);
    }
}

function foo(obj: GenericClass) {
    if (obj.constructor === ConcreteClass) {
        obj;  // never, Wat?
    }
}

It's obviously possible for obj.constructor to be ConcreteClass since that class is derived from GenericClass. The error that this issue reports is simply a follow-on effect of the incorrect narrowing that gets picked up in an inferred type predicate.

@ahejlsberg
Copy link
Member

It goes all the way back to #32774 that implemented "constructor type guards". There is no logic to deal with derived classes.

@fatcerberus
Copy link

#32774 that implemented "constructor type guards"

Wait, why is #16035 still open then?

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jun 24, 2024
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Design Limitation" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

8 participants