Skip to content

Check index array length #38000

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
5 tasks done
mdbetancourt opened this issue Apr 16, 2020 · 18 comments
Closed
5 tasks done

Check index array length #38000

mdbetancourt opened this issue Apr 16, 2020 · 18 comments
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@mdbetancourt
Copy link

Search Terms

Indexing, Array, Strict check

Suggestion

Strict check to indexing a array

Use Cases

Safety with arrays usages

Examples

const arr: House[] = .... // same as [] | House[]
arr[0] // throw error
if(arr.length > 0) {
  arr[0] // OK
  arr[200] // throw error
}
const arr2: [House, House] = ...
arr[0] // OK
arr[1] // OK

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code (with a flag)
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@Kingwl
Copy link
Contributor

Kingwl commented Apr 16, 2020

let a = []
a[1000] = 1 
a.length // 1001

JS AWESOME🤷🏻‍♂️

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds labels Apr 16, 2020
@RyanCavanaugh
Copy link
Member

We go into this a bit at #13778. There isn't a huge amount useful that can be done here while still being sound given the existence of things like pop, push, and clear and the fact that those things can be invoked through indirect operations.

@rubenlg
Copy link

rubenlg commented May 28, 2020

push(), pop() and the other arguments provided in this thread did not prevent TypeScript from implementing tuples, even though these operations break tuples completely:

function foo(x: [number, number]) {
    console.log(x[2]); // Expected compiler error: no element at index '2'.
    x.push(5); // Should this even be allowed on a tuple?
    console.log(x[2]); // Still compiler error? Now there is index 2.
    x.pop(); x.pop(); // Should this even be allowed on a tuple?
    console.log(x[1]); // No problem here? Now length is actually 1
}
function bar(x: [number, number], y: number) {
    // Works, but should fail unless y is typed to be 0, 1 or 0|1
    // I'm ignoring this same as tuples ignore it too.
    console.log(x[y]); 
}

Assuming that tuples are still a nice addition to the language, we could leverage them to provide some incremental index safety here, e.g.:

function foo(x: number[]) {
  if (x.length > 0) {
    // New behavior: x becomes [number, ...number[]] rather than just number[].
    console.log(x[1000]) // New behavior, TS only allows indexing at 0.
  }
  if (x.length == 2) {
    // New behavior: x becomes [number, number]
    console.log(x[1000]) // TS error (already happens today for [number, number])
  }
}

So the idea would be:

  1. If you declare your array as a regular T[] array, you are on your own when indexing it.
  2. If you declare a tuple with a fixed length, you get strict index checking (already happens today).
  3. If you declare a tuple with some fixed params and a rest operator, you get strict index checking because you provide some knowledge to the compiler about the length.
  4. A check on the length acts as a type guard that transforms the array into a tuple (possibly with rest operator), enabling the checks in 2 and 3.

As an alternative, we could do this only for immutable arrays/tuples. You can't do push, pop or randomly write to those anyway.

Another alternative is to drop 4 (which can still be implemented with custom type guards), but at least implement 3, so we can do:

function hasOneOrMore<T>(x: T[]): x is [T, ...T[]] {
    return x.length > 0;
}

function foo(x: number[]) {
  if (hasOneOrMore(x)) {
    console.log(x[1000]) // Fails because of change 3 above.
  }
}

@Jamesernator
Copy link

I would definitely find even just basic cases working better than nothing. At current I use a custom helper that takes an array and returns if it's a tuple of a given length e.g.:

function isLength<T>(arr: T[], length: 0): arr is [];
function isLength<T>(arr: T[], length: 1): arr is [T];
function isLength<T>(arr: T[], length: 2): arr is [T, T];
// etc
function isLength<T>(arr: T[], length: number): arr is T[] {
    return arr.length === length;
}

I use it quite a lot as I'm also using --noUncheckedIndexedAccess, but it'd be super nice to be builtin so I can just use arr.length === 0 rather than using a custom function that only exists for the type system (or more accurately forgetting to use it, and then having an error that value could be undefined). Having <, <=, >, >= would be even better for things like const [first, ...rest] and so on.

@Kingwl
Copy link
Contributor

Kingwl commented Jun 6, 2021

@Jamesernator

Well... Just for fun:
image

playground

@Lonli-Lokli
Copy link

@Jamesernator I havent checked your sample but from what I see you cannot guarantee that array has at least 1 element, right? So your method should be named 'exactLength', instead of 'atLeast'

@JohnWeisz
Copy link

JohnWeisz commented Feb 8, 2023

This is more complex unfortunately

You can do this:

let arr = [0, 1, 2];

arr.length = 10;

arr[9] = 9;

And yet...

arr[7] === undefined // true

@rubenlg
Copy link

rubenlg commented Feb 8, 2023

I understand the complexity for mutable arrays. But what about ReadonlyArray? That one doesn't allow mutation (as long as you don't shut down the type checker with any, same as many other TS features).

One issue is that mutable arrays (potentially sparse by accident) are automatically converted to immutable, so this code is still problematic:

function firstElement<T>(x: readonly T[]): T {
  if (x.length === 0) throw new Error('Invalid argument');
  return x[0]; // Seems safe, we have checked length, and it's immutable.
}
let arr = [];
arr[2] = 1;
firstElement(arr); // Should not return undefined

However, given the amount of code I've seen that uses --noUncheckedIndexedAccess and does stuff like this:

if (x.length > 0) return x[0]!; // The compiler doesn't realize I already checked the length???

I'd say --noUncheckedIndexedAccess is already being misused, and is not as effective as it could be.

One possibility to bring this feature to ReadonlyArray in a safer way would be to disable the automatic conversion from mutable to immutable. That would mean that you either do a cast if you are convinced the array is not sparse, or you use an assertion or type guard function, for example:

function notSparse<T>(arr: ReadonlyArray<T|undefined>): arr is ReadonlyArray<T> {
  return arr.every(x => x !== undefined);
}
// This is fine, Array<T> can be assigned to ReadonlyArray<T|undefined> acknowledging that
// it is potentially sparse.
if (notSparse(arr)) {
  firstElement(arr);
}

But this is a breaking change (needs a flag, etc).

Of course, a better solution would be to change JavaScript to add non-sparse arrays to the standard (even if it's just a patch, similar to Object.freeze), which is what we want most of the time anyway. But I haven't seen any interest in this direction.

Alternatively, TypeScript could bridge the gap, with proposals like #24752. Although I acknowledge that this complicates the compiler even more, as it would have to check boundaries for both reads and writes, in subtly different ways.

In the meantime, the pattern I've developed with --noUncheckedIndexedAccess is to replace this:

if (x.length > 0) return x[0]!; // The compiler doesn't realize I already checked the length???

With this:

const first = x[0];
if (first !== undefined) return first;

No need to shut down the type checker, or use the ! operator, and it also works with dynamic indexing.

@RyanCavanaugh
Copy link
Member

@rubenlg just because the variable you have has type readonlyarray doesn't mean that someone else doesn't have a reference to the same value with a mutable type of binding, So the readonlyness of array tells you nothing about whether or not it's mutable in practice

@rubenlg
Copy link

rubenlg commented May 3, 2023

@RyanCavanaugh True, and that argument applies to objects as well, not just arrays. For example:

interface Mutable {
  x?: number;
}

interface Immutable {
  readonly x?: number;
}

function consumeNumber(x: number) {
  if (x === undefined) throw new Error('Undefined');
}

const MUTABLE: Mutable = {
  x: 2
};

const READONLY: Immutable = MUTABLE;

if (READONLY.x) {
  MUTABLE.x = undefined;
  consumeNumber(READONLY.x);
}

In both cases (objects and arrays), the automatic conversion from mutable to readonly is problematic. But in the case of objects, Typescript favors convenience, pretending the developer knows what they are doing when they declare it as inmutable.

@MDanialSaleem
Copy link

The above discussion makes sense as to why the length check is not enough for type narrowing, but what about when the index is known to contain a non null object because it has been retrieved from findIndex such as:

    const index = items.findIndex(item => somecondition);
    if (index !== -1) {
      items[index].children = //still get error;
    }

Although this can be easily replaced with this which does not throw any error, I am just curious as to why the first example does not work:

    const foundItem = items.find(item => someCondition);
    if (foundItem != null) {
      foundItem.children = // no error;
    }

@rubenlg
Copy link

rubenlg commented May 20, 2023

@MDanialSaleem Several reasons:

  1. Because findIndex only guarantees that the result is within the array length or -1, it doesn't guarantee anything about the contents of the array, which can still be undefined.
  2. Because index is just a regular number. It doesn't matter that it came from findIndex. When you use it to index the array, it's the same as any other number.

So in the end, it has the exact same problems as doing a bounds check on any other index. Here is an example in which your first code snippet could have a bug, and the compiler is catching it:

const x: number[] = new Array(10); // Sparse array, all 10 elements are undefined
const index = x.findIndex((x, i) => i == 2);  // returns 2
console.log(x[index].toFixed(2)); // Typescript complains, and it's actually catching a bug

Of course, you can write other callbacks for findIndex that successfully guard against undefined values. But Typescript can't statically analyze the findIndex callback implementation to understand whether it guards against undefined values or not.

@MDanialSaleem
Copy link

Thanks @rubenlg for the detailed answer.

Of course, you can write other callbacks for findIndex that successfully guard against undefined values. But Typescript can't statically analyze the findIndex callback implementation to understand whether it guards against undefined values or not.

Yes, I was referring specifically to this case. So TS cannot, right now, infer if the parameter passed to a function is not null by statically analyzing the body of the function, but can it not be implemented, potentially using the same mechanism that TS uses to infer return types?

Although, I do not think there will be many valid usecases of this, considering that the second example in my original question is a very valid way of doing things.

@typescript-bot
Copy link
Collaborator

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

@demurgos
Copy link

demurgos commented Dec 2, 2023

If/once the record and tuple support lands, narrowing based on a native tuple length should become possible.

@GermanJablo
Copy link

@RyanCavanaugh I think @rubenlg made good points that weren't answered.

Namely, why are restrictions imposed on arrays that are not imposed on tuples or objects?

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Feb 7, 2025

Trade-offs can differ in their weight by scenario. Arrays, for example, have many built-in methods that mutate them; objects do not.

@GermanJablo
Copy link

GermanJablo commented Feb 7, 2025

And what do you think about something like what @jcalz suggested here:
#40316 (comment)

(I just left a comment at the end of that thread as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests