microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.8k stars 12.46k forks source link

4.2.3 Regression: Type with optional properties not safely asserting with `undefined` in their unions #45523

Closed frank-weindel closed 3 years ago

frank-weindel commented 3 years ago

Bug Report

Sorry I don't feel like I have the proper TypeScript vernacular to describe this issue in words very well. So any help there will be highly appreciated! See example below.

🔎 Search Terms

optional property, properties, Record, type assertion, indexed accessor

🕗 Version & Regression Information

⏯ Playground Link

Playground link with relevant code

💻 Code

type MyType = {
  a: string, // Hover type: string
  b?: number, // Hover type: number | undefined
  c?: string, // Hover type: string | undefined
  d?: string // Hover type: string | undefined
};

const myObj: MyType = {
  a: 'value',
  b: 123,
  c: undefined
};

const pojo: Record<string, string | number> = myObj; // ... (Line A)
/*
Compile error expected, but doesn't happen in TypeScript >= 4.2.3:

Type 'MyType' is not assignable to type 'Record<string, string | number>'.
  Property 'b' is incompatible with index signature.
    Type 'number | undefined' is not assignable to type 'string | number'.
      Type 'undefined' is not assignable to type 'string | number'.(2322)
*/

Object.keys(pojo).forEach(key => {
  const val = pojo[key]; // typeof val is: string | number ... (Line B)
  console.log(val);
})

// Console output:
// "value"
// 123
// undefined             <---- undefined is incompatible with (string | number)! ... (Line C)

🙁 Actual behavior

No compile error on the assignment on Line A. Hence an undefined value can emerge from a Record<string, string | number> (Line C)

🙂 Expected behavior

The following compile error is expected on the assignment on Line A.

Type 'MyType' is not assignable to type 'Record<string, string | number>'.
  Property 'b' is incompatible with index signature.
    Type 'number | undefined' is not assignable to type 'string | number'.
      Type 'undefined' is not assignable to type 'string | number'.(2322)

This will inform the developer to either add undefined to the Record value union, or find another way to fix the error.

fatcerberus commented 3 years ago

This looks like a use case for exactOptionalPropertyTypes (TS 4.4, currently in beta), which would prevent the assignment to myObj due to the explicit use of undefined for an optional property. That being said, undefined can already emerge from a Record<string, T> even without this pitfall:

console.log(pojo["arglbarghlarggggg"]);  // undefined, no compile error

...which is why noUncheckedIndexedAccess exists (and why val has undefined in its type in your playground).

RyanCavanaugh commented 3 years ago

What @fatcerberus said; the desired behavior is achieved with that configuration (EOPT on, NUIA on).

frank-weindel commented 3 years ago

This looks like a use case for exactOptionalPropertyTypes (TS 4.4, currently in beta), which would prevent the assignment to myObj due to the explicit use of undefined for an optional property.

What @fatcerberus said; the desired behavior is achieved with that configuration (EOPT on, NUIA on).

Interesting, but wouldn't this still be considered a bug if it worked prior to 4.2.3? I'm glad there's a new option to cover it in 4.4, but confused why there's an option in 4.4 to fix something that broke in 4.2.3.

...which is why noUncheckedIndexedAccess exists (and why val has undefined in its type in your playground).

Ahh yes, I did turn that on in the playground later while trying to find a solution. Originally it was off. But that option shouldn't be needed at all in this specific case.

frank-weindel commented 3 years ago

@RyanCavanaugh @fatcerberus So I tried enabling both noUncheckedIndexedAccess and exactOptionalPropertyTypes in 4.4 and it does not have the effect that I'd expect.

  1. First, just enabling it with the code I posted causes a compile error because of my assignment of property c as undefined on line 13. That makes sense based on what exactOptionalPropertyTypes is designed to do. Okay.
  2. If I change my code to explicitly allow undefined as a value for optional properties b, c, d, it now compiles, but now we're back to the same essential place I was at in my original bug report. Line A should be reporting an error due to undefined not being in the union for the Record value. Again this worked correctly prior to 4.2.3.
// noUncheckedIndexedAccess on, exactOptionalPropertyTypes on

type MyType = {
  a: string, // Hover type: string
  b?: number | undefined, // Hover type: number | undefined
  c?: string | undefined, // Hover type: string | undefined
  d?: string | undefined // Hover type: string | undefined
};

const myObj: MyType = {
  a: 'value',
  b: 123,
  c: undefined
};

const pojo: Record<string, string | number> = myObj; // ... (Line A)
/*
Compile error STILL expected, but doesn't happen in TypeScript >= 4.2.3:

Type 'MyType' is not assignable to type 'Record<string, string | number>'.
  Property 'b' is incompatible with index signature.
    Type 'number | undefined' is not assignable to type 'string | number'.
      Type 'undefined' is not assignable to type 'string | number'.(2322)
*/

Object.keys(pojo).forEach(key => {
  const val = pojo[key]; // typeof val is: string | number | undefined (undefined added to union only because of noUncheckedIndexedAccess) ... (Line B)
  console.log(val);
})

// Console output:
// "value"
// 123
// undefined             <---- technically OK now only because of noUncheckedIndexedAccess, but Line A is the real issue ... Line C
typescript-bot commented 3 years ago

This issue has been marked as 'Question' and has seen no recent activity. It has been automatically closed for house-keeping purposes. If you're still waiting on a response, questions are usually better suited to stackoverflow.

frank-weindel commented 3 years ago

@RyanCavanaugh @fatcerberus Please see the above if you get a chance. The suggested solution does not solve the core of the problem. Again this was working fine in a previous version of TypeScript, but since regressed. I'm not aware of any release documentation saying this was regressed intentionally.

andrewbranch commented 3 years ago

I'm not aware of any release documentation saying this was regressed intentionally.

https://devblogs.microsoft.com/typescript/announcing-typescript-4-2/#relaxed-optional-index-rules

fatcerberus commented 3 years ago

Hmm, I wonder if exactOptionalPropertyTypes should disable that behavior.

andrewbranch commented 3 years ago

I think exactOptionalPropertyTypes makes that behavior more robust—it indicates that the optional properties represent keys that are either present or missing, but not undefined-valued, just like an index signature represents keys that could be present or not. The behavior at the time was known to be unsound but was a usability fix falling out of another bug fix; exactOptionalPropertyTypes actually tightens it up nicely.

fatcerberus commented 3 years ago

it indicates that the optional properties represent keys that are either present or missing, but not undefined-valued, just like an index signature represents keys that could be present or not.

That was my point - the mentioned change makes it possible to explicitly assign undefined to a Record type without undefined in its value slot (see example above), even with exactOptionalPropertyTypes enabled. Seems inconsistent.

andrewbranch commented 3 years ago

Oh, you’re talking about properties that are declared optional and have undefined in their property type signature? Yeah, that could be worth changing. I feel like I commented something about that on the exactOptionalPropertyTypes PR, actually.