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

fix(rest-spread): do not require Symbol for String or fn parameters #9794

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@clshortfuse
Copy link

clshortfuse commented Mar 30, 2019

Q                       A
Fixed Issues? Fixes #9277
Patch: Bug Fix? 👍
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Pass (with no tests added)
Documentation PR Link
Any Dependency Changes? No
License MIT

This applies the same logic as other runtime helpers by checking if typeof Symbol === 'function' before using Symbol.iterator as seen here:

if (typeof Symbol === "function" && Symbol.asyncIterator) {

The CoreJS2 version of this example would be compiled as:

if (typeof _Symbol === "function" && _Symbol$asyncIterator) {
  AsyncGenerator.prototype[_Symbol$asyncIterator] = function () {
    return this;
  };
}

You can also see this at

if (typeof Symbol === "function" && Symbol.iterator) {

So I can confirm the syntax is correct. I've tested on IE11 and spread now works properly (after adding a polyfill for Array.from).

Tested with:

function sum(x, y, z) {
  return x + y + z;
}

const numbers = [1, 2, 3];

console.log(sum(...numbers));
// expected output: 6

The only actual code change in packages/babel-helpers/src/helpers.js and everything else is auto generated by make bootstrap.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Mar 30, 2019

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/10682/

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Mar 30, 2019

It's should be fixed at least for working with arrays, now it should not work.
image
This approach will cause the same problems with Symbol injection in runtime and preset-env like the previous PR, it should be fixed. However, this approach looks better for me since it does not add special cases and injection of Map, Set, etc.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Mar 30, 2019

@zloirock Do you have reference as to how it would produce an issue? Do you mean the conversion with corejs2? I did find other pieces of helper use typeof Symbol !== 'undefined which wouldn't care if Symbol() is available or not and then check Symbol.iterator. That would let Symbol be an object.

I did have another idea to solving this and that's ditching Symbol altogether. The reality is the code is checking to see if the Object has Symbol.iterator... and then never actually uses it. Unlike the other pieces of code that actually use Symbol.asyncIterator and Symbol.iterator, this one doesn't. If the code was going to call iter[Symbol.iterator]() then that's different.

In the end, it's just going to call Array.from and not use Symbol anyway. I think it makes for sense to just target what Array.from would actually do. Yes, all Objects that can be iterated with Symbol are arrays, but not that's exactly the best way to check for Array.from compatibility.

I was only targeting the regression specifically, but the alternative of Object.prototype.toString.call(iter) === "[object Arguments]" isn't that great either. For example, some ArrayLikes such as HTMLCollections can be used with Array.from but its prototype would be [object HTMLCollection]. Trying to target every single variation in some sort of whitelist is just asking for issues down the line (like Map or Set).

According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#Polyfill the only thing that Array.from really needs is to check the following is Object(iter) != null. To be a bit more robust, we can check if iter.length > 0 || iter.length === 0. (It'll make sure it's a number and not null or NaN). I'm pretty sure Array.length can't be negative, but that can easily checked as well.

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Mar 31, 2019

@clshortfuse this line will add tens of kilobytes to the bundle and it's a serious regression.

It could be fixed on runtime and preset-env sides. But typeof checks of built-ins should not be just ignored - they should return or be replaced to related boolean.

In the end, it's just going to call Array.from and not use Symbol anyway.

I propose to use this approach in the loose mode - it keeps all problems to a minimum.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Mar 31, 2019

I'm willing to dive in a bit more into the issue. I know sometimes solutions to problems tend to get shelved for lack of priority. I think finding the mechanics through which an Object can use Symbol.iterator can be a good goal.

Plainly stated, all the current code checks is whether the object is a candidate for Symbol iterator. We don't have to replicate the entirely Symbol functionality, but, instead, only it's check it's candidacy. (ie: if it can get an iterator, then it can receive an Array.from call).

Also, I think it's important to note the that the only purpose for the entire check is to simply cast (of sorts) an Object to an Array. As far as I've seen, there's no reference to this function outside of ToArray, so maybe taking a step back and looking at the original purpose may prove useful.

As for the mention of bloat, I'm assuming the issue lies with the importing of _Symbol from an external library. This is my first time playing around with a PR for Babel, so I wasn't aware of what consequences some functions would have. Still, I'll try to see what alternatives I can come up with. I'll probably need to dive deep into the ECMA spec and review a handful of popular Symbol polyfills just to ensure we don't cause any conflicts, and ultimately, hopefully it can be used outside of loose mode.

@clshortfuse clshortfuse force-pushed the clshortfuse:fix-ie11-spread-babelv7 branch from cc952fe to ded2532 Mar 31, 2019

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Mar 31, 2019

I took another stab at this. I took another approach based on how iterableToArrayLimit attempts to use Symbol.iterator

Basically, it's wrapped in a try block. If it fails, then it discards the error and tries the prototype. If that doesn't success, toArray is going to call _nonIterableRest() which throws a TypeError.

throw new TypeError("Invalid attempt to destructure non-iterable instance");

Doing it this way instead of how v6 was doing it (checking prototype first) avoids the performance penalty of the string comparison in contexts that support Symbol.iterator. I broke out the prototype whitelist into an array, so we can change it in the future to:

  const prototypeWhiteList = [
      '[object Arguments]',
      '[object HTMLCollection]',
      '[object NamedNodeMap]',
      '[object Map]',
      '[object Set]',
    ];

Out of curiosity, and not exactly related to the issue, but if we call (Array.prototype.from || [].slice).call(iter) we can then remove any requirements for IE11. It would only serve as a fallback. I'm sure there's some specific reason why we can't do this now (probably core-js), but it would be nice to have.

'[object Arguments]',
];
let hasSymbol = false;
try {

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2019

Why is a try/catch needed here instead of just checking if Symbol.iterator exists or not?

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 9, 2019

Author

You'd have to check if Symbol exists before checking if Symbol.iterator exists. I squashed the previous commit, but checking if Symbol exists would bloat the code up, as explained the one of the previous comments.

Doing it this way removes the need for typeof checks and lets the browser pass the Symbol check failure to catch. Since we're not actually using Symbol.iterator the result would be same. If it can't check the object to see if it has it, then it wouldn't have it anyway.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2019

try/catch has a performance penalty that's far worse than a few extra gzippable bytes.

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 9, 2019

Author

As I mentioned in the other comment, this is the fourth try in the same helper file. I don't see why this specific one should be treated any differently.

Edit: 4th try, 2nd specific to Symbol

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 9, 2019

because in that case, it's being used to catch errors thrown by user code (the iterator method). In this case it's used to catch errors in your code itself (attempting unchecked member access on a maybe-not-object).

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 9, 2019

Author

Ah, in the next() function. Okay, back to the drawing board.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 10, 2019

According to ES6 spec, the following prototypes use @@iterator

  • String
  • Array
  • Map
  • Set

WeakMap isn't guaranteed to have an iterator. TypedArray is listed as well, but since it looks like a native abstract type, I couldn't find a way to target all of them. I removed Map and Set since it would add more imports in corejs2. It doesn't seems possible to really check if an object uses Symbol.iterator without actually checking for Symbol and iterator. I did have another method of checking for Symbol, namely:

const env = ((typeof window !== 'undefined') && window)
  || ((typeof global !== 'undefined') && global)
  || ((typeof self !== 'undefined') && self);
if (!env || 'Symbol' in env) {
  if (Symbol.iterator in iterObject) {
    return Array.from(iter);
  }
}

Since I'm only targeting fixing the V6 regression with this PR, I resorted to leaving it out.

Show resolved Hide resolved packages/babel-helpers/src/helpers.js Outdated
// ES6 prototypes that use @@iterator
const prototypeWhiteList = [
String.prototype,
Array.prototype,

This comment has been minimized.

Copy link
@jridgewell

jridgewell Apr 10, 2019

Member

Array.isArray

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 10, 2019

Author

Array.isArray isn't the same a prototype check. For example, Array.isArray would return true when checked against Array.prototype where this wouldn't. Also, Array.isArray is already checked prior to this function.

Show resolved Hide resolved packages/babel-helpers/src/helpers.js Outdated
}
// Check for custom iterator outside ES6 spec, iterable WeakMap, or iterable TypedArray
if (Symbol.iterator in iterObject) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Apr 10, 2019

Member

Wasn't the point of this PR to do a typeof Symbol !== 'undefined' check?

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 10, 2019

Author

No. It's to fix the spread regression specifically.

This comment has been minimized.

Copy link
@jridgewell

jridgewell Apr 10, 2019

Member

But none of your changes will help spreading the HTMLCollection from the issue: [...document.getElementsByTagName('div')]. I don't understand what you're looking for then.

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 10, 2019

Author

The input code shows the error, but doesn't accurately detail the regession. It's in the description, which is more accurate. Basically, all spread functions fail on IE11 because it's checking for Symbol before checking the prototype. It's a regression from v6. I believe it's caused by 4da3f3b

This comment has been minimized.

Copy link
@jridgewell

jridgewell Apr 10, 2019

Member

The code in the OP goes through arrayWithHoles's Array.isArray check, so this change does nothing still. I need an explicit "this fails in IE 11, doesn't fail with this change" case so that I can review the PR.

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 10, 2019

Author

I think you're missing the point of the [object Arguments] check. That block never executes on environments without Symbol. Therefore any function with spread arguments would fail.

function func1(...args) {
  console.log(args[0]);
  // expected output: 1

  console.log(args[1]);
  // expected output: 2

  console.log(args[2]);
  // expected output: 3
}

func1(1, 2, 3);

Edit: Forgot the spread!

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 10, 2019

Author

Looking back at the issue from 3 months ago, the request was a partial fix for the regression while the more complicated issue of completely eliminating Symbol is explored. That PR has been stuck for a couple of months now.

I think that's why I had the mindset of swapping the order of execution, at least to minimize the issue.

This comment has been minimized.

This comment has been minimized.

Copy link
@clshortfuse

clshortfuse Apr 10, 2019

Author

Yes, and wrap them in parentheses. Also, any object that is a prototype of Array and String would align with the ES6 spec.

I'm not seeing much more we can do that wouldn't trigger extra corejs2 imports. I have no way of checking for the internal TypedArray type. Also, NodeList, HTMLCollection and others aren't part of the ES6 spec. I realized that this function, at least as it's called, is not checking for array-likes. It's checking for iterables. Therefore, something like this:

[...({ 0: 'a', 1: 'b', 2: 'c', length: 3})]

would throw an error on an ES6 supporting browser, and therefore should throw an error on Babel. Support for array-likes would have to be loose. I think this the best we can do without violating spec.

This comment has been minimized.

Copy link
@jridgewell

jridgewell Apr 10, 2019

Member

We can add a string check, that's fine. The array check is already covered by arrayWithHoles (the difference between Array.prototype passing Array.isArray and not Array.prototype.isPrototypeOf(Array.prototype) doesn't matter). Let's do the string check, then arguments check, then isIterable check.

@clshortfuse clshortfuse changed the title fix(spread): do not require Symbol fix(spread): do not require Symbol for rest parameters Apr 10, 2019

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 10, 2019

Okay, I've made the changes to this code. I also changed the commit and topic to better reflect the limited scope of this fix. I'm only targeting the rest parameter regression here.

From my research, I've hit a bit of a wall getting a catch-all solution. I've come to agree with @zloirock that the loose option is probably the only possible way to make it all work, but it definitely wouldn't be to ES6 spec. For example, on a real ES6 browser, this:

[...({ 0: 'a', 1: 'b', 2: 'c', length: 3})]

would throw an non-iterable error. That means babel should as well. It would be bad if babel would run something fine when transpiled to ES5, and then if somebody makes a ES6 version without babel, realizes that it doesn't work. That's because spread only works for iterables, NOT array-likes. Even though Array.from would accept it, it would be wrong to do so. Therefore any checks based on .length wouldn't be to spec.

There's more that can be done to target Map and Set which are part of the ES6 spec, but in my opinion, that would require changes to how typeof is transpiled. They should be checked without corejs2 importing the classes. Perhaps a special $isUndefined() could help in this case. Regardless, I think the best way to attack this problem is piece by piece and this is just a partial fix. We can explore that in another PR (I also have a couple of other theories as to checking if an object is using Array.prototype.values() as @@iterator).

Lastly, I want to apologize to the devs for the possibly annoying back and forth with the changes due to not fundamentally understanding the true underlying nature of @@iterator in ES6.

Note to @jridgewell : The reason why I didn't put the String check outside iterableToArray is because I wasn't sure if I should put it with all functions that call it. I felt it was safer to keep it inside.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 10, 2019

If I add

// Do not add import if only checking typeof
if (t.isUnaryExpression(parent, { operator: "typeof" })) return;

to

Then I can add the following without forcing imports:

if (typeof Symbol !== 'undefined' && (Symbol.iterator in Object(iter))) {
  return Array.from(iter);
}

Please advise to do so, or just keep this PR as is.

Edit: Though taking another look, that would cause a false positive. The reality is that we probably should let the compiler throw an error saying Symbol is not found. We wouldn't want to incorrectly let the function pass void, which throws a TypeError. The user could read the error and assume that ... isn't compatible with the object in question. We actually don't know if it's not compatible because Symbol isn't available and the error shouldn't be a false one.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Apr 10, 2019

Please add an exec.js test from #9794 (comment)

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 10, 2019

@jridgewell Let me know when you want me to squash the commits.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Apr 10, 2019

No need, we can squash merge them on GitHub.

@clshortfuse clshortfuse changed the title fix(spread): do not require Symbol for rest parameters fix(rest-spread): do not require Symbol for String or fn parameters Apr 10, 2019

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Apr 10, 2019

We will fix this to also work with Map/Set without loading the polyfills from core-js, but thank you for at least fixing this case!

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Apr 11, 2019

I don't see any sense in this PR in the current form since without Symbol.iterator will work only arguments and strings, but even they require Array.from. Spread will not work even with arrays without Symbol.iterator.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 11, 2019

@zloirock I don't understand. You rather it be in a more broken state?

It's a regression. There is no catch-all fix that I've seen without polyfilling Symbol. If we can at least partially resolve the regression, then people don't have to change perfectly working V6 code around.

Edit:

Spread will not work even with arrays without Symbol.iterator.

I'm pretty sure the function that is called before this one checks Array.isArray, so you don't need Symbol.Iterator for arrays, you just need Array.from support.

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Apr 11, 2019

@clshortfuse I'm not against of adding support of work spread without Symbol. iterator to babel@7, however I don't think that the current behavior is a regression - it's a breaking change in a major release, but by my vision, it's available since it makes babel behavior more correct by the spec. However, OK, let babel@7's spread work even without Symbol.iterator. I don't understand what's the heck is going on in this PR, see my comment above - those changes are almost completely useless since it does not support even arrays.

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Apr 11, 2019

Ok, my vision of this helper for babel@7:

function iterableToArray(it) {
  switch (Object.prototype.toString.call(it).slice(8, -1)) {
    // As an option, they will work even without `Array.from` polyfill,
    // however, it could cause problems with custom iterators:
    case "Arguments":      // It will not work in old IE without `core-js`
    case "HTMLCollection": // 2 most popular iterable DOM collections,
    case "NodeList":       // for work with other you should load `core-js`
      return Array.prototype.slice.call(it);
    case "String":         // Since of code units, we should use `Array.from`
    case "Set":            // It will work with `es6-shim` if they will add
    case "Map":            // internal @@toStringTag support
      return Array.from(it);
  }
  if (it == null) return;
  // It's more correct by the spec than `in` check
  // However, it will not work with `@babel/runtime-corejs2` -/
  const iterator = it[Symbol.iterator];
  if (typeof iterator !== 'function') return;
  return Array.from({ [Symbol.iterator]: iterator.bind(it) });
}
@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 11, 2019

@zloirock, I originally had an array prototype check but we agreed to remove it since the previous calling function is going to call Array.isArray.

I have my old commits here. I did have a list of every ES6 type in spec. I decided against the typeof check, before calling Set, Map or Symbol because it would lead to the wrong error. The user should get notified they don't exist and not a TypeError. That would be a spec violation. It can be confusing to give the user the incorrect error and we don't want people to think the object they are using spread against is an invalid one.

I opted out of non ES6 prototypes like HTMLCollection because they're not part of the actual spec. It's stricter, but whitelisting would be too loose, IMO, and go against the goal of being to spec.

I'm being pretty strict to limiting to just what ES6 has with String and rest arguments. Anything else should throw the correct error if it's not available. If users don't want it to be so strict, we can improve the loose mode.

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Apr 11, 2019

Ok, seems I missed a moment about Array.isArray in arrayWithoutHoles -> toConsumableArray, however, this PR still makes spread work only on arguments and strings and only with polyfilled Array.from, which is not really useful. As you can see in the example of the helper above, you can unobtrusively check the argument on internal classes without type/reference errors and loading unnecessary polyfills. Iterators for DOM collections were added to core-js only because of a babel's feature request, they are added in @babel/polyfill, @babel/runtime, @babel-preset-env, so adding them in those cases and not adding to special cases of this helper (if they are will be added) are double standards.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 11, 2019

@zloirock I can easily add the code. I don't have an issue with that. That's how I originally envisioned it, but I wasn't sure we were doing whitelists.

I've researched that Map, Set, Array, String, TypedArray (which is abstract), and Arguments are in Spec. I honestly did try the prototype name because it's not exactly 100% safe. But then again, we're checking Arguments the same way, so it would be a bad double standard.

The roadblock I reached is realizing spread doesn't support Array-likes (objects with length). It only supports iterables. Array.from is more flexible than ES6 spread. There's going to have to be some flexibility and I wasn't sure how loose we were willing to go.

My idea is to whitelist as much as possible and then go for Symbol.Iterator. I think looser than that, then the user should consider loose. We don't want code that only works on Babel, but doesn't work on real browsers (using Spread on Array-likes without Symbol.Iterator).

I'll work on adding Map and Set as you suggested with the prototype name in String format.

@ljharb

This comment has been minimized.

Copy link

ljharb commented Apr 11, 2019

Note that es6-shim’s Array.from would work in that example without any changes.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 11, 2019

Okay, I'm checking @zloirock code at a time that's not 2AM and on my phone 😆, and it does seem to meet all the targets I was looking for and adds the slice.call() I mentioned in one of the previous comments.

On the whitelist I had prepared, I had couple of other types that are spread compatible that are part of spec:

  • [object String Iterator]
  • [object Array Iterator]
  • [object Map Iterator]
  • [object Set Iterator]
  • [object Int8Array]
  • [object Uint8Array]
  • [object Uint8ClampedArray]
  • [object Int16Array]
  • [object Uint16Array]
  • [object Int32Array]
  • [object Uint32Array]
  • [object Float32Array]
  • [object Float64Array]
  • [object BigInt64Array]
  • [object BigUint64Array]

Basically, it's Iterators which are iterable and the TypedArray types. Thankfully, TypedArray iterators are [object Array Iterator] which reduces the big list.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Apr 11, 2019

If there are built in iterators, won't there also be Symbol.iterator?

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 11, 2019

@nicolo-ribaudo Yeah. It would just be a question of whether it's faster to bypass whatever we're doing with Symbol check and immediately return with Array.from.

@clshortfuse

This comment has been minimized.

Copy link
Author

clshortfuse commented Apr 11, 2019

How does everyone feel about just committing this as-is with the limited fix? I wasn't really targeting fixing everything. I just wanted rest arguments working. I feel like like the other PR that targeted all of the Symbol issues has been abandoned. The regression of breaking rest arguments is over a year old at this point (March 8th 2018).

I could open up another PR for fixing everything and we can start fresh.

@ljharb

This comment has been minimized.

Copy link

ljharb commented Apr 11, 2019

Iterative fixes are good, to be sure.

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Apr 11, 2019

I'm happy with merging this as is.

@zloirock

This comment has been minimized.

Copy link
Member

zloirock commented Apr 11, 2019

@clshortfuse BigUint64Array / BigInt64Array available only in engines with Symbol support.

I'd prefer a version with all required special cases or a raw version without anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.