Add noUncheckedIndexedAccess TypeScript compiler flag

[rekmarks]

This PR adds the noUncheckedIndexedAccess compiler option to tsconfig.json. This flag forces us to check if the values of index signature keys are defined before working with them. For details, see the tsc documentation.

It is no exaggeration to say that much of our object type safety is mere theater without this flag. Without it, the following is valid TypeScript:

const obj: Record<string, unknown[]> = {}
const val = obj['lmao']
val.push('yolo') // no complaint

I advocate that we end this insanity now, and enable noUncheckedIndexedAccess everywhere. Unsurprisingly, our controllers contain the most violations of this rule, but only about 60 all in all: MetaMask/core#554.

The main drawback of enabling this flag is that we have to add non-null assertions or unnecessary if statements to some parts of our code, because TypeScript generally can't map enumerated keys to their objects. For example:

const obj: Record<string, string[]> = getMyObject()
for (const key of Object.keys(obj)) {
  const val = obj[key] // "val" is: unknown[] | undefined
  val.push('foo') // error
}

However, trading convenience for type safety is the value proposition of TypeScript, so that's life, I guess.

Fixing the above line and keeping our linter happy is as simple as:

const obj: Record<string, string[]> = getMyObject()
for (const key of Object.keys(obj)) {
  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
  const val = obj[key]! // Note the postfix bang. "val" is now: unknown[]
  val.push('foo') // success!
}

[Gudahtt]

This seems good, but I'm wondering if waiting on TypeScript v4.4 would make this easier 🤔 Because of the better handling of unset v.s. undefined properties.

[rekmarks]

Hm, I suppose we can wait two weeks, although I haven't considered how much easier it'll be. This comment from the PR that implements --strictOptionalProperties (they renamed it to something else but I forget what) seems to suggest that they complement one another, and I suppose it's an opportunity to do all of the resulting refactors at once, if nothing else.

[Gudahtt]

I was hoping this would let us avoid the null assertion operator in some of the cases where this change forced its introduction. Null assertions are escape hatches that we should try to avoid if possible. If we start sprinkling them everywhere for this rule, we likely won't have reason to come back to them anytime soon.

[mcmire]

I am not sure how I feel about this. I mean, I understand the root problem here, but I don't mind how TypeScript has solved this problem. noUncheckedIndexedAccess doesn't feel like a satisfying alternative to me, but I'd have to play around with it.

[Gudahtt]

This seems like a pretty clear case of there being a complete lack of type safety when it comes to index access, with this flag turning it back on. I don't understand what you mean about liking how TypeScript has solved this problem absent this flag... what problem are you referring to? How is it solved?

[mcmire]

@Gudahtt Sorry, I wasn't as accurate with my wording as I could have been. I was going to respond with something that emphasized the caveat that Erik mentioned — that I am concerned that this would add obstacles to both writing TypeScript and reading TypeScript — in that we would need to make a decision about what is real on behalf of TypeScript, and communicate the reasons we are overriding TypeScript in this way — which I feel may be unnecessary (practically, not theoretically, based on the code we tend to write). But after reviewing the violations that this would cause in controllers, I realized that there is one issue that would be worth catching: using the result of findIndex or indexOf to access a value from an array without first checking that that index is not -1. It appears we do this more times than I would like. I will say that I am a bit disappointed in the coarseness of this option — there are a couple of cases I've seen where I think TypeScript could be smarter, and I think this option brings those weaknesses out — but perhaps that will get better in the future.

So I've changed my mind on this. I regret that we have to do this, but it seems more beneficial than not.

[Gudahtt]

I should clarify that I expect most of the caveats mentioned by Erik will no longer be a problem after the exactOptionalPropertyTypes flag is enabled, which is very beneficial for type safety on its own. This is what I was alluding to when I mentioned TypeScript 4.4, as that's when the flag was added (not sure that I understood at the time that it was opt-in).

I would prefer that we not merge this until after the exactOptionalPropertyTypes flag is enabled, and not until we've tried both flags on a sizable repo (such as @metamask/controllers). I have a draft PR up now for enabling exactOptionalPropertyTypes but it's blocked by a dependency: MetaMask/core#796

[Gudahtt]

LGTM! exactOptionalPropertyTypes has been enabled so I don't have any more reservations about this change introducing too much friction. Though I guess we'll see.