[no-unnecessary-condition] ignore array index return

[astorije]

Consider the following snippet:

const foo = ['a', 'b', 'c']; // type: string[]
const bar = foo[5]; // type: string, value: undefined

// Passes the rule
if (foo[5]) {
  console.log(foo[5]);
}

// Fails the rule
if (bar) {
  console.log(bar);
}

Thanks to @Retsam's work on #1534, the first example passes the linter, however as the author mentions in their PR, it doesn't handle less trivial cases, such as storing the value in a variable (which is very common when you want to use the value being tested in the body of the if).

I realize that supporting this (and other advanced) cases might be a lift. In the meantime, or alternatively, any thoughts on an option to entirely ignore arrays? In the current state, I don't believe there is a way to make the second example above pass the linter without disabling the rule or getting rid of the variable.
I believe such option was mentioned in #1544, for reference.

Versions

package	version
@typescript-eslint/eslint-plugin	2.24.0
@typescript-eslint/parser	2.24.0
TypeScript	3.8.3
ESLint	6.8.0
node	12.14.0
npm	6.13.4

[bradzacher]

It would not really be possible, no.

From the POV of typescript, a string is a string. Doesn't matter where it came from.
So for TS, there's no difference between const x = 'a' and const y = foo[5] - they both just have a string type.

Which means in order to detect this, the rule would have to do control-flow analysis of a variable to determine where the value in any variable has come from.
In some cases this might be simple to do, but it very, very quickly degrades into an intractable problem to solve (see some examples below).

There's but one case that's "easy" to solve, and that is a const variable whose value was directly assigned from an array index:

const bar = foo[5];
if (bar) {}

Happy to accept a PR if you want to support this case, otherwise I'd say that it's not possible to do.

---

const foo = ['a', 'b', 'c'];
const bar = foo[5];

if (bar) {} // "relatively" easy to detect, as there has been exactly one assignment.

let bam = foo[5];

bam = 'a';
if (bam) {} // hard to tell that if the value is an array

if (someCondition) { bam = 'a'; }
if (bam) {} // impossible to know if the value came from an array

const buz = foo[5];
const bang = buz;
if (bang) {} // really, really, really hard to know if the value came from an array

test(bar);
function test(baz: string) {
  if (baz) {} // impossible to detect
}
 

[astorije]

Thanks @bradzacher, yet another amazing piece of feedback, very helpful :)
It's very interesting to learn more about the dependence on the language, the limitation or capabilities of various things, etc.

In the examples above, this is also an acceptable alternative and satisfies the linter:

const foo = ['a', 'b', 'c']; // type: string[]

// Passes the rule
if (5 in foo) {
  console.log(foo[5]);
}

(Or course it's a little silly like this but imagine a dynamic index)

As we go through all our projects one by one, we'll see if other cases arise. Should the format described above come up too often, I'll see if I can look at improving the linter.

[astorije]

Following-up on this, it would also be great to be able to ignore Records as well. We are having the following case:

In our own code, we have something of the sort:

if (!client.getResolvers().Foo?.id) {
    client.addResolvers({
      Foo: {
        id: /* ... */,
      },
    });
}

client is controlled by a third-party library, which defines getResolvers() return type as:

[key: string]: {
  [field: string]: { /* ... */ };
};

In other parts of the code, we have been able to rewrite the value of the record as [field: string]: { /* ... */ } | undefined; but since this is a third-party dependency, we do not have this luxury here.

Because client.getResolvers().Foo is non-nullable type-wise, the rule flags the extra ? as unnecessary, which is not true.

I think having one or multiple options to ignore things like this, lookups that can be undefined even though TS typing indicates it is not, would make things safer, as following the linter recommendation breaks the code here. What do you think?

[bradzacher]

The problem is that that that option would add a lot of unsafety in the rule, as you'll no longer get warned about records within your control.
I don't think that makes it a good option.

A workaround is to inline the call and apply a custom type:

declare const client: {
    getResolvers(): Record<string, Record<string, number>>;
    addResolvers(resolver: Record<string, Record<string, number>>): void;
};

type SafeResolversRecord = Record<string, Record<string, number | undefined> | undefined>;

const resolvers: SafeResolversRecord = client.getResolvers();
if (resolvers.Foo?.id) {
    client.addResolvers({
        Foo: {
            id: 1,
        },
    });
}

depending on how the module defines its types, you could even augment this type into the module itself to make it easier to consume:

import "foo";
declare module "foo" {
  export type SafeResolversRecord = Record<string, Record<string, number | undefined> | undefined>;
}

////////

import { SafeResolversRecord } from 'foo';

[astorije]

That's really great advice, thank you @bradzacher!

[sindresorhus]

microsoft/TypeScript#13778 would fix this, right?

[bradzacher]

Yup! It would definitely fix this.
This is a problem for all index access in TS, and is a huge source of unsafety in codebases.
TBF - flow (which prioritises type soundness above all else) has this same problem. In this case, both systems just prioritise the devx over safety for this.

Because of #1534, we handle the trivial to detect case of if (arr[0]) {} (the rule detects that arr is an array type, and ignores the condition on it).
But it doesn't currently handle any more cases (like assigning to a variable).

We can use scope analysis to handle this case:

const x = arr[0];
if (x) {} // detectable

Any other cases mentioned in #1798 (comment) get very fuzzy and likely to false positive/negative.

[jmicco]

In addition to array types, this also causes problems for Map index if I have a type like:

export interface ITargetEntry {
    repo: string
    path: string
}
export interface ITargetMap {[target: string]: ITargetEntry}
const targetMap: ITargetMap = {...}
const targetEntry: ITargetEntry = targetMap[product]
if (targetEntry) {
...
}

The index into the map can certainly be undefined - but the no-unnecessary-condition flags it - this is not an array index it is an index into a string map.

[Retsam]

@jmicco Yeah, that's a well-known limitation as well. Unlike the array case, that one has a pretty good workaround, though. It's better to declare your map in a way that indicates that keys might not exist:

interface ITargetMap {
    [target: string]: ITargetEntry | undefined
}
// Or `type ITargetMap = Partial<Record<string, ITargetEntry>>` for short

[jmicco]

Hi @Retsam, I thought I tried that and it did not work - my apologies, that fix did prevent the complaint, although it seems like the type imputation for such a hash needs to generate the type as "x | undefined", but I understand that is the problem for the typescript people and not for this rule.

…

On Mon, Jul 27, 2020 at 3:44 PM Retsam ***@***.***> wrote: @jmicco <https://github.com/jmicco> Yeah, that's a well-known limitation as well. Unlike the array case, that one has a pretty good workaround, though. It's better to declare your map in a way that indicates that keys might not exist: interface ITargetMap { [target: string]: ITargetEntry | undefined}// Or `type ITargetMap = Partial<Record<string, ITargetEntry>>` for short — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1798 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABSTB6AVM7JI2CXTRHIXBXLR5X7LFANCNFSM4LTVENYA> .

[JoshuaKGoldberg]

As a side note, --noUncheckedIndexedAccess introduced in TypeScript 4.1 addresses the original case. But that's a very strict compiler setting and it's reasonable that folks would want to apply this lint rule instead.

[bradzacher]

The best solution we could build in here would be to add some scope analysis to the rule to help it understand where a value came from. That would let us automatically handle some more cases that we currently ignore when it's inline in the condition.

Going forward I would definitely agree that everyone should just be using noUncheckedIndexedAccess. Your code is as safe as your types are, and that compiler option makes your types closer to the truth.
It can be a bit cumbersome - but better to have a few extra provably unnecessary checks than be missing unknowingly necessary checks.

This rant is a bit off topic so I've put it behind a spoiler block - but I just wanted to highlight to people why having "provably unnecessary" checks in your codebase isn't a problem as a motivating case for why you should migrate codebases to noUncheckedIndexAccess.

Facebook uses flow which has a lot more strictness than TS does. This leads to a lot of "provably useless" checks, but it leads to ultimately safer code.

For example - for type refinements flow invalidates refinements after a function call because it can't provably know if that function call invalidated the refinement:

function foo() {
  const obj: { prop: number | null } = { prop: 1 };
  return {
    clear() {
      obj.prop = null;
    },
    obj,
  };
}

const result = foo();
if (result.obj.prop != null) {
  // typeof result.obj.prop === number
  result.clear();

  // in TS   - typeof result.obj.prop === number
  // in flow - typeof result.obj.prop === number | null
  result.obj.prop.toFixed();
}

ts repl
flow repl

This leads to a lot of code patterns you only see inside flow codebase - like re-asserting refinements after a function call, or pre-assigning a value before a function call.

Additionally flow does not have a "non-null assertion" operator, and the type system doesn't allow you to manually "cast" a nullish type to a non-nullish type like TS does. Why? Because it's inherently unsafe and can easily lead to bugs!

declare function foo(): number | null;

const x = foo();

// TS allows both of these "unsound" type operations
x!.toFixed();
(x as number).toFixed();

// flow does not
(x: number).toFixed();

ts repl
flow repl

This again means that if you've got a null which you know can't exist there - you have to add a runtime check to clear it (or reach for an any to cast through).

The FB codebase is larger than any codebase you will ever work on, I guarantee. It has many, many "unnecessary" checks within the code for the sake of "making flow happy", and it does not harm the maintainability or runtime performance of the system.

The safety has been deemed to be worth the verbosity, and it ultimately leads to fewer runtime crashes.

[publicvirtualvoid]

I really enjoy this rule because it makes tidying up after a refactor a lot more effective, but the situation at the moment is a little tricky to manage.

noUncheckedIndexedAccess is not particularly smart and doesn't understand simple for loops. It can't be disabled per file/line like a lint rule, and enabling it on a large existing project can produce a huge number of build errors. Putting the necessary guards in place to resolve the errors would make some code considerably more convoluted.

Could something like noUncheckedIndexedAccess theoretically exist as an eslint rule such that it can be disabled in certain contexts?

[bradzacher]

theoretically - yes - it could be implemented.
practically - in order to implement it, you would have to do a bunch of work to rebuild everything TS does internally. It would be pretty complicated and a lot of work because you need to implement control-flow analysis as well so that you can understand like:

const x: number[] = [];
if (x[0]) {
  x[0].length; // is "safe" due to the condition 
}

Which is really the most complicated part of it. You'd ultimately just be rebuilding a chunk of TS

[ddhorstman]

I'd be interested in updating this rule to cover the most trivial additional case, as I've run into this issue in my own codebase:

const x = arr[0];
if (x) {} // detectable

However, I've never worked on linter rules before, and I'm not sure where to start. It sounds like this is detectable via "scope analysis" - could anyone offer advice on how I might be able to learn how to do this?

[bradzacher]

@ddhorstman - I believe we already have handling for this simple case

[frontsideair]

I'm having trouble with false negatives and this is the only issue I could find on this topic. I can open a separate one if needed.

Here's the repro.

Copied here for convenience:

declare const actionData: {
    email: string | string[];
    password: string | string[];
} | undefined

export const result = actionData?.email?.[0];
                                    // ^^ -> ts-eslint thinks this is unnecessary

I have TS noUncheckedIndexedAccess option turned on in my project so this should not be unnecessary, and it causes an error in production like so: Cannot read properties of undefined (reading '0')

[bradzacher]

@frontsideair you can access string characters using array index notation. So your optional chain does nothing - the report is correct.

You need to write stricter code using either typeof or Array.isArray.

But either way - according to your runtime error your types are simply incorrect. We can only act on the types you provide - string | string[] is not a nullish type, which doesn't line up with your runtime error at all.

[frontsideair]

First of all, thanks for taking the time to look into this.

However I would never expect ts-eslint to work with the types I didn't provide, but the type I provided is not string | string[], it's string | string[] | undefined, as can be seen from the screenshot.

Also I removed the string from the union and it was still reported as unnecessary, here's the playground.

I would expect this rule to not report the last optional chain as unnecessary.

[bradzacher]

@frontsideair that's not how optional chaining works!

Based on your types actionData?.email[0] may return undefined if and only if actionData is null or undefined.
Optional chaining is a short-circuiting operator. That means that it stops evaluating the chain if it's undefined.

Put another way, it's equivalent to this code:

// actionData?.email[0] is the same as
let result = undefined;
if (actionData != null) {
  result = actionData.email[0];
}

Note how the .email and [0] is only evaluated if and only if the chained object is non nullish?

Whilst the result of actionData?.email may be undefined, that is not because the email property may be undefined. If you inspect the return type of actionData?.email[0] you'll note that it's also possibly undefined. Again this is because of the first optional chain.

To further illustrate, if you were to write your unnecessary code (based on your types), you would end up with this equivalent code:

// actionData?.email?.[0] is the same as
let result = undefined;
if (actionData != null) {
  if (actionData.email != null) {
    result = actionData.email[0];
  }
}

See now how that second optional chain is truly unnecessary based on your types?
The rule is correctly flagging your code based on your declared types.

[frontsideair]

Yes, you are absolutely correct. I was confused because the types I'm actually working were coming from a library and it seems to be wrong. I simplified them as they were shown and got it wrong by extension. I'll try to open an issue with the library itself, thanks for clarifying!