Skip to content

Type parameter on ts.some/ts.find is too narrow when called on union of arrays, works with explicit instantiation #52179

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
jakebailey opened this issue Jan 10, 2023 · 9 comments · Fixed by #52180

Comments

@jakebailey
Copy link
Member

jakebailey commented Jan 10, 2023

This is a follow-up to #52111 and #52123.

In the below code, every works as expected, which is new as of #52123. However, the some function can't be called on the array type at all, even though reasonably there's a top type which works (A), just like it works for every.

interface A           { a: string; }
interface B extends A { b: string; }
interface C extends A { c: string; }

declare function isC(x: A): x is C;

declare function every<T, U extends T>(array: readonly T[], callback: (element: T, index: number) => element is U): array is readonly U[];
declare function every<T, U extends T>(array: readonly T[] | undefined, callback: (element: T, index: number) => element is U): array is readonly U[] | undefined;
declare function every<T>(array: readonly T[] | undefined, callback: (element: T, index: number) => boolean): boolean;

declare function some<T>(array: readonly T[] | undefined): array is readonly T[];
declare function some<T>(array: readonly T[] | undefined, predicate: (value: T) => boolean): boolean;

function foo(array: readonly B[] | readonly C[] | undefined) {
    if (every(array, isC)) {
        array;
        // ^?
    }

    if (some(array)) {
        array;
        // ^?
    }

    if (some(array, isC)) {
        array;
        // ^?
    }
    
    if (some<A>(array)) {
        array;
        // ^?
    }

    if (some<A>(array, isC)) {
        array;
        // ^?
    }
}
Output
"use strict";
function foo(array) {
    if (every(array, isC)) {
        array;
        // ^?
    }
    if (some(array)) {
        array;
        // ^?
    }
    if (some(array, isC)) {
        array;
        // ^?
    }
    if (some(array)) {
        array;
        // ^?
    }
    if (some(array, isC)) {
        array;
        // ^?
    }
}
Compiler Options
{
  "compilerOptions": {
    "strict": true,
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "target": "ES2017",
    "jsx": "react",
    "module": "ESNext",
    "moduleResolution": "node"
  }
}

Playground Link: Provided

@jakebailey
Copy link
Member Author

I will say that the first overload to some could be:

export function some(array: readonly unknown[] | undefined): array is readonly unknown[];

And function the same way (proving the array isn't undefined), but the callback case is the one that errors in the SFT branch.

@jakebailey
Copy link
Member Author

This is another case on the SFT branch: Playground Link

enum SyntaxKind {
    Block,
    Identifier,
    CaseClause,
    FunctionExpression,
    FunctionDeclaration,
}

interface Node { kind: SyntaxKind; }
interface Expression extends Node { _expressionBrand: any; }
interface Declaration extends Node { _declarationBrand: any; }
interface Block extends Node { kind: SyntaxKind.Block; }

declare function find<T, U extends T>(array: readonly T[] | undefined, predicate: (element: T, index: number) => element is U, startIndex?: number): U | undefined;
declare function find<T>(array: readonly T[] | undefined, predicate: (element: T, index: number) => boolean, startIndex?: number): T | undefined;

function foo(array: readonly Declaration[] | readonly Block[] | undefined) {
    const a = find(array, (node) => true); // error!
    //    ^?


    const b = find<Declaration | Block>(array, (node) => true);
    //    ^?

    const c = find<Node>(array, (node) => true);
    //    ^?
}

@jakebailey
Copy link
Member Author

So, this extends past ts.some and more into "array related helpers when passed unions of arrays"; maybe these are the cases that we don't expect to work.

@jakebailey jakebailey changed the title Type parameter on ts.some is too narrow when called on union of arrays, works with explicit instantiation Type parameter on ts.some/ts.find is too narrow when called on union of arrays, works with explicit instantiation Jan 10, 2023
@jakebailey
Copy link
Member Author

@ahejlsberg for interest.

@jakebailey
Copy link
Member Author

jakebailey commented Jan 10, 2023

Then there's this...

// @strict: true

const enum SyntaxKind {
    Modifier,
    Decorator,
}

interface Node {
    kind: SyntaxKind;
}

interface Modifier extends Node { kind: SyntaxKind.Modifier; }
interface Decorator extends Node { kind: SyntaxKind.Decorator; }

declare function isModifier(node: Node): node is Modifier;
declare function isDecorator(node: Node): node is Decorator;

declare function every<T, U extends T>(array: readonly T[], callback: (element: T) => element is U): array is readonly U[];

declare const modifiers: readonly Modifier[] | readonly Decorator[];

function foo() {
    every(modifiers, isModifier); // error? (undesired)
    every(modifiers, isDecorator); // no error (desired)
}

Playground Link

Swap the union order on modifiers and the errors flip. Maybe this is a different issue, as it's related to whatever is "leftmost" being the contravariant inference thanks to our algorithm for choosing a supertype, which is now exposed in this example because the new logic from the PR makes it work for every type in the union but the first one.

@jakebailey
Copy link
Member Author

jakebailey commented Jan 10, 2023

Unbelievably, I figured out what might be a fix for all some of these cases... Will send a PR tonight/tomorrow morning.

@ahejlsberg
Copy link
Member

Regarding the error with find:

function foo(array: readonly Declaration[] | readonly Block[] | undefined) {
    const a = find(array, (node) => true); // error!
    //    ^?
    const b = find<Declaration | Block>(array, (node) => true);
    //    ^?
    const c = find<Node>(array, (node) => true);
    //    ^?
}

It's not clear there's anything we can do here. Neither of the co-variant inferences we make for T is a supertype of the other, and there are no contra-variant inferences to help us out. The type readonly Declaration[] | readonly Block[] clearly indicates that we don't want to mix the two element types and therefore we don't infer Declaration | Block. This is a long standing principle that I don't think we want to mess with.

@jakebailey
Copy link
Member Author

Yeah, I had found some more examples that already existed in the codebase pre-SFT like this.

I'm going to retest SFT with #52180 as LKG and see how far we get; I'm guessing it will turn out okay.

@fatcerberus
Copy link

fatcerberus commented Jan 11, 2023

find feels like one of those cases like with Array#includes where you really want to allow bivariant inputs (#36352, #26255) but there's currently no mechanism in the type system to do so. 😿

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants