[HOLD for payment 2024-10-10] [$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: @adelekennedy

[melvin-bot]

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

[melvin-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).

[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

1. Incrementally update tsconfig.uncheckedindex.json, module by module, until in includes all files
2. 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]

⚠️ 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.

[melvin-bot]

This issue has not been updated in over 15 days. @ShridharGoel, @roryabraham, @adelekennedy, @eh2077 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[roryabraham]

At this point, the last PR has been dragging on too long without enough proactive or responsive communication. As such, if it is not merged within the next one or two days, we are going to reassign this issue

[melvin-bot]

@ShridharGoel, @roryabraham, @adelekennedy, @eh2077 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@ShridharGoel, @roryabraham, @adelekennedy, @eh2077 Huh... This is 4 days overdue. Who can take care of this?

[zfurtak]

As it was decided in the slack discussion I will take over this issue

[ShridharGoel]

Hi, this task is almost complete. The only thing to look into is the perf test issue (which seems to be happening only on CI), so I'm not sure if another assignment is needed.

[melvin-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.

[melvin-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.

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.43-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Add prefer-at condition to eslint #49534

If no regressions arise, payment will be issued on 2024-10-10. 🎊

For reference, here are some details about the assignees on this issue:

• @ShridharGoel requires payment (Needs manual offer from BZ)
• @zfurtak does not require payment (Contractor)
• @eh2077 requires payment through NewDot Manual Requests

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@eh2077] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@adelekennedy] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.