Repository navigation
Array.shift() should return defined for consistency with random accessΒ #51035
Description
Suggestion
π Search Terms
in:title array shift
#13778
β 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
Whether an array element is defined/undefined is inconsistent. If you use direct access it's always defined even when the control flow clearly indicates it couldn't possibly be; if you use method-based access it's always possibly undefined even when the control flow clearly indicates it must be defined.
The discussion for random access in #13778 ended in keeping it defined by default and adding a pedantic compiler flag for making it all undefined. I'm proposing an extension of that to shift and other Array methods that should, for consistency, follow the same behaviour.
π Motivating Example
const foo = [1,2,3,4,5]
const bar = (_: number) => _
const x = foo.shift() // x is undefined even though foo has elements defined above
bar(x) // type error
const y = foo[99] // y is defined even though foo does not have that many elements
bar(y) // fine
if (foo.length > 0) {
const z = foo.shift() // z is undefined even though we just checked foo.length > 0
bar(z) // type error
}
if (!(foo.length)) { // or foo.length === 0 whatever floats your boat
const z = foo[99] // z is defined even though we just checked it couldn't possibly be
bar(z) // fine
}π» Use Case
I can understand that it always could be undefined, e.g. in this case:
const x: number[] = [1, 2, 3]
;(async () => {
await longAsyncTask();
while (x.length > 0) x.shift()
})()
;(async () => {
await longAsyncTask();
console.log(x.shift()) // could be undefined if the previous async block executes first
})()However, this is a rarer case than x[n] which is much more commonly undefined. Given that x[n] is by default assumed defined unless the --noUncheckedIndexedAccess compiler flag is provided, .shift() should follow the same pattern.
Currently we use .shift() and a queue-based structure rather than random access, but TypeScript actually penalizes this. The only ways around this are assertions ! or adding an extra call for random access.
Workaround 1 (!):
const foo = [1,2,3,4,5]
const bar = (n: number) => n
bar(foo[99]) // somehow fine
bar(foo.shift()) // error?
bar(foo.shift()!) // workaroundConsider the following case as why this is a problematic anti-pattern:
const foo = myArray.map(myFunc) // I write this thinking the signature of foo is number[]
// however due to a bug in myFunc, signature is actually (number | undefined)[]
bar(foo.shift()!) // I type this as my workaround to behaviour described above
// now no type checking occurs above and I have a much more obscure bug in my code!Workaround 2 is more type-safe (extra call for random access):
bar(foo[0])
foo.shift();However, this is an anti-pattern at best.
Activity
fatcerberus commentedon Oct 3, 2022
Thereβs no way to have the return type of a method depend on a compiler flag, so the stricter option was chosen.
cuuupid commentedon Oct 3, 2022
Given the less strict option was chosen for random access until that flag was added, wouldn't it be more consistent to assume defined in this case?
MartinJohns commentedon Oct 3, 2022
Consistency is a really bad argument when the proposed change would effectively reduce type safety.
Instead of
while (x.length > 0) { x.shift() }you can just do