MetaMask / MetaMask/eslint-config

Enable `@typescript-eslint/non-nullable-type-assertion-style`

Open
#348 1 comment 0 reactions 0 assignees View on GitHub
team-wallet-framework
Dominant language
JavaScript
Stars
12
Forks
21
PR merge metrics
No merged PRs in 30d

Description

## Motivation

We currently ban non-nullable assertions (`!`) completely with the ESLint rule `@typescript-eslint/no-non-null-assertion`.

However, there are arguments for using `!` assertions as a tool for improving type safety.

## Explanation

The ESLint rule `@typescript-eslint/non-nullable-type-assertion-style` provides alerts when a `as` assertion on a nullable type could be replaced with a `!` instead.

Enabling this rule wouldn't require disabling `@typescript-eslint/no-non-null-assertion`. We could still mandate ESLint disable comments when using `!` to make it clear that they should be used with caution and as an exception rather than the norm.

## In defense of `!`

### 1. `!` is safer than `as`

> `!` non-null assertions are generally preferred for requiring less code and being harder to fall out of sync as types change.
Source: https://typescript-eslint.io/rules/non-nullable-type-assertion-style/

`as` enforces a positive definition that specifies what a type is, while `!` applies a negative definition, specifying what a type isn't.

In other words, `as` prevents any type other than the asserted one from being applied, even if a more accurate one could be inferred due to code changes. `!` doesn't share this brittleness, as it allows any other type to be inferred in place of the existing one, only stepping in to strip `| undefined`, `| null` from the type.

For this reason, replacing `as` with `!` where applicable may represent a net improvement for type safety.

```
const example: string[] | undefined = ...
const bad = (example as string[]).pop();
const good = example!.pop();
```

### 2. TypeScript inference on nullability is limited

Some examples where the code guarantees the non-nullability of a variable, but TypeScript fails to infer this fact.

1. Optional chaining

```ts
while (queue[0]?.origin === originOfCurrentBatch) {
// `queue` and its head element are defined
const nextEntry = queue.shift()!;
}

while (queue.length > 0) {
// `queue` and its head element are defined
const nextEntry = queue.shift()!;
}
```

2. Array index access

```ts
if (chainIds.length > NETWORK_CACHE_LIMIT.MAX) {
const oldestChainId = chainIds.sort(
(c1, c2) =>
Number(this.state.chainStatus[c2]?.lastVisited) -
Number(this.state.chainStatus[c1]?.lastVisited),
)[NETWORK_CACHE_LIMIT.MAX];
// `oldestChainId` will always be defined, as `chainIds` is guaranteed to have at least `NETWORK_CACHE_LIMIT.MAX` elements
oldChainIds.push(oldestChainId!);
}
```

3. Index signature

```ts
const oldChainIds = Object.keys(chainIds).filter(
(chainId) =>
// `chainId` is of type `keyof typeof chainIds`, meaning `chainIds[chainId]` must be defined
chainIds[chainId]!.lastVisited < currentTimestamp - NETWORK_CACHE_DURATION,
);
```

4. Assertions made in a different conditional branch

```ts
/*
* @param options.currentVersion - The current version. Required if
* `isReleaseCandidate` is set, but optional otherwise.
*/
export async function updateChangelog({
currentVersion,
isReleaseCandidate,
tagPrefixes = ['v'],
}: {
currentVersion?: string;
isReleaseCandidate: boolean;
tagPrefixes: [string, string[]];
}) {
if (isReleaseCandidate && !currentVersion) {
throw new Error(
`A version must be specified if 'isReleaseCandidate' is set.`,
);
}
// Due to the first null check, if `isReleaseCandidate` is truthy, `currentVersion` should always be truthy
if (
isReleaseCandidate &&
await getMostRecentTag({ tagPrefixes }) === `${tagPrefixes[0]}${currentVersion!}` // `currentVersion` is inferred as 'string | undefined'
) {
throw new Error(
`Current version already has tag, which is unexpected for a release candidate.`,
);
}
...
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by locating the shareable ESLint configuration and the existing @typescript-eslint/no-non-null-assertion setting. Enable @typescript-eslint/non-nullable-type-assertion-style without removing the existing prohibition, then run the repository's lint checks and confirm the configuration accepts the rule.

Written by the indexing model from the issue text.

Assessment

Tech stack
eslint, typescript
Domain
tooling
Issue type
Feature
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.