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

Sub-optimal type parameter inference with strictFunctionTypes enabled #52111

Closed
jakebailey opened this issue Jan 5, 2023 · 3 comments · Fixed by #52123
Closed

Sub-optimal type parameter inference with strictFunctionTypes enabled #52111

jakebailey opened this issue Jan 5, 2023 · 3 comments · Fixed by #52123
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@jakebailey
Copy link
Member

These are cases I've extracted from #49929; they are cases where if you don't specify the type parameters yourself, the inference isn't good.

This is potentially a full duplicate of #49924, however, the fix suggested in that issue does not work properly in our codebase. To test these, it may be easiest to just clone #49929 with the explicit type parameters removed and see what works and what doesn't.

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; }
interface Identifier extends Expression, Declaration { kind: SyntaxKind.Identifier; }
interface CaseClause extends Node { kind: SyntaxKind.CaseClause; }
interface FunctionDeclaration extends Declaration { kind: SyntaxKind.FunctionDeclaration; }

type HasLocals = Block | FunctionDeclaration;
declare function canHaveLocals(node: Node): node is HasLocals;

declare function assertNode<T extends Node, U extends T>(node: T | undefined, test: (node: T) => node is U): asserts node is U;
declare function assertNode(node: Node | undefined, test: ((node: Node) => boolean) | undefined): void;

function foo(node: FunctionDeclaration | CaseClause) {
    assertNode(node, canHaveLocals)

    node;
    // ^?

    assertNode<Node, HasLocals>(node, canHaveLocals)

    node;
    // ^?
}


declare function isExpression(node: Node): node is Expression;

declare function cast<TOut extends TIn, TIn = any>(value: TIn | undefined, test: (value: TIn) => value is TOut): TOut;

function bar(node: Identifier | FunctionDeclaration) {
    const a = cast(node, isExpression);
    //    ^?

    const b = cast<Expression>(node, isExpression);
    //    ^?
}
Output
"use strict";
var SyntaxKind;
(function (SyntaxKind) {
    SyntaxKind[SyntaxKind["Block"] = 0] = "Block";
    SyntaxKind[SyntaxKind["Identifier"] = 1] = "Identifier";
    SyntaxKind[SyntaxKind["CaseClause"] = 2] = "CaseClause";
    SyntaxKind[SyntaxKind["FunctionExpression"] = 3] = "FunctionExpression";
    SyntaxKind[SyntaxKind["FunctionDeclaration"] = 4] = "FunctionDeclaration";
})(SyntaxKind || (SyntaxKind = {}));
function foo(node) {
    assertNode(node, canHaveLocals);
    node;
    // ^?
    assertNode(node, canHaveLocals);
    node;
    // ^?
}
function bar(node) {
    const a = cast(node, isExpression);
    //    ^?
    const b = cast(node, isExpression);
    //    ^?
}
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

@ahejlsberg
Copy link
Member

I think everything works if you slightly tweak my suggestion here to instead introduce an overload with three type parameters to handle cases where the input and output types share a supertype but aren't otherwise related:

declare function assertNode<T extends Node, U extends T>(node: T | undefined, test: (node: T) => node is U): asserts node is U;
declare function assertNode<T extends N, U extends N, N extends Node>(node: T | undefined, test: (node: N) => node is U): asserts node is U;

declare function cast<TOut extends TIn, TIn = any>(value: TIn | undefined, test: (value: TIn) => value is TOut): TOut;
declare function cast<TOut extends T, TIn extends T, T>(value: TIn | undefined, test: (value: T) => value is TOut): TOut;

Here's a modified example where everything appears to work as expected.

@jakebailey
Copy link
Member Author

jakebailey commented Jan 5, 2023

This is the workaround I mentioned that I tried and reverted; if you look at that playground, you'll see the assertion function has an error on its return because it doesn't like the relationship of the input and output.

The change in cast also has the unfortunate side effect of letting you cast anything to anything else, since it'll happily choose unknown/any as a supertype:

declare function isNumber(x: unknown): x is number;

declare const nodeArray: NodeArray<Node>;

const x = cast(nodeArray, isNumber);
//    ^? number

Which is "fine" because it'll just throw, but does mean you can't know if you're attempting to cast to a totally unrelated type.

@ahejlsberg ahejlsberg self-assigned this Jan 6, 2023
@ahejlsberg ahejlsberg added the Bug A bug in TypeScript label Jan 6, 2023
@ahejlsberg ahejlsberg added this to the TypeScript 5.0.0 milestone Jan 6, 2023
@ahejlsberg
Copy link
Member

First a simplified example:

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

declare function cast<T, U extends T>(x: T, test: (x: T) => x is U): U;

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

function f1(a: A, b: B) {
    const x1 = cast(a, isC);  // cast<A, C>
    const x2 = cast(b, isC);  // cast<A, C>
}

Currently, the cast(b, isC) call fails because we infer B for both T and U. In cases where we have inferences from both co- and contra-variant positions, we prefer the co-variant inference when it is a subtype of the contra-variant inference (i.e. when it is more specific). In the example we have two inference candidates, A and B, for T, and we choose B because it is more specific than A. However, we also have a requirement that our inference for U must extend our inference for T, but we fail to consider that as we're choosing. In the example, C extends A, but not B, so really A is the only meaningful inference for T.

I'm going to put up a PR that adds this extra consideration to type inference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants