[no-unnecessary-condition] False positives with arrays and optional chains

[romeovs]

This

{
  "rules": {
    "@typescript-eslint/no-unnecessary-condition": ["error", "ignoreRhs": true | false] 
  }
}

type Item = {
  type : string
}

function fn (arr : Item[]) : string {
  // eslint complains about the optional chain on the next line
  if (arr[0]?.type === "foo") {
    return 'FOO'
  }
  return 'BAR'
}

Expected Result
No lint because the array item might not exist.

Actual Result
Eslint error, even though there should not be one

Versions

package	version
@typescript-eslint/eslint-plugin	2.11.0
@typescript-eslint/parser	2.12.0
TypeScript	3.7.3
ESLint	6.7.2
node	12.13.1
npm	6.12.1

[bradzacher]

This is because typescript does not have any concept of array indices being undefined.
When we ask typescript for the type of the thing being optional chained (arr[0]), typescript tells us that in no uncertain terms is this nullish.

Because the rule runs off of the types, and assumes that they are correct, it flags it as unnecessary.

You'll see the same "false positive" if arr[0] resolved to a type that couldn't be falsy:

const x: object[] = [];
if (x[0]) {} // error - unnecessary condition, x[0] is always truthy

That particular case is being fixed in #1534

cc @Retsam - as they've been working on the rule.

To "fix" this, we have to manually encode into the rule the fact that array indices might be undefined.

[Retsam]

Ah, right I forgot about this case. I can get a fix in for this case, too. I should be able to apply pretty much the same logic as #1534, so I'll probably just add it to that PR.

It'll have the same limitation: it won't work with an intermediary variable - this case is a lot harder to detect:

const head = array[0];
if (head?.type === "foo") { return "FOO"; }

But the originally linked code should be easy enough to fix.

[schmod]

For the second case described above, can we make this rule configurable to opt-out of its behavior for the optional-chaining operator?

[bradzacher]

happy to accept a PR if you'd like an option.

[Retsam]

The issue described above also happens without the optional-chaining operator. This will raise a lint error with the same fundamental issue as head?.type === "foo"

const head = array[0];
if (head) { return "FOO"; }

So I don't think an option that specifically disables checking of optional-chaining is the best solution here.

[romeovs]

Yes an option to ignore no-unnecessary-condition for array types would be ideal.
But I'm guessing that's hard to do.

Is there anyone working on this over at TypeScript? This seems like a glaring hole in their type system.

[bradzacher]

But I'm guessing that's hard to do.

@Retsam already has a PR up #1534

Is there anyone working on this over at TypeScript? This seems like a glaring hole in their type system.

To my knowledge every single typed programming language makes the assumption that you've already done bounds checks before accessing an index.
So it's a hole in every single type system that has ever existed.

languages like C/C++ can either segfault, or just have undefined behaviour, and do no checks at compile time.
languages like C#/Java throw an explicit exception at runtime, and do no checks at compile time.
languages built on top of JS throw no exception - they return undefined, and do no checks at compile time.

You'd find that it's not done explicitly in any language because it pretty much impossible to solve, unless arrays are completely immutable always.
You would have to track the expected length of each and every array via control-flow analysis, and would effectively require runtime pointer tracking and analysis.

const array1 = [1, 2];
const array2 = ['a'];
function hasSomeSideEffect() {
  // clear array 1
  array1.splice(0, 10000);
}

function foo(arg: string[]) {
  // assume arg is length 0
  if (arg.length === 0) {
    return;
  }
  // assume arg is length 1 or more

  hasSomeSideEffect();
  // did the above function change the length of arg?
  // it's literally impossible to tell
  // if you pass `array1` into the function, then hasSomeSideEffect just cleared the argument
  // if you pass `array2` into the function, then hasSomeSideEffect did nothing

  // what should the length of arg be??
}

[Retsam]

The relevant issue is Typescript#13778. I do think this is something TS could do better on. (Oh hey, someone mentions this linter rule, neat!)

This is a common point of unsafeness in typed languages, but that's often because the program crashes (or does undefined behavior, C++) if you try to read outside of bounds, and there's no way to express that in the type system; but JS's out-of-bounds behavior of returning undefined is perfectly expressible.

In the above issue, they suggest, in the issues, changing the signature of array from:

interface Array<T> {
   [index: number]: T
   // Other stuff
}

to

interface Array<T> {
   [index: number]: T | undefined
   // Other stuff
}

(This is better than Array<T | undefined> because the signature of (e.g.) map is unchanged)

And you can theoretically just do that in a .d.ts file, but I'd really love to have that as a compiler flag.

This would treat all arrays as unknown length: cases where the array is a known length can probably be expressed with tuple types, (which wouldn't include | undefined), and there's always arr[x]! which would maintain the existing behavior.

---

Tangentially, there are some languages that at least offer safe APIs for indexing, (that return something like Maybe<T>)... but I also can't think of a mainstream language that doesn't have a "dangerous" API (and frequently only the dangerous API). It's just too common an operation for the languages to be willing to put too much ceremony around.

(Though there are "dependent type" language where stuff like the length of the array can be represented as part of its type... but those are still pretty experimental AFAIK)

[Retsam]

I've updated #1534 to handle this case. It was, (unsurprisingly), a little more complex than I expected, because I didn't initially consider how a single array access "infects" the rest of the chain.

arr[n]?.x // I knew this was going to be a problem
  ?.y // but this is also a problem
  ?.z // etc.

But overall it wasn't too bad.