[$1000] Enable the noUncheckedIndexedAccess TS compiler option

[roryabraham]

Slack proposal: https://expensify.slack.com/archives/C01GTK53T8Q/p1717468504413819

Problem

We currently have crash occurring on production. The problematic code can be summarized with a minimal example:

type MyType = {
    something: string;
};

const myArr: MyType[] = [];

// myItem is inferred to have type MyType, when really it is undefined 
const myItem = myArr[42];

// No compiler error, but the app will crash
console.log(myItem.something);

As you can see, we unsafely indexed an array, and then assumed the result was defined. Then trying to access a property of undefined, we experience a crash.

Solution

Enable the noUncheckedArrayAccess TypeScript config. With that config enabled, the type of myItem is correctly inferred to MyType | undefined, and we get a compiler error when trying to access myItem.something, preventing the crash at compile time.

Issue Owner

Current Issue Owner: @ShridharGoel

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eh2077 (External)

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[roryabraham]

It's worth noting that when enabling this rule we get more than 1000 errors across more than 200 files, so in order for me to accept a proposal it would need to include a rollout plan so we can incrementally adopt this new rule across a subset of files at a time.

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Enable the noUncheckedIndexedAccess TS compiler option.

What is the root cause of that problem?

New change.

What changes do you think we should make in order to solve the problem?

We need to enable the noUncheckedArrayAccess TypeScript config.

{
  "compilerOptions": {
    // other compiler options
    "noUncheckedArrayAccess": true
  }
}

Once noUncheckedArrayAccess is enabled, TypeScript will infer the type of an array access expression as including undefined. For example, accessing an element of an array myArr: MyType[] at an arbitrary index will be inferred as MyType | undefined.

On enabling:

type MyType = {
    something: string;
};

const myArr: MyType[] = [];

// myItem is inferred to have type MyType | undefined
const myItem = myArr[42];

// Compiler error: Object is possibly 'undefined'
console.log(myItem.something);

Add a unit test:

test('array access should handle undefined values', () => {
    const myArr: MyType[] = [];
    const myItem = myArr[42];
    expect(myItem).toBeUndefined();
});

Incremental adoption:

Add it only to some files or some modules can be done by using a new tsconfig.incremental.json.

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "noUncheckedArrayAccess": true
  },
  "include": [
    "src/module1/**/*.ts",
    "src/module2/file1.ts"
  ]
}

One by one we can keep adding modules. Once most of the files are updated, we can include the check in tsconfig.json.

[eh2077]

@ShridharGoel Thanks for your proposal.

How will the new tsconfig.incremental.json cooperate with npm run typecheck which uses tsconfig.json from the root directory? Is it possible to achieve the goal by editing the tsconfig.json from project root directory?

[ShridharGoel]

We can use something like tsc -p tsconfig.uncheckedindex.json as the command (using uncheckedindex in the name now).

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[roryabraham]

We can use something like tsc -p tsconfig.uncheckedindex.json as the command (using uncheckedindex in the name now).

That sounds reasonable. So the rollout plan would look something like this:

1. Create a new tsconfig.uncheckedindex.json that only applies the rule to some reasonable subset of files.

2. Update npm run typecheck to do this:

   tsc && tsc -p tsconfig.uncheckedindex.json

3. Incrementally update tsconfig.uncheckedindex.json, module by module, until in includes all files
4. Add noUncheckedArrayAccess to the base tsconfig.json, remove tsconfig.uncheckedindex.json and restore npm run typecheck to the way it is now.

sounds good, the only other thing I'd like to figure out is how we want to break it up into reasonably-sized PRs. Some of these might be a bit trickier than your standard TS migration, because it actually involves changing runtime code.

Going to assign this one a bounty of $1000 because it's going to be a few PRs.

[melvin-bot[bot]]

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

[roryabraham]

@ShridharGoel I look forward to reviewing the first PR.

[ShridharGoel]

Planning to start on this within 12 hours (its quite late in the night for me as of now).

[roryabraham]

Just as a heads up, @blazejkustra and @fabioh8010 ended up objecting to the addition of this rule. @fabioh8010 has an alternate suggestion that he'll propose - using .at() instead of [] to index arrays (since it has the same effect and gives us checked indexed access of arrays in particular.

we may want to pause here for further consensus

[fabioh8010]

Posted the proposal here.

[ShridharGoel]

Oh okay, I had invested quite some time into this since a lot of changes had to be done (it wasn't complete yet though). Should I start working on the ESlint check (using at()) that was discussed on Slack?

[fabioh8010]

@roryabraham Did you have a chance to look at the .at() proposal?

[roryabraham]

Sorry for the delay here, let's switch to the .at() proposal 👍🏼

[fabioh8010]

Nice @ShridharGoel ! So the first step would build a rule ESLint rule to enforce the use of the Array .at() method instead of direct access with array[], more details here.

It's important that the ESLint rule should be able to not only identify array[] usage and suggest .at() instead, but also be able to auto-fix those situations. The rule should be created on https://github.com/Expensify/eslint-config-expensify.

Please let us know if you have any further questions, thanks!

[ShridharGoel]

@fabioh8010 @eh2077 https://github.com/Expensify/eslint-config-expensify/pull/104

[fabioh8010]

@ShridharGoel I tried to review and test today but I'm having some local issues with the tests, I will let you know once I'm able to fully review the PR.