Skip to content

Array access possibly undefined with noUncheckedIndexedAccess despite findIndex #61388

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Sainan opened this issue Mar 10, 2025 · 12 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@Sainan
Copy link

Sainan commented Mar 10, 2025

πŸ”Ž Search Terms

noUncheckedIndexedAccess, findIndex, undefined

πŸ•— Version & Regression Information

Tried 5.8.2 and nightly (5.9.0-dev.20250310).

⏯ Playground Link

https://www.typescriptlang.org/play/?noUncheckedIndexedAccess=true&ts=5.8.2#code/MYewdgzgLgBAhgJwQLhmArgWwEYFMEDaAujALwwECMANDAEy0DMRA3AFCiSwCWAJgB5l4SAHQAzbmF4BJKbn4AKQaQB8MZeToBKdtzEwFfQQEJyAWkpaYAbzYx7MTtHWoMOfEMSEjrOw6cgADa4IoEgAOZKOmwAvmxAA

πŸ’» Code

const arr: number[] = [1, 2, 3];
const idx = arr.findIndex(x => x == 2);
if (idx != -1) {
    const x: number = arr[idx];
    console.log(x);
}

πŸ™ Actual behavior

Type 'number | undefined' is not assignable to type 'number'.
  Type 'undefined' is not assignable to type 'number'.

πŸ™‚ Expected behavior

Code 'compiles' without errors.

Additional information about the issue

No response

@MartinJohns
Copy link
Contributor

This is working as intended. findIndex() does not narrow the type of arr, which would be necessary to allow unchecked index access.

@Sainan
Copy link
Author

Sainan commented Mar 10, 2025

Well, of course it doesn't narrow the arr, but it guarantees that idx == -1 || (idx >= 0 && idx < arr.length), and given that I checked idx != -1, it is now guaranteed that idx is in range of the array, which because it is number[], should yield that x is number and not number | undefined.

@Sainan
Copy link
Author

Sainan commented Mar 10, 2025

If you consider this too complex, there's also the simpler case of

const arr: number[] = [1, 2, 3];
const idx: number = ...;
if (idx >= 0 && idx < arr.length) {
    const x: number = arr[idx];
    console.log(x);
}

which fails with the same error.

@MartinJohns
Copy link
Contributor

It's just a limitation of how TypeScript is designed. It needs to narrow the type, otherwise the check does not work. There's no hardcoded special behavior for findIndex(), it's just a method returning a number, nothing indicates that retrieving this value and checking it means the index access is valid.

I suggest you to write the code this way:

const arr: number[] = [1, 2, 3];
const idx: number = ...;
const x: number | undefined = arr[idx];
if (x !== undefined) {
    // ...
}

@Sainan
Copy link
Author

Sainan commented Mar 10, 2025

What I'm saying is that there's guarantees about the range of idx which influence how indexing an array with the number will behave.

I don't think it's that much different from narrowing a range of possible string values, e.g. like here:

type Rarity = "COMMON" | "UNCOMMON" | "RARE";
const probabilities: Record<Exclude<Rarity, "RARE">, number> = { COMMON: 80, UNCOMMON: 20 };
declare function getKey(): Rarity;
const key: Rarity = getKey();
if (key != "RARE") {
    const probability: number = probabilities[key];
    console.log(probability);
}

@MartinJohns
Copy link
Contributor

I don't think it's that much different from narrowing a range of possible string values, e.g. like here:

Completely different case. Here the key can be narrowed to "COMMON" | "UNCOMMON", and according to the type of probabilities it always has a COMMON and an UNCOMMON property, so accessing is valid.

What I'm saying is that there's guarantees about the range of idx which influence how indexing an array with the number will behave.

And I'm telling you: The compiler doesn't know about this guarantee. There's nothing about the signature of findIndex() that lets the compiler know that the idx is valid to access.

@Sainan
Copy link
Author

Sainan commented Mar 10, 2025

Well, in the case of if (idx >= 0 && idx < arr.length), the guarantee is given by the branch. Updating findIndex typings to provide that guarantee can be done later.

@MartinJohns
Copy link
Contributor

Well, in the case of if (idx >= 0 && idx < arr.length), the guarantee is given by the branch.

This can't be done either. length is just a number property, nothing about it indicates that a later indexed access will succeed if checked. See #38000.

You need to think about it on a type level, not on a runtime level. You see the code and think yourself "yeah, of course this will be fine when running", but how would the compiler know about this by looking at the types?

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 10, 2025
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 10, 2025

I would recommend turning off noUncheckedIndexedAccess if you're always confident that your accesses are in-bounds. It is intentionally a blunt instrument and we don't intend to add new kinds of complex CFA which would weaken its soundness.

#13778 (comment)

Not directed at OP: I kept trying to warn people this would happen 🫠

Image

Image

@Sainan
Copy link
Author

Sainan commented Mar 10, 2025

FWIW, I personally am only looking into adoption of this flag to reduce the number of false-positives I'm getting from eslint's no-unnecessary-condition rule which is otherwise very optimistic about e.g. indexing on Record<string, ...> always being a hit, and on that side of the ecosystem they also have no interest in making the tooling more useful/eliminating false-positives. See typescript-eslint/typescript-eslint#10921.

@JoshuaKGoldberg
Copy link
Contributor

on that side of the ecosystem they also have no interest in making the tooling more useful/eliminating false-positives

Apologies for the drive-by nitpicking, but: we do have interest! We're just blocked from doing anything ourselves. The situation is that we directly use TypeScript's inference. Anything like the requested "do this one specific case better" would need to be solved at the TypeScript level. Asking typescript-eslint to do better here is like asking your coffee mug to give you fresher coffee beans. It's a delivery method, not the roaster itself. πŸ˜›

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

5 participants