express.d.ts simple usage broken in latest bits

[billti]

A basic Express 4 sample has errors with the latest bits. It appears to be a regression in TypeScript 2.1.

Below is how far I’ve managed to reduce the code into a standalone repro before heading home (from the definition currently in @types/express).

If I compile with the current bits in the “release-2.0.5” branch there is no error. This is specific to what is currently in “release-2.1”.

severity: 'Error'
message: 'Argument of type '(err: any, req: Response, res: NextFunction, next: any) => void' is not assignable to parameter of type 'PathParams'.
  Type '(err: any, req: Response, res: NextFunction, next: any) => void' is not assignable to type '(string | RegExp)[]'.
    Property 'push' is missing in type '(err: any, req: Response, res: NextFunction, next: any) => void'.'

var app: myApp;

app.use((err: any, req, res, next) => { return; }); // <--- error on this call to 'use'


interface myApp {
    use: IRouterHandler<this> & IRouterMatcher<this>;
}

    interface IRouterHandler<T> {
        (...handlers: RequestHandler[]): T;
        (...handlers: RequestHandlerParams[]): T;
    }

    interface IRouterMatcher<T> {
        (path: PathParams, ...handlers: RequestHandler[]): T;
        (path: PathParams, ...handlers: RequestHandlerParams[]): T;
    }

    type PathParams = string | RegExp | (string | RegExp)[];
    type RequestHandlerParams = RequestHandler | ErrorRequestHandler | (RequestHandler | ErrorRequestHandler)[];

    interface RequestHandler {
        (req: Request, res: Response, next: NextFunction): any;
    }

    interface ErrorRequestHandler {
        (err: any, req: Request, res: Response, next: NextFunction): any;
    }

    interface Request {
        method: string;
    }

    interface Response {
        statusCode: number;
    }

    interface NextFunction {
        (err?: any): void;
    }

[vladima]

was broken in 8094b2c

[HerringtonDarkholme]

A even slimmer repro:

var use: Overload;
use((req, res) => {}); // <--- error on this call to 'use'

interface Overload {
    (handler: Arity1): void;
    (handler: Arity2): void;
}

interface Arity1 {
    (req: string): void;
}

interface Arity2 {
    (req: number, res: string): void;
}

The problem is unannotated parameter inference is not changed after first call of checkExpression.
Later when choosing overload, the parameter type mismatches other signatures.

[HerringtonDarkholme]

The problem here is in choosing which overload signature for contextual typing.

function checkFunctionExpressionOrObjectLiteralMethod(node: FunctionExpression | MethodDeclaration, contextualMapper?: TypeMapper): Type {
  // omitted....

  if (mightFixTypeParameters || !(links.flags & NodeCheckFlags.ContextChecked)) {
    const contextualSignature = getContextualSignature(node);
    const contextChecked = !!(links.flags & NodeCheckFlags.ContextChecked);
    if (mightFixTypeParameters || !contextChecked) {
      links.flags |= NodeCheckFlags.ContextChecked;
      if (contextualSignature) {
        const signature = getSignaturesOfType(type, SignatureKind.Call)[0];  // <- problem here
        if (contextSensitive) {
          assignContextualParameterTypes(signature, contextualSignature, contextualMapper || identityMapper);
        }
  // omitted...
}

The signature is the first one in overloaded function. Before 8094b2c contextSensitive is true only when no parameter is annotated, so the overload will always succeed. It's no longer true after partial annotation inference...

@ahejlsberg @sandersn Any idea about fixing this?

I have thought about 1. give up contextual typing if in an overloaded call, or 2. reassign contextual parameter types in chooseOverload.

[sandersn]

@HerringtonDarkholme I'm pretty sure that contextual typing does not apply when the contextual type is an overloaded call signature. So probably (1) is correct.

Your minified repro illustrates this -- (req, res) => {} would have been contextually sensitive before 8094b2c but would not actually get a contextual type because Overload is overloaded.

[HerringtonDarkholme]

Yes, (req, res) => {} line is also an error in 2.0.6. Thank @sandersn for pointing out.

[RyanCavanaugh]

We shouldn't be applying contextual types to a function expression with greater parameter arity than the source overload. It's not a complete fix, but prevents breakage in cases where doing so is clearly wrong.

[mhegazy]

@vladima can you take a look.

[HerringtonDarkholme]

I just want to confirm, should this code break in TS 2.0.6? It suffers from the same problem here.

var use: Overload;
use(a => {}); // <--- error on this call to 'use'

interface Overload {
    (handler: ReturnString): void;
    (handler: ReturnVoid): void;
}

interface ReturnString {
    (a: string): string;
}

interface ReturnVoid {
    (a: number): void;
}

[sandersn]

Yes, it turns out the bug has been there a long time. @RyanCavanaugh thinks about 1.1 or so. We just noticed it now because a lot more things are contextually typed than before.

[mhegazy]

@HerringtonDarkholme the behavior with your last sample is expected. There is no way at runtime for the caller to know what the type of the call back argument is, so what would use call this callback with? is it a number or a string? there is no way to really tell; thus this declaration is rather misleading..

What you would want to say is, and use may call this callback it with a number, or a string. thus the right way to declare that is to say you expect a function that can handle "both" number and string arguments:

interface Overload {
    (handler: AcceptsNumberOrString): void;
}

interface AcceptsNumberOrString {
    (a: string): string;
    (a: number): void;
}

and you would implement it with a function that has two overloads.