microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
101.08k stars 12.49k forks source link

Allow ArrayLike as a spread source #7596

Closed iby closed 9 months ago

iby commented 8 years ago

When trying to pass caller arguments to another function with ...arguments the compiler complains with Type 'IArguments' is not an array type. Version 1.8.9.

jwbay commented 8 years ago

The arguments object is not an array type (though it resembles one), so the compiler error seems correct. Accept the arguments with a rest operator and they'll get converted to an array.

function foo(...args) {
   bar(...args);
}
iby commented 8 years ago

Now when you say it, I remember looking it up a dozen times now… Still, that functionality would be natural, especially given that apply can be used directly with arguments.

iby commented 8 years ago

@mhegazy I wasn't asking a question. This works in javascript, typescript fails to compile. arguments is not an array, it's a fact, but it should be compatible with the spread operator.

mhegazy commented 8 years ago

it is not an array. quoting MDN:

The arguments object is an Array-like object corresponding to the arguments passed to a function.

it goes on to say:

The arguments object is not an Array. It is similar to an Array, but does not have any Array properties except length. For example, it does not have the pop method. However it can be converted to a real Array

so,

function f() {
  arguments.push(0);          // Runtime error: Uncaught TypeError: arguments.push is not a function(…)
  arguments.indexOf(0);       // Runtime error: Uncaught TypeError: arguments.indexOf is not a 
  Array.isArray(arguments);   // false
}
f(1,2);
iby commented 8 years ago

We're having a miscommunication. I am not saying it's an array, I'm stating the opposite, that it is not array, but it should be compatible with the spread operator.

function foo(){
    console.log(...arguments);
}

foo('bar', 'baz');

This throws a compile error, but produces a working code. Naturally arguments would be expected to work with the spread operator.

mhegazy commented 8 years ago

i see.

well. some of the transformation on a spread uses slice. which does not exist. in theory we can allow ArrayLike, and call Array.prototype.slice.apply(v) instead of v.slice

mhegazy commented 8 years ago

updating the title.

kitsonk commented 8 years ago

It isn't valid ES6 syntax:

'use strict';

function bar() {
  console.log(arguments);
}

function foo() {
  bar.call(this, ...arguments);
}

foo('bar', 'qat');

It fails in Firefox (but not in Chrome or Edge) and so would have to be transformed even when targeting ES6 with little or no value. It was the "magic" behaviours of arguments was a big part of the value of the spread/rest operator in the first place.

mhegazy commented 8 years ago

as per the ES6 spec: http://www.ecma-international.org/ecma-262/6.0/#sec-createmappedargumentsobject, arguments is actually iterable. and spread allows iterables: http://www.ecma-international.org/ecma-262/6.0/#sec-runtime-semantics-arrayaccumulation.

This means that we need to emit Array.prototype.slice.call(value) instead of value.slice(); and since we are doing that, we might just relax the restriction to say that a spread target needs to be ArrayLike.

kitsonk commented 8 years ago

Cool... Firefox has a bug then (as it can't find the iterable interface on arguments).

One challenge though is that doing that will leak arguments and can impact JVM optimisation (which is another argument for using the rest operator to collect argument (...args) before spreading them again, because the rest emit does it in a non leaky way.

jeffreymorlan commented 8 years ago

An ArrayLike is not necessary an Iterable, nor vice versa. Since the native ES6 spread operator only allows Iterables, extending it to allow ArrayLikes would require type-dependent emit for --target ES6.

RyanCavanaugh commented 8 years ago

Accepting PRs; this should be pretty straightforward.

One big warning is that this is going to end up allowing string as a spread source, and a spreaded string with surrogate pairs is going to behave differently in ES6 compared to a downlevel ES5 emit using Array.prototype.slice.call. TL;DR: Caveat emptor when spreading in surrogate-pair-containing strings in ES5+ES6-split codebase (hopefully this applies to literally no one).

jeffreymorlan commented 8 years ago

Two more issues:

mhegazy commented 8 years ago

Do you still polyfill if the expression is already an Array?

Not sure if there is a noticeable difference here for a normal program.

If so, you hurt the performance of existing code, but if not, type-dependent emit is required.

Do not think we can do type-directed emit.

What if the global Array is shadowed by a local?

Currently the TS compiler does not guard against these, and that has not been an issue in the past. e.g. Object in __extends, __assign, and __decorate, React in JSX emit and Math in exponentiation operator polyfill. so we could check for it, but does not seem to be a big issue in practice.

jeffreymorlan commented 8 years ago

I've extensively used the spread operator on Arrays because it's a lot more convenient to concatenate two arrays of different subtypes than using concat directly:

declare var a: Derived1[], b: Derived2[], c: Base[];

c = a.concat(b); // Error
c = (<Base[]>a).concat(b); // Works but annoying
c = [...a, ...b]; // Works, and currently just transpiles down to .concat

The proposed change would turn this into an inefficient double-copy of both arrays. So for performance sensitive applications this would make the common case of Array concatenation painful - now it has to be written as cast-and-concat, at the benefit of maybe more convenience in the much rarer case of concatenating other ArrayLikes.

I do think it might be an improvement to allow ArrayLike in spread for non-concatenation uses only where it wouldn't force extra copying:

mhegazy commented 8 years ago

The proposed change would turn this into an inefficient double-copy of both arrays. So for performance sensitive applications this would make the common case of Array concatenation painful - now it has to be written as cast-and-concat, at the benefit of maybe more convenience in the much rarer case of concatenating other ArrayLikes.

not sure i see the issue here. today this is emitted as:

c = a.concat(b);

under this proposal it would be emitted as:

c = Array.prototype.concat.call(a, b);

which should not be much different perf wise, and no extra copies needed.

Arnavion commented 8 years ago

@mhegazy Everything prior to your comment implies that you intended to emit it as Array.prototype.slice.call(a).concat(b)

mhegazy commented 8 years ago

oh, no. that was not what i had in mind.. sorry if this was not clear.

today we make an assumption that the value is an Array at run time. so we could access array methods directly on the object, i.e. a.concat, a.slice. if we relax that to just ArrayLike, then we can not just access them on the object, we have to access them on Array.prototype. other than that, there are no intended changes to the emit. no additional calls that are not in the emit today.

so these:

c = [...a, ...b];
foo(...c);
[...x] 
function f([a, ...x]) {}

today emit as:

c = a.concat(b);
foo.apply(void 0, c);
x.slice();
function f(_a) {
    var a = _a[0], x = _a.slice(1);
}

under this proposal would emit as:

c = Array.prototype.concat.call(a, b);
foo.apply(void 0, c);
Array.prototyp.slice.call(x);
function f(_a) {
    var a = _a[0], x = Array.prototype.slice.call(_a, 1);
}
jeffreymorlan commented 8 years ago

@mhegazy That doesn't work - .concat flattens out only actual Arrays, but other ArrayLikes are treated as single elements:

> var myArrayLike = {length: 2, 0: 'apples', 1: 'bananas'};
> var myRealArray = ['apples', 'bananas'];

// Array.prototype.concat flattens out real arrays:
> Array.prototype.concat.call(myRealArray, ['carrots'])
[ 'apples', 'bananas', 'carrots' ]
> Array.prototype.concat.call(['carrots'], myRealArray)
[ 'carrots', 'apples', 'bananas' ]

// ...but it does not flatten out ArrayLikes, in either "this" or argument position:
> Array.prototype.concat.call(['carrots'], myArrayLike)
[ 'carrots', { '0': 'apples', '1': 'bananas', length: 2 } ]
> Array.prototype.concat.call(myArrayLike, ['carrots'])
[ { '0': 'apples', '1': 'bananas', length: 2 }, 'carrots' ]

So allowing ArrayLike in spread, other than the trivial cases [...x] and f(...x), requires a double copy (slice to convert to array, then concat)

mhegazy commented 8 years ago

oh. did not think of that. duh.. back to the drawing board then :)

pablobirukov commented 7 years ago

[...document.querySelectorAll(".foo")] triggers Type 'NodeListOf<Element>' is not an array type (with target: "es5"). But I think it would be nice to have it successfully transpiled to Array.prototype.slice.call(document.querySelectorAll(...)) since NodeListOf<Element> is iterable

mhegazy commented 7 years ago

This should be addressed by https://github.com/Microsoft/TypeScript/pull/12346

aaron-resnick commented 6 years ago

I believe #12346 did not address this, as I'm still seeing the same error in TS 2.6.2

kitsonk commented 6 years ago

@aaronresnick as mentioned in the PR text, when targeting ES3/ES5 you have to use --downlevelIteration. Are you using that?

vitaly-t commented 6 years ago

So, to clarify, as of this day it still doesn't work, and the reason being - it doesn't work in FireFox.

In the meantime, all our Node.js code in JavaScript uses arguments for spread operator without any issues, but the TypeScript can't/won't do it regardless, for compatibility with FireFox.

Shouldn't support for spread of array-like objects become the default at this point, or at least default when building for the server-side?

fregante commented 5 years ago

The spread operator only works on Iterables and ArrayLikes aren't Iterables.

// from lib.es5.d.ts
interface ArrayLike<T> {
    readonly length: number;
    readonly [n: number]: T;
}

Try this anywhere:

[...{length: 0}];
// TypeError: ({length:0}) is not iterable

arguments however is both Iterable and ArrayLike, and it already works correctly in TypeScript:

[...arguments]

This issue should be closed as resolved or invalid.

sushruth commented 3 years ago

Valid -

I have now realized that ReadonlyArray<> is a better tool for this job than ArrayLike<>. Text below is invalid.

Invalid -

One problem I had that led me to this issue was not being able to type spread arguments as ArrayLike like so -

function getProps<K extends ArrayLike<keyof User>>(...props: K) {
  // above line gives this error at `...props: K`  - A rest parameter must be of an array type.(2370)
}

However there is a tiny workaround for this -

function getProps2<K extends ArrayLike<keyof User>>(...props: K[number][]) {
  // No error above!
}

I hope this helps some.

RyanCavanaugh commented 9 months ago

This works now