Optional properties coming from conditional spread lose "| undefined"

[danvk]

TypeScript Version: 4.2.0-dev.20201105

Search Terms:

• spread optional

Expected behavior:

The start symbols should both have type number | undefined.

Actual behavior:

The one destructured from the conditional spread has somehow dropped the undefined.

Related Issues:

#39376

Code

I'd expect the two start variables in the following code to have the same type. But they don't, one is number, whereas the other is number | undefined:

declare let hasDates: boolean;

{
  const nameTitle = { name: 'Khufu', title: 'Pharaoh' };
  const pharaoh = {
    ...nameTitle,
    ...(hasDates && { start: -2589, end: -2566 }),
  };
  // type is const pharaoh: {
  //     start?: number;
  //     end?: number;
  //     name: string;
  //     title: string;
  // }, which is character-for-character identical to Reign, below.
  const { start } = pharaoh;
  // const start: number
}

interface Reign {
  start?: number;
  end?: number;
  name: string;
  title: string;
}
declare let reign: Reign;

{
  const { start } = reign;
  // const start: number | undefined
}

Output

"use strict";
{
    const nameTitle = { name: 'Khufu', title: 'Pharaoh' };
    const pharaoh = Object.assign(Object.assign({}, nameTitle), (hasDates && { start: -2589, end: -2566 }));
    // type is const pharaoh: {
    //     start?: number;
    //     end?: number;
    //     name: string;
    //     title: string;
    // }, which is character-for-character identical to Reign, below.
    const { start } = pharaoh;
    // const start: number
}
{
    const { start } = reign;
    // const start: number | undefined
}

Compiler Options

{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "alwaysStrict": true,
    "esModuleInterop": true,
    "declaration": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "moduleResolution": 2,
    "target": "ES2017",
    "jsx": "React",
    "module": "ESNext"
  }
}

Playground Link: Provided

[RyanCavanaugh]

@andrewbranch I think we should consider for 4.1 at least; this seems bad

[andrewbranch]

This is very interesting. It’s actually been a bug at least since 3.8, with #34853, but the new behavior of conditional spreads creating optional properties increases your chances of observing it. Here’s an example of the bug in 3.8.3.

Basically, as far as I can tell, we’ve never had the necessary machinery to mix optionality into the type retrieved from a synthetic optional property. The place where we union with undefined for optional parameter declarations and property signatures actually looks at the declaration node for the ? token—we never even look at SymbolFlags.Optional for this purpose.

Despite having diagnosed the problem, I’m having trouble fixing this in a way that doesn’t broadly change the type printout of these synthetic optional property symbols from x?: number to x?: number | undefined. The couple different places where you could add the optionality also come into play in the node builder. I’m assuming the reason this isn’t a problem for properties that can be traced back to an optional property signature is because we simply use that property signature node as part of the printout. I’m not sure the best way to proceed—I think it would be ok to just tweak the node builder such that if a property is both optional and synthetic, and its type is a union, we can filter undefinedType from the union. But maybe there’s a more subtle option I’m missing. /cc @weswigham

[weswigham]

Hm, that doesn't seem so bad, at least. Certainly better than a synthetic optional property declaration or the like. The other option would be adding a SymbolLink flag that represents "should this property have undefined mixed in" (the thing we directly look up from the declaration). Essentially adding a cache for the declaration lookup, then directly setting that cache in synthetic property situations like this.

[ahejlsberg]

Why is it a problem that the printout changes to x?: number | undefined. We do that in other places anyway. For example:

type Foo = (x?: number) => void;  // Prints back as (x?: number | undefined) => void

Or am I misunderstanding something?

[andrewbranch]

It’s not a huge problem, it just changes a bunch of seemingly unrelated baselines, and is more verbose while conveying no more information, so it seems undesirable all other things held equal.

[ahejlsberg]

is more verbose while conveying no more information, so it seems undesirable all other things held equal.

I tend to agree, but that is nonetheless what we currently do for print back of optional properties. If we're going to change it, we should change it across the board, not just in this targeted scenario.

[andrewbranch]

Huh, I must have been confused by existing baselines that already exhibit this bug, because I was confident this didn’t happen for property signature printback, but apparently I’m wrong. In that case, I think this is a small change, and we can look at making printback more concise separately.

[ahejlsberg]

Actually, I think the current scheme does convey important information. The type of an optional property really is xxx | undefined since that's what you get when you read the property and that's what you're allowed to assign to the property. It's just that instead of complaining that x?: number is an invalid declaration, as a convenience we automatically union in undefined.

[andrewbranch]

Hmm, actually this causes another break. Consider this case:

declare let r: { [key: string]: number };

declare let fromAnnotation: { x?: number };
r = { ...fromAnnotation }; // Error: Type 'undefined' is not assignable to type 'number'.

let fromExpression = { ...!!true ? { x: 3 } : {} };
r = { ...fromExpression }; // Ok

The latter works today, and is asserted in a test, only because when we get the type of the x property in fromExpression, we incorrectly do not include undefined. If we fix that type, both assignments error, which is... a win for consistency, and I don’t know how bad the bad consequences are—but it is a breaking change.

[danvk]

@andrewbranch much of the discussion in the Design Meeting was about index types. I don't entirely see the connection. Is an index signature somehow involved behind the scenes here?

I noticed that the example in the release notes for TS 4.1 RC is also broken because of this issue:

interface Person {
    name: string;
    age: number;
    location: string;
}

interface Animal {
    name: string;
    owner: Person;
}

function copyOwner(pet?: Animal) {
    return {
        ...(pet && pet.owner),
        otherStuff: 123
    }
}

const owner = copyOwner();

const loc = owner.location;  // type is string, should be string | undefined
console.log(loc.blink());
// 💥 Cannot read property 'blink' of undefined

playground

[andrewbranch]

@danvk a lot of the conversation focused on index signatures because fixing this bug breaks an existing test where an object with optional properties is asserted to be assignable to an index signature. There is no disagreement that the type of the property is a bug, but we can’t fix it this late in the release cycle because the bug has been around at least since 3.8, and fixing it is a breaking change. So, much of the discussion focused on the question of when we do fix the property type, should we consider additional changes to make the assignability of optional properties to index signatures less breaky.