Disallowing non-null assertions using the ! postfix operator disregards its validity in the majority of code-bases

[KilianKilmister]

What version of this package are you using?
"version": "19.0.1",

What operating system, Node.js, and npm version?

node-v14.9.0
npm-7.0.0-beta.9
macOS Catalina-v10.15.6

Summary

The postfix operator has the very valid usecase of making the type system behave reasonably in places where it can't grasp the causality of an operation. This restriction specifically affects arrays and maps in their most basic functionaly. There is no alternative of reasonalble terseness, so the existing alternatives cause clutter that is completly unnecessary in terms of type-safety.

Array and Maps are so common that this rule affect an enormous amount of code while forbidding the use of the ! postfix operator will only catch very few things that could lead to actual runtime issues.

While there are other things that are negatively affected by this, The specific ones stated here is more than enough to make this rule unusable across the majority of common code-bases.

Current Rule Definition

{
  '@typescript-eslint/no-non-null-assertion': 'error'
}

Suggested Change

{
  '@typescript-eslint/no-non-null-assertion': 'off'
}

Examples of the type system including undefined where it is causally impossible

Maps and their most basic functionality

Given a function mapping object k/v pairs to a ReadonlyMap

function mapObject <R extends Record<PropertyKey, unknown>> (object: R) {
  const map = new Map<keyof R, R[keyof R]>()
  for (const key in object) {
    map.set(key, object[key])
  }
  // we explicitly set the map as readonly, so as far as type-safety is
  // concerned this map will never change
  return map as ReadonlyMap<keyof R, R[keyof R]>
}

We know the exact state of the returned Map and it is declared as immutable, but the type system isn't aware of the consequences.

const map = mapObject({ 0: 'a', 1: 'b', 2: 'c' } as const)

// never the less, typescript still assumes that the result could be undefined
// typeof zero --> "a" | "b" | "c" | undefined
const zero = map.get(0)

// similarly even if we explicitly check for the existence of the key-value pair
// typescript won't be any smarter
if (map.has(1)) {
  // typeof one --> "a" | "b" | "c" | undefined
  const one = map.get(1)
}

const mapB = new Map<string, [count: number, message: string]>()

function someAPI (key: string) {
  // we explicitly add the key/value pair if it doesn't exist
  if (!mapB.has(key)) mapB.set(key, [0, `this is the entry for ${key}`])
  // typeof value --> [count: number, message: string] | undefined
  const value = mapB.get(key)
  // throws a compiler error
  value[0]++ // error TS2532: Object is possibly 'undefined'.
  return value[1] // error TS2532: Object is possibly 'undefined'.
}

Arrays and their most basic functionality

Given a function that sorts any amount of number and returns them in an array

function sortNumbers <T extends number> (...args: T[]) {
  return args.sort((a, b) => a - b)
}

We know that there is guarantied to be a number at the first position, and accessing it via indexing does confirm that again. Never the less typescript will throw a compiler error if we want to assign the return value of the first call to Array.prototype.shift to typeof arr[0]

const arr = sortNumbers(2, 5, 7, 3, 1, 8, 10)
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
let smallest = arr[0]
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10 | undefined
const shift = arr.shift()
// throws a compiler error
smallest = shift // error TS2322: Type '2 | 1 | 5 | 7 | 3 | 8 | 10 | undefined' is not assignable to type '2 | 1 | 5 | 7 | 3 | 8 | 10'. Type 'undefined' is not assignable to type '2 | 1 | 5 | 7 | 3 | 8 | 10'.

Probably the most common way of checking wether an array is empty of not is checking wether its length is 0 or falsy, nut the type system doesn't understand that causality.

// ReturnType<typeof someAPI2> --> string | undefined
function someAPI2 (arr: string[]) {
  // we explicitly check wether the array is empty or not
  if (arr.length) {
    // return type --> string | undefined
    return arr.pop()
  }
  // return type --> string
  return 'received an empty array'
}

Comparing the ! postfix operator to the available alternatives

We use the same functions as above

Using the ! postfix operator

The resulting source code is very clean. The addition of the operator doesn't plaster the code with extra information, type infernce reflects runtime behaviour, and the operators are transpiled a way at compile time.

Maps

function mapObject <R extends Record<PropertyKey, unknown>> (object: R) {
  const map = new Map<keyof R, R[keyof R]>()
  for (const key in object) {
    map.set(key, object[key])
  }
  return map as ReadonlyMap<keyof R, R[keyof R]>
}

const map = mapObject({ 0: 'a', 1: 'b', 2: 'c' } as const)

// typeof zero --> "a" | "b" | "c"
const zero = map.get(0)!

if (map.has(1)) {
  // typeof one --> "a" | "b" | "c"
  const one = map.get(1)!
}

// with a more generic map
const mapB = new Map<string, [count: number, message: string]>()

function someAPI (key: string) {
  if (!mapB.has(key)) mapB.set(key, [0, `this is the entry for ${key}`])
  const value = mapB.get(key)!
  // no error
  value[0]++
  return value[1]
}

Arrays

function sortNumbers <T extends number> (...args: T[]) {
  return args.sort((a, b) => a - b)
}
const arr = sortNumbers(2, 5, 7, 3, 1, 8, 10)
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
let smallest = arr[1000]
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
const shift = arr.shift()!
// no error
smallest = shift

// ReturnType<typeof someAPI2> --> string
function someAPI2 (arr: string[]) {
  if (arr.length) {
    // return type --> string
    return arr.pop()!
  }
  // return type --> string
  return 'received an empty array'
}

Using conditionals

The resulting source AND production code include a large amount of redundant operations, the code might appear a bit odd at first, but it is readable.

Maps

function mapObject <R extends Record<PropertyKey, unknown>> (object: R) {
  const map = new Map<keyof R, R[keyof R]>()
  for (const key in object) {
    map.set(key, object[key])
  }
  return map as ReadonlyMap<keyof R, R[keyof R]>
}

const map = mapObject({ 0: 'a', 1: 'b', 2: 'c' } as const)

const zero = map.get(0)
if (zero ?? true) throw new Armageddon('The impossible has happened')
// typeof zero --> "a" | "b" | "c"

if (map.has(1)) {
  const one = map.get(1)
  if (one ?? true) throw new Armageddon('Start stockpiling supplies')
  // typeof one --> "a" | "b" | "c"
}

// with a more generic map
const mapB = new Map<string, [count: number, message: string]>()

function someAPI (key: string) {
  if (!mapB.has(key)) mapB.set(key, [0, `this is the entry for ${key}`])
  const value = mapB.get(key)
  if (value ?? true) throw new Armageddon('The end is nigh')
  // no error
  value[0]++
  return value[1]
}

Arrays

function sortNumbers <T extends number> (...args: T[]) {
  return args.sort((a, b) => a - b)
}
const arr = sortNumbers(2, 5, 7, 3, 1, 8, 10)
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
let smallest = arr[0]
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
const shift = arr.shift()
if (shift ?? true) throw new Armageddon('For may the Lord have mercy on our souls')
// no error
smallest = shift

// ReturnType<typeof someAPI2> --> string
function someAPI2 (arr: string[]) {
  if (arr.length) {
    // return type --> string
    out = arr.pop()
    if (out ?? true) throw new Armageddon('*grabs keys to panic-room*')
    return out
  }
  // return type --> string
  return 'received an empty array'
}

Using type assertions

The resulting source code grows a considerable amount as we need explicitly write the type logic which was inferred in the other examples before we can cast it. And not all of it is stripped at compile time if we use the more practical approach and use what inferrence we can instead of typing everything manually. Either way, as a result the source becomes a lot less consumable for readers and mentainers because of all the declarations are (wether real or type) have to be kept in mind to avoid naming conflicts. much of the necessary logic also has to use conditional types which can get very large very fast. All of this is also much more error prone compared to pure inference. (i infact had to correct this section multiple times)

Maps

function mapObject <R extends Record<PropertyKey, unknown>> (object: R) {
  const map = new Map<keyof R, R[keyof R]>()
  for (const key in object) {
    map.set(key, object[key])
  }
  return map as ReadonlyMap<keyof R, R[keyof R]>
}

// NOTE: need a declaration
const obj_A = { 0: 'a', 1: 'b', 2: 'c' } as const

const map = mapObject(obj_A)

// typeof zero --> "a" | "b" | "c"
const zero = map.get(0) as (typeof obj_A)[keyof typeof obj_A]

if (map.has(1)) {
  // typeof one --> "a" | "b" | "c"
  const one = map.get(1) as (typeof obj_A)[keyof typeof obj_A]
}

type Arr = [count: number, message: string]
// with a more generic map
const mapB = new Map<string, Arr>()

function someAPI (key: string) {
  if (!mapB.has(key)) mapB.set(key, [0, `this is the entry for ${key}`])
  const value = mapB.get(key) as Arr
  // no error
  value[0]++
  return value[1]
}

Arrays

function sortNumbers <T extends number> (...args: T[]) {
  return args.sort((a, b) => a - b)
}
const arr = sortNumbers(2, 5, 7, 3, 1, 8, 10)
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
let smallest = arr[0]
// typeof smallest --> 2 | 1 | 5 | 7 | 3 | 8 | 10
const shift = arr.shift() as (2, 5, 7, 3, 1, 8, 10)[]
// no error
smallest = shift

// ReturnType<typeof someAPI2> --> string
function someAPI2 (arr: string[]) {
  if (arr.length) {
    // return type --> string
    return arr.pop() as string
  }
  // return type --> string
  return 'received an empty array'
}

[ospfranco]

I managed to get this working by not using standard or standard js, but by manually installing the rule set and using a .eslintrc.js file where I could turn this rule off... not ideal, but better than nothing

[LinusU]

I'm curious to know how often this actually comes up in your projects 🤔

In my team we love having this rule since it avoids some very hard to track down bugs where null/undefined have propagated quite far from the actual ! use. E.g. "cannot read property foo of undefined" in another part of the app.

We are using a combination of ts-unwrap (which works like ! but with a runtime check that throws an error if the value is null/undefined), and "eslint-disable" comments.

What I like about having to use "eslint-disable" for these cases is that it signals that "hey, TypeScript cannot guarantee that this code works, so we have to be extra careful here" to the developers.

Just my 2 cents, would love to hear more about your use cases...

[ospfranco]

While I cannot talk for the community at large, the ! is very convenient

I have also worked with flow codebases that uses the equivalent of ts-unwrap which is invariant... and while runtime checks are useful, our codebase was not free of dumb typecasting (ex: (foo: any)()) because flow doesn't have this operator and sometimes added runtime checks brings nothing.

So basically, even though you can add runtime checks all over the code, it is not a panacea, simply telling the type checker to shut up is sometimes what you want

[theoludwig]

I'm against this rule, let the compiler do its job, and warns you if you do mistakes.
Instead to do :

const arr = sortNumbers(2, 5, 7, 3, 1, 8, 10)
let smallest = arr[0]
const shift = arr.shift()!
smallest = shift

Check yourself if shift is actually null | undefined.

const shift = arr.shift()
if (shift == null) {
  throw new Armageddon('For may the Lord have mercy on our souls')
}
smallest = shift

It adds a little bit of code, but it is way safer than using !.

For example :

const arr = sortNumbers()
let smallest = arr[0]
const shift = arr.shift()!
smallest = shift

This code would compile, but you would get shift as undefined at runtime whereas, for TypeScript, it would be a valid `number.

[mightyiam]

@rostislav-simonik and I looked at this. Here is our summary. In the vast majority of cases where a value may be nullish, type guards should be used. In the expectional cases where a non-null assertion is appropriate, do use one and place an eslint-disable-line above it.

By the way, type guards in TypeScript have improved since the opening of this issue, if that matters.

With this we are closing this issue. Feel free to try to change our minds.