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

Normative: allow ArraySpeciesCreate to create non-arrays #1289

Closed
wants to merge 1 commit into from

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Aug 17, 2018

Summary: this PR modifies the methods Array.prototype.{concat, filter, map, slice, splice} such that they return an instance of the same class1 as the object on which they are invoked even if that object is not an array, rather than, as currently, only in the case that the object is an array (that is, was created by Array or a subclass thereof).

1 to be precise, of the class given by original.constructor[Symbol.species], if it exists


Array.prototype.map and similar methods which create new arrays defer to Symbol.species to determine the constructor for the resulting object. However, unlike other usages of Symbol.species such as RegExp.prototype[Symbol.split], the array prototype methods will only use Symbol.species when the object on which the method is invoked is an actual array. (I believe this may have been an oversight introduced in ES6 when specifying the magic cross-realm behavior of Array.prototype.map and friends.)

This means that in the following code

class KindaArray { // NB: not extending `Array`, possibly because it needs to extend some other thing
  static get [Symbol.species]() {
    console.log('reached'); // not reached
    return KindaArray;
  }

  get map() {
    return [].map;
  }
}

let x = new KindaArray;
x[0] = 'foo';
x.length = 1;

let y = x.map(t => t + 'bar');
y instanceof KindaArray

the last line evaluates to false. Since Array prototype methods are explicitly intended to be usable on things which are not arrays, and Symbol.species is explicitly intended to allow methods which create new instances to return instances of the same class as the original instance rather the class on which the method was originally defined, this is surprising to me. The current behavior also seems not particularly useful. This is especially so because a proxy for an array is treated as an array for the purposes of these algorithms, and such a proxy might not behave like an array in any way.

This PR changes the behavior of the above code so that reached is printed and the last line evaluates to true.

Note that this change is observable even in code which does not explicitly reference Symbol.species: currently Array.prototype.map.call(new Uint8Array([0,1,2]), x => x + 1) returns an Array; with this change it would return a Uint8Array, just like Uint8Array.prototype.map.call(new Uint8Array([0,1,2]), x => x + 1).

In practice, I don't expect this to affect much if any existing code. That people might be relying on the behavior in the previous paragraph is my only real worry, and I'm hopeful that's not the case. There might also be a slight performance regression when applying these methods to array-likes, as in the fairly common pattern of [].slice.call(arguments, 0), since these will need to do two additional lookups (to resolve arguments.constructor[Symbol.species] to undefined).

See twitter discussion. Also see #1178.

cc @tabatkins @allenwb

@bakkot bakkot added the needs consensus This needs committee consensus before it can be eligible to be merged. label Aug 17, 2018
@ljharb ljharb added the normative change Affects behavior required to correctly evaluate some ECMAScript source text label Aug 17, 2018
@allenwb
Copy link
Member

allenwb commented Aug 17, 2018

Note that this revised behavior is exactly what I originally intended for ArraySpeciesCreate and the built-in methods that use it. The original version in ES6 had a bug that wasn't discovered and and subsequent refactoring were careful to preserve that buggy behavior.

Copy link
Member

@allenwb allenwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. It preserves the originally intended backwards compatibility edge cases.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that this will impact existing code in the following ways:
Borrowing an affected Array prototype method and .calling it on an object:

  • → will now have an observable Get on "constructor"
  • if that constructor is the current realm's Array, or a non-object, then this will return the same ArrayCreate as before.
  • → If the constructor is a "not Array" object, it will now have an observable Get on Symbol.species
  • if that is null or undefined, it will return the same ArrayCreate as before
  • → if that is a non-constructor, it will throw a TypeError, instead of returning ArrayCreate as before.
  • If that is a constructor, it will return a constructed object per the Species-indicated constructor, likely matching the author's intention (whereas before, it would have returned ArrayCreate)

Assuming that's correct, then the spec change LGTM, pending the consensus discussion in the meeting.

@bakkot
Copy link
Contributor Author

bakkot commented Aug 17, 2018

@ljharb, here's a summary of (almost) all of the relevant different kinds of objects and their observable behavior when passed to ArraySpeciesCreate before and after this PR. (I'm going to ignore the cases where the object is a revocable proxy or exotic object.)

Assume the object in question is named originalArray. For the purposes of this list:

  • "x is an array" means that IsArray ( x ) would return true
  • "x is not an array" means that IsArray ( x ) would return false
  • %Array% is the original value of Array from the current realm,
  • %ArrayPrototype% is the original value of Array.prototype% from the current realm, and
  • @@species is the original value of Symbol.species.

Before:

1. An object which is an array, whose .constructor gives the original value of Array from a different realm.

  1. Get originalArray.constructor.
  2. Return a new exotic array object whose prototype is %ArrayPrototype%.

2. An object which is an array, whose .constructor property gives any other value:

  1. Get originalArray.constructor; call it ctor.
  2. If ctor is any primitive other than undefined, throw a TypeError.
  3. If ctor is undefined, return a new exotic array object whose prototype is %ArrayPrototype%.
  4. Get ctor[@@species]; call it species.
  5. If species is null or undefined, return a new exotic array object whose prototype is %ArrayPrototype%.
  6. Otherwise, if species is not a constructor, throw a TypeError.
  7. Otherwise, return the result of calling species as a constructor.

If originalArray.constructor gives %Array% and no one has overridden %Array%[@@species], then species will be %Array% and so this will return a new exotic array object whose prototype is %ArrayPrototype%. This is the case when invoking array methods on arrays from the same realm as the method (e.g., [0, 1].concat([2, 3]).

3. An object which is not an array:

  1. Return a new exotic array object whose prototype is %ArrayPrototype%.

After:

The first and second cases do not change.

The third case now unconditionally follows the steps in the second. Such an object does not follow the steps in the first even if its .constructor gives the original value of Array from a different realm, as it would if it were an array.

@allenwb
Copy link
Member

allenwb commented Aug 17, 2018

@bakkot
various places

Return a new array whose prototype is %Array%.

that should be

Return a new exotic array object whose prototype is %ArrayPrototype%

@allenwb
Copy link
Member

allenwb commented Aug 17, 2018

Rather than inferring intent from the (buggy) algorithm, let's just capture the original intent

  • In the absence of legacy compatibility behaviors and malformed object structures, ArraySpeciesCreate should do the following:
  1. Get the original array object's "constructor"
  2. Get the value the constructors @@species
  3. If the species property is missing or its value is null or undefined, return result of ArrayCreate
  4. If the value of species is not a constructor throw a TypeError
  5. Return the result of invoking the species value as a constructor.

Note that the built-in @@species property of %Array% is a get accessor that returns this so normally constructed subclasses of %Array% that don't over-ride @@species will produce the subclass constructor as the @@species values.

  • In the absence of legacy compatibility concerns, there would have been additional error conditions such as throwing if the original array object did not have a "constructor" property. But instead, in all of the following legacy special cases the result of ArrayCreate is returned:
  1. The original array does not have a constructor property.
  2. The value of the original array's constructor property is null or undefined.
  3. The original array is an array exotic object whose constructor is the %Array% object from a realm other than the current realm.

Note that prior to ES6, all places that currently use ArraySpeciesCreate did the equivalent of ArrayCreate so there were no observable property accesses. The original (buggy) structuring of the ES6 ArraySpeciesCreate was primarily about implementing the above normal and special cases in a way that avoid any redundant observable property accesses.

@bakkot
Copy link
Contributor Author

bakkot commented Aug 17, 2018

@allenwb:

Return a new exotic array object whose prototype is %ArrayPrototype%

Whoops, thanks. Edited my original comment to avoid confusion.

let's just capture the original intent

Thanks, this matches my understanding except for the following caveat:

The value of the original array's constructor property is not a constructor function.

To be precise I think this should say "is an object which is not a constructor function". The algorithm still throws for, for example, x = []; x.constructor = 0; x.map(()=>{});.

@allenwb
Copy link
Member

allenwb commented Aug 17, 2018

@bakkot

To be precise I think this should say "is an object which is not a constructor function". The algorithm still throws for, for example, x = []; x.constructor = 0; x.map(()=>{});.

Then that's another bug in the algorithm, because the about sequence would have been valid in ES3/5 and hence needs to be preserved.

Special case 2 should probably be: The value of the original array's constructor property is null or undefined.

The x.constructor = 0 case should work by looking up 'constructor' on %NumberPrototype", to get %Number% and then looking up @@species on %Number% which should return undefined which should cause ArrayCreate in step 3 of the normal behavior. In other words, for the purposes of this algorithm (actually most algorithms) constructor values that are primitive types are coerced to wrapper objects for property access purposes.

I'll update #1289 (comment)

@bakkot
Copy link
Contributor Author

bakkot commented Aug 17, 2018

Then that's another bug in the algorithm, because the about sequence would have been valid in ES3/5 and hence needs to be preserved.

Since engines have been throwing for this case for a while now, I think it's probably fine to continue doing so, despite that breaking with ES5.

The x.constructor = 0 case should work by looking up 'constructor' on %NumberPrototype%, to get %Number% and then looking up @@species on %Number% which should return undefined

Uh, that looks like one too many lookups for a constructor property. Shouldn't this be

looking up 'constructor' on x to get 0 and then looking up @@species on %NumberPrototype% which should return undefined

?

@allenwb
Copy link
Member

allenwb commented Aug 17, 2018

@bakkot

Since engines have been throwing for this case for a while now...

Yes that occurred to me to, but:

  1. This is an obscure edge case. Who knows how many things the current implementations have broken they they've never heard about because the breakage wasn't a major website.
    2.. Throwing on primitive values introduces a hard to explain inconsistency into the language. Property accesses on primitive values normally do property lookups using the corresponding wrapper prototype. Why should this be treated inconsistently as a special case. WTF JS!

WRT "took many look ups" . Just forget about that paragraph that is trying to (poorly and unnecessarily) explain property lookup on primitive values.

Instead, I think special case 2 should be:

  1. The Type of the value of the original array's constructor property is not Object.

@anba
Copy link
Contributor

anba commented Aug 17, 2018

Requiring constructor and @@species lookups for all objects will likely lead to performance regressions, for example in Speedometer about 1/3 of all ArraySpeciesCreate calls for slice and splice is performed with non-arrays (https://bugzilla.mozilla.org/show_bug.cgi?id=1376572#c5). The same bug also has some numbers showing the general overhead when performing constructor and @@species lookups.

Apart from the performance issue, it will also be necessary to validate that this change won't break any existing applications. Specifically when Array.prototype.{concat,filter,map,slice,splice} is called with non-array objects which already have a @@species property. For example [].slice.call(new Int32Array(4)) will throw with the proposed change.

@bakkot
Copy link
Contributor Author

bakkot commented Sep 25, 2018

The committee decided not to do this, in light of potential performance and web compatibility concerns. See further discussion in #1313.

@bakkot bakkot closed this Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text spec bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants