Inconsitent (dis)allowing of undefined in spread operator

[olekzabl]

TypeScript Version: 2.7.0-dev.20171118

Code

declare let v1: undefined;
declare let v2: {[key: string]: string};
declare let v3: {[key: string]: string}|undefined;

let map: {[key: string]: string};
map = {...v1}; // FAILS
map = {...v2};
map = {...undefined}; // FAILS
map = {...v3}; // OK

Expected behavior:

1. Consistency: undefined values are either allowed or disallowed in spread operator, regardless of whether they come explicitly, through a variable of undefined type, or through a variable of a variant type with an undefined branch.
2. Ideally, backward compatibility with the previous versions.

Actual behavior:

1. Values of "bare" undefined type are forbidden, while values of at least some X|undefined types
   are allowed.
2. No backward compatibility, as the lines commented with "FAILS" are accepted by 2.5.3.

This was probably just an oversight. @sandersn

[mhegazy]

PRs are welcomed.

[Kingwl]

if --strictNullChecks is not enabled
undefined in union type will be ignored
is this a feature? what is the expected behavior

[mhegazy]

is this a feature? what is the expected behavior

this is the expected behavior.. with --strictNullChecks false, undefined and null are in the domain of all types.

[srolel]

@mhegazy I'd like to work on this. I'm uncertain what the behavior should be. Should a union type that includes undefined be "unspreadable", or that undefined type should be spreadable?

[sandersn]

@mosho1 start with the tests in tests/cases/conformance/types/spread, particularly objectSpreadStructNull, spreadUnion2 and spreadUnion3. While I agree that the current rules are complex, any simplification needs to preserve those behaviours.

The PR that introduce the more complex rule is #18451, added to fix #16326. The obvious simplification that I see is to allow undefined everywhere. Maybe you can come up with something better, though.

[sandersn]

The important question about undefined/null or other primitives is whether using them in a spread is ever intentional. Right now they're disallowed because we think their use in a spread is the result of a mistake, so it makes sense for Typescript to give an error. #16326 gives a real, useful pattern, which is why we added an exception for it.

[srolel]

@sandersn I sometimes use optionally undefined parameters in object spread. For example, {...defaults, ...overrides, ...someMoreOverrides}. I think being able to have overrides and someMoreOverrides optional is useful. The same goes for null.

[sandersn]

Like this?

function f(overrides: { a } | undefined, someMoreOverrides: { b } | undefined) {
    return { a: 1, ...overrides, ...someMoreOverrides };
}

The second case of isValidSpreadType explicitly allows unions with undefined, false and null by saying that types that have a DefinitelyFalsy member and are still valid spread types once the definitely falsy member is gone, are valid spread types.

function isValidSpreadType(type: Type): boolean {
    return !!(type.flags & (TypeFlags.Any | TypeFlags.NonPrimitive) ||
        getFalsyFlags(type) & TypeFlags.DefinitelyFalsy && isValidSpreadType(removeDefinitelyFalsyTypes(type)) ||
        type.flags & TypeFlags.Object && !isGenericMappedType(type) ||
        type.flags & TypeFlags.UnionOrIntersection && !forEach((<UnionOrIntersectionType>type).types, t => !isValidSpreadType(t)));
}

It's quite possible this only works with the simple example I came up with, though.

[srolel]

Yes, that's the only valid usage I can think of, and I can't think of an example where that rule doesn't work for it. I can't see a valid use-case for spreading strictly undefined or null types, and agree with the decision to disallow it. The only issue then is this being an unintended backward incompatible change.