Fix contextually typed parameter issues

[ahejlsberg]

This PR fixes multiple issues related to contextually typed parameters:

• Previously we'd consider a parameter of a function expression, arrow function, or object literal method to be contextually typed if any contextual type could be found for the function. We now consider a parameter to be contextually typed only if the contextual type of the function provides a call signature and that call signature has a parameter in the corresponding position.
• Previously, in checkFunctionExpressionOrObjectLiteralMethod, we'd only assign types to contextually sensitive parameters if a contextual type was found. We now always assign types to contextually sensitive parameters, thus also recording the fact that there was no contextual type. This matters because different phases of overload resolution may change the contextual type, causing inconsistent results depending on when the type of a parameter is resolved. For example, we might see different types in quick info vs. batch compilation.

Some examples:

const f1 = (x = 1) => 0;  // x: number
const f2: any = (x = 1) => 0;  // x: number (previously implicit any error)
const f3: unknown = (x = 1) => 0;  // x: number (previously implicit any error)
const f4: Function = (x = 1) => 0;  // x: number (previously implicit any error)
const f5: (...args: any[]) => any = (x = 1) => 0;  // x: any
const f6: () => any = (x = 1) => 0;  // x: number (previously any)
const f7: () => any = (x?) => 0;  // Implicit any error (previously any and no error)
const f8: () => any = (...x) => 0;  // x: []

declare function memoize<F extends Function>(func: F): F;

// Type (x: number, y?: number) => number
const add = (x: number, y = 0): number => x + y;

// Type (x: number, y?: number) => number
const memoizedAdd1 = memoize(add);

// Type (x: number, y?: number) => number (previously implicit any error on y)
const memoizedAdd2 = memoize((x: number, y = 0): number => x + y);

Related to this PR, I discovered an issue caused by #25937 and #31574:

declare function foo<T>(obj: T, settings: (row: T) => { value: string, func?: Function }): void;

foo(new Error(),
    o => ({
        value: o.name,
        func: x => 'foo'
    })
);

Above, hovering over the call to foo correctly shows T was inferred as Error, but hovering over o shows type any because of a circularity error (which isn't displayed by quick info). The issue is caused by code in #31574 that I have now removed. Furthermore, I have restricted the functionality introduced by #25937 to only kick in for parameterless arrow functions (because the functionality is safe only so long as there are no parameters that can be referenced in the return type).

Fixes #28816.
Fixes #30840.
Fixes #36052.

[ahejlsberg]

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 694f9e5. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 694f9e5. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 694f9e5. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

[ahejlsberg]

RWC tests look good (two implicit any errors disappeared in favor of actual types). DT tests are clean. Community tests show a new error in webpack which is correct (type annotations appear to be wrong).

[weswigham]

@typescript-bot pack this

[typescript-bot]

Heya @weswigham, I've started to run the tarball bundle task on this PR at 694f9e5. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/62242/artifacts?artifactName=tgz&fileId=86B48641ABD3179462DCEF602DCCDE9E4BEB7BF0B49DCAB3B7E17DD0C999F1FE02&fileName=/typescript-3.8.0-insiders.20200129.tgz"
    }
}

and then running npm install.

---

There is also a playground for this build.

[weswigham]

You've re-fixed #31574 in a different way; rather than reentering function expression checking from the outside to ensure contextual parameter types get set, you're locking in a type context or no, since the required fix is simply that a type (any type) be locked in, to prevent us from looking at the uninstantiated signature. There's a good argument to be made that, rather than locking in a zero-context type, we should be locating the appropriate context and applying it eagerly if we are invoked without context information (avoiding any circularity that may be involved in "restarting" the operation from a more appropriate point, as currently happens in quick info). I think that the current scheme could produce weirdly differing results between similar code structures, where when you move a function before/after another call in the same contextual context, its inferred type changes between the declared and contextually inferred results. So while this achieves internal consistency in a similar way to the other fix, since it guarantees a locked in parameter type, it seems like it is eschewing spatial consistency between similar structures to do so, as now the behavior in theory wobbles between declared and inferred type depending on which entrypopint the code's structure dictates.

Also, you should really add a fourslash test for the quickinfo issue you describe in the OP.

[weswigham]

Oh, also add a normal test and fourslash test for this:

declare function foo<T>(settings: (row: T) => { value: string, func?: Function }, obj: T): void;

foo(o => ({
        value: o.name,
        func: x => 'foo'
    }),
    new Error(),
);

which is to say, your example, but with inverted argument order. This PR actually fixes it (entirely due to skipping fetching the return type because o is a context sensitive parameter); without this PR is errors on o.name with an Object is of type unknown error. We should make sure the unreported quickinfo/ordering issues have regression tests.

[weswigham]

Oof, I'd also wanna see some fourslash/type tests for

declare function foo<T extends { name: string }, U extends keyof T>(settings: (row: T) => { value: T[U], func?: Function }, obj: T, key: U): U;

function q<T extends { name: string }>(x: T): T["name"] {
    return foo(o => ({
        value: o.name,
        func: x => 'foo'
    }),
        x,
        "name"
    );
}

foo(o => ({
    value: o.name,
    func: x => 'foo'
}),
    new Error(),
    "name"
);

while the error behaviors are the same both with and without this PR, the signature quickinfo on both foo calls has very different types in each, so recording the behavior in a test seems good.

[weswigham]

I'm still a little concerned that we may now have a "when I write my declarations in this order it works, but in this order it fails" situation, since weather a contextual parameter type or a context-less parameter type is cached depends on the entrypoint; but I haven't been able to come up with a test case yet, so.