Disable widening for nameless function expressions and arrow functions

[weswigham]

Fixes #241
Fixes #20859

I exempted named function expressions from the change (not mentioned in the original discussion on #241), since their (widened) type needs to be internally visible. We already have a test to that effect.

[weswigham]

This causes some good RWC breaks. In our VSCode test, we find more locations where not all code paths return a value; places where someone's written a branch with return null or return undefined and no other return statements that return an actual value. This looks like a good thing, as with noImplicitReturn, I was surprised these weren't caught already. In 4 of our other tests, this catches errors in the style of #20859, where an excess property was supplied but not caught before. In another one of our tests, we catch an assignability error hidden by a null return type widening to an any, similar to the example given in #241.

[RyanCavanaugh]

@JsonFreeman !

[weswigham]

We had a lot of discussion about this today and a generally happy with the direction, but realized that the current implementation allowed fresh types to leak into contexts where they would generate bad errors. After thinking about weather widened signatures were a good idea for a bit, I've realized that, generally speaking, you want excess property checks whenever your return type has a contextual type (meaning you're getting assigned to something). Ergo, I now only disable widening for nameless function expressions and arrow functions when there is a contextual type. The allows the fresh return type to be assigned to the contextual type, report freshness errors as desired, and then cease to exist (as the contextual type or an instantiation of it becomes the resulting type after that in the flow). A nice effect of this (compared to a widening signature) is that you still get excess property checks on union'd return types, if applicable, eg:

declare function s(arg: () => {a: {b: number}}): void;
s(() => (Math.random() > 0.5 ? {
    a: {
        a: 12,
        b: 11
    }
} : {
    a: {
        b: 11
    }
}));

reports an excess property error on the object literal in the first branch for the extraneous a.

[weswigham]

@ahejlsberg What do you think about this with the contextual type presence limitation?

[ahejlsberg]

@weswigham I like the basic principle (i.e. keeping around the most specific type when it is known that the type isn't going to become an inferred type). However, there are cases where you have a contextual type and become (part of) an inferred type. For example, in a destructuring with a default value, the default value is the contextual type for the destructured value, and the final inferred type is a union of the two:

let { x = undefined } = { x: 42 };  // number | undefined

Here, the 42 is contextually typed by undefined, and then subsequently widened. We'll have to account for this situation somehow. There may be others as well, though I don't recall any right now.

[weswigham]

@ahejlsberg I've implemented a flag on getContextualType that causes it to not look for contextual types implied from binding patterns, which is then used by function return type contextual typing. Technically, this means that in

const { x = () => true }/*: OptionalAnnotation */ = { x: () => false };

x will now have type () => boolean instead of () => false | () => true.

IMO, binding pattern contextual types are something we've already introduced layers of code to work around in a few places already, actually. We introduced them in #4598 to deal with improving the implied optionality of inferred signature types, then in #10577 because the literal on the RHS was contextually typed by the initializer in the binding pattern, we altered how we checked destructuring initializers to produce a union type (rather than verifying assignability) to work around the top level initializer being contextually typed by the subordinate initializer (indeed, had there been no contextual type there would be no issue). When you think about what the destructuring desugars to:

const _tmp/*: OptionalAnnotation */ = { x: () => false };
const x = _tmp.x === undefined ? (() => true) : _tmp.x;

in the general case, it doesn't seem to make sense for the LHS binding pattern with intializer types to contextually type the RHS at all (it is a conditional assignment after it in the flow), given where else we apply contextual types (or where we'd apply contextual types in the equivalent). Parameter locations are special because we want to imply even more information (from use!) than is typical in a normal expression or statement context.

[typescript-bot]

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.