Skip to content

A length>0 on a readonly array should guarantee destructured element is known, not undefined #42909

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
cefn opened this issue Feb 21, 2021 · 4 comments
Closed
5 tasks done
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@cefn
Copy link

cefn commented Feb 21, 2021

Suggestion

πŸ” Search Terms

Array Destructuring, noUncheckedIndexedAccess, length check

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • 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, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

A length check with length more than zero should eliminate undefined from possible values when destructuring.

Looking at the example below, it's impossible that destructuring [unshifted,...rest] from the array could lead to unshifted being undefined, since the length is checked first. Nevertheless when noUncheckedIndexedAccess is set in Tsconfig, it treats undefined as a possible path.

function unshiftOrNothing(items:number[]) : number{
    if(items.length > 0){
        const [unshifted, ...rest] = items;
        return unshifted;
    }
    return 0;
}

πŸ“ƒ Motivating Example

I have encountered this a lot when manipulating 'immutable' arrays e.g. for queues or redux state which require the use of destructuring, since array methods change the array.

I can't identify a type-safe alternative or check which ensures that the typescript compiler picks up on the fact the first item must be set.

πŸ’» Use Cases

A workaround is to lie to the compiler that items is a tuple containing at least one item. However, I don't know what knock-on effect that has on e.g. type guarantees about ...rest - it's presumably expected to be a zero-length tuple which might break other things.

function workaround(items:number[]) : number{
    if(items.length > 0){
        const [unshifted, ...rest] = items as [number];
        return unshifted;
    }
    return 0;
}
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Feb 22, 2021
@RyanCavanaugh
Copy link
Member

noUncheckedIndexedAccess gives the highest-possible level of array safety, and we still can't be sure about destructurings as soon as there's any code between the check and the destructuring

function unshiftOrNothing(items:number[]) : number{
    if(items.length > 0){
        console.log("the result of clearing the array was " + clearItemsArray());
        const [unshifted, ...rest] = items;
        return unshifted;
    }
    return 0;
}

We don't intend to special-case the exact form of there being no in-between code between the length check and the destructuring.

@cefn
Copy link
Author

cefn commented Feb 24, 2021

I didn't make clear that for the typical immutable case, flagging it readonly would be straightforward or even preferred.

Nevertheless with indexed access checks, the compiler considers that the readonly array below could potentially be empty after a >0 zero length check. Would welcome other workarounds or ways to nudge the compiler but I haven't found any which have an effect other than casting (potentially unsafe if I missed some case - as you drew attention to).

function unshiftOrNothing(items:ReadonlyArray<number>) : number{
    if(items.length > 0){
        const [unshifted, ...rest] = items;
        return unshifted;
    }
    return 0;
}

@cefn cefn changed the title A non-empty array check should guarantee first destructured element is known type A length>0 on a readonly array should guarantee destructured element is known, not undefined Feb 24, 2021
@RyanCavanaugh
Copy link
Member

Readonly is not immutability. You can have a readonly view of an array which is itself mutable.

@cefn
Copy link
Author

cefn commented Feb 25, 2021

Thanks, Ryan!

I had misunderstood how the readonly marking was managed. I misinterpreted compilation errors I've encountered before around readonly. You've taught me an array which typescript treats as read/write in one context could become readonly within a different one without bypassing typescript.

I knew these are just javascript arrays and obviously mutable under the hood but hadn't recognised the situations where writing was possible actually from within typescript. Thought readonly arrays which had no other references, or which had arrived with the flag already were the only ones the compiler would accept as readonly, making them 'compile-time safe'. This was a mistake.

For example in this code sample I thought adding the export to the const mutableArray would prevent using it as an argument to immutablePush as it was no longer guaranteed to be unchanged once it was exported and therefore could be modified by other code. I thought that e.g. setting it as export const mutableArray as const would then allow it to be safely exported and still treated as readonly. In fact typescript tracks no such features and enforces no such rules.

export const mutableArray:number[] = [];
mutableArray.push(3)
mutableArray.push(4)
mutableArray.push(5)

function immutablePush( numbers: ReadonlyArray<number>){
    return [...numbers, 6,7,8];
}

immutablePush(mutableArray);

Thanks for taking the time to fill me in.

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

2 participants