[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]

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

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @ShridharGoel paid $500
• Contributor: @zfurtak is from an agency-contributor and not due payment
• Reviewer: @eh2077 owed $500

BugZero Checklist (@adelekennedy)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[adelekennedy]

@ShridharGoel & @eh2077 sent you offers in Upwork

Job Posting

[eh2077]

@adelekennedy Thanks for checking payment of this task. I'm not sure if we're eligible for the full payment of this job because the task was reassigned in the middle of the last PR, see here #43055 (comment)

I think it'll be better to wait for @roryabraham 's input before issuing the payment.

[adelekennedy]

Oh thank you @eh2077 I totally missed that (this issue has gotten pretty dense) Rory is out on parental leave so I'm making a call based on all the comments above - I think an even split here may be the most fair between you and @ShridharGoel?

[eh2077]

Oh thank you @eh2077 I totally missed that (this issue has gotten pretty dense) Rory is out on parental leave so I'm making a call based on all the comments above - I think an even split here may be the most fair between you and @ShridharGoel?

@adelekennedy Personally, the payment of $500 is fair. Thanks!

Just one more question, can I request payment through NewDot instead?