Skip to content
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

Allow ArrayLike as a spread source #7596

Closed
iby opened this issue Mar 20, 2016 · 28 comments
Closed

Allow ArrayLike as a spread source #7596

iby opened this issue Mar 20, 2016 · 28 comments
Labels
ES6 Relates to the ES6 Spec In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@iby
Copy link

iby commented Mar 20, 2016

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
Copy link
Contributor

jwbay commented Mar 20, 2016

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
Copy link
Author

iby commented Mar 20, 2016

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.

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Mar 28, 2016
@mhegazy mhegazy closed this as completed Mar 28, 2016
@iby
Copy link
Author

iby commented Mar 28, 2016

@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
Copy link
Contributor

mhegazy commented Mar 28, 2016

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
Copy link
Author

iby commented Mar 29, 2016

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 mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Question An issue which isn't directly actionable in code labels Mar 29, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Mar 29, 2016

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
Copy link
Contributor

mhegazy commented Mar 29, 2016

updating the title.

@mhegazy mhegazy changed the title Type 'IArguments' is not an array type Allow ArayLike as a spread soruce Mar 29, 2016
@iby iby changed the title Allow ArayLike as a spread soruce Allow ArayLike as a spread source Mar 29, 2016
@kitsonk
Copy link
Contributor

kitsonk commented Mar 30, 2016

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
Copy link
Contributor

mhegazy commented Mar 30, 2016

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.

@mhegazy mhegazy reopened this Mar 30, 2016
@mhegazy mhegazy added the ES6 Relates to the ES6 Spec label Mar 30, 2016
@kitsonk
Copy link
Contributor

kitsonk commented Mar 30, 2016

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
Copy link
Contributor

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 RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels May 9, 2016
@RyanCavanaugh RyanCavanaugh added this to the Community milestone May 9, 2016
@RyanCavanaugh RyanCavanaugh added the Good First Issue Well scoped, documented and has the green light label May 9, 2016
@RyanCavanaugh
Copy link
Member

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
Copy link
Contributor

Two more issues:

  • Do you still polyfill if the expression is already an Array? If so, you hurt the performance of existing code, but if not, type-dependent emit is required.
  • What if the global Array is shadowed by a local? (I have a lot of modules that define classes with the same names as JS globals like Object, Number, String - no Array, but I'm sure plenty of other people have that one.) If using the spread operator will need access to Array.prototype that's a breaking change.

@mhegazy
Copy link
Contributor

mhegazy commented May 10, 2016

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
Copy link
Contributor

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:

  • [...x] - This has to do a copy anyway, so no major loss from changing x.slice() to Array.prototype.slice.call(x).
  • f(...x) - Transpiles to f.apply(void 0, x). Since .apply works on any ArrayLike, no change in emit is necessary.

@mhegazy
Copy link
Contributor

mhegazy commented May 11, 2016

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
Copy link
Contributor

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

@mhegazy
Copy link
Contributor

mhegazy commented May 11, 2016

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
Copy link
Contributor

jeffreymorlan commented May 11, 2016

@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
Copy link
Contributor

mhegazy commented May 11, 2016

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

@mhegazy mhegazy added In Discussion Not yet reached consensus and removed Help Wanted You can do this Good First Issue Well scoped, documented and has the green light labels May 11, 2016
@mhegazy mhegazy removed this from the Community milestone May 11, 2016
@pablobirukov
Copy link

[...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
Copy link
Contributor

mhegazy commented Jan 3, 2017

This should be addressed by #12346

@aaron-resnick
Copy link

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

@kitsonk
Copy link
Contributor

kitsonk commented Dec 29, 2017

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

@vitaly-t
Copy link

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?

@DanielRosenwasser DanielRosenwasser changed the title Allow ArayLike as a spread source Allow ArrayLike as a spread source Jun 18, 2018
@fregante
Copy link

fregante commented May 24, 2019

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
Copy link

sushruth commented Mar 12, 2021

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
Copy link
Member

This works now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ES6 Relates to the ES6 Spec In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests