Short-circuit or (||) allows incorrect code to typecheck

[marcomorain]

TypeScript Version: 2.1.4

Code

interface Data {
  items: number[]
}

export interface DataCollection {
  [i: string]: Data
}

const collection: DataCollection = {}

const datum: Data = collection['not-present'] || 'horse' // this line should  fail the type-check

datum.items = [1, 2, 3];

Expected behavior:

A compile time error.

I would expect something like:

error TS2322: Type '"horse"' is not assignable to type 'Data'.

Actual behavior:

A runtime error:

Uncaught TypeError: Cannot create property 'items' on string 'horse'

[svieira]

Are you compiling with strictNullChecks on? If so, you need to add an explicit | undefined to your definition of DataCollection and everything will behave as expected.

interface Data {
  items: number[]
}

export interface DataCollection {
  [i: string]: Data | undefined
}

const collection: DataCollection = {}

const datum: Data = collection['not-present'] || 'horse' // this line now *does* fail the type-check

datum.items = [1, 2, 3];

[RyanCavanaugh]

In general you want to use

interface DataCollection {
  [i: string]: Data | undefined
}

where you'll be accessing by keys of unknown veracity and

interface DataCollection {
  [i: string]: Data
}

when you know that all keys you're using do map to values.

[jeffreymorlan]

It doesn't seem right for || to just ignore the right side's type like that. Either:

1. TS is assuming that the left side is always truthy, in which case the || should be an error because the right side is dead code,
   or
2. TS isn't assuming that, in which case the result should be a union type.

[willfrew]

@RyanCavanaugh, this definitely doesn't strike me as Working As Intended. @svieira's comment is unrelated to the actual bug reported here. Assigning a Data | string to a const explicitly typed with an interface that strings do not fulfil should not depend on strictNullChecks (unless I'm missing something totally obvious).

Please take another look?

[svieira]

💡 Ah, now I see @jeffreymorlan - yes, that does look like it should be caught as dead code.

@willfrew - the issue is that you're not assigning Data | string in the case of strictNullChecks - you are assigning Data because you've explicitly said that DataCollection will never return undefined. It is dead code in that case and should probably be warned about unless allowUnreachableCode is on.

[willfrew]

@svieira I stand corrected, I was missing something obvious 😅 thanks for explanation!

[marcomorain]

I've been busy so sorry for the delay in getting back.

Here is my tsconfig.json

{
  "compileOnSave": true,
  "compilerOptions": {
    "target": "es5",
    "jsx": "react",
    "noImplicitAny": true,
    "noImplicitThis": true,
    "strictNullChecks": true,
    "moduleResolution": "node",
    "sourceMap": true,
    "allowJs": true
  }
}

Thanks for the suggestion @RyanCavanaugh - I'll add the | undefined to my code.

It still seems strange to me that given an interface like this:

interface DataCollection {
  [i: string]: Data
}

the compiler assumes that all keys will map to values. I'd prefer if the compiler forced the user to prove to it that this is true. It's similar to strict null checks, where calling foo.bar will produce a warning that foo can be null unless otherwise shown to be not null.

[RyanCavanaugh]

Opinions tend to differ on this one, which is understandable (it tends to depend on the application and the preferences of the developer), so making not-undefined the default lets people opt in with a straightforward mechanism of adding | undefined.

There's sort of an analog to arrays where people typically write code like

for(var i = 0; i < arr.length; i++) {
  console.log(arr[i].something);
}

We could make you write this instead

for(var i = 0; i < arr.length; i++) {
  // non-null assert because you "know" you checked array bounds
  console.log(arr[i]!.something);
}

But it doesn't actually fix the real problem where you accidently write this

for(var i = 0; i <= arr.length; i++) {
  // Got in the habit of writing 'arr[i]!' and now I'm sad again
  console.log(arr[i]!.something);
}

[marcomorain]

Thanks for the input Ryan, and thanks for all the hard work you do on Typescript. It's really appreciated. 👍