Improve array destructuring spec compliance

[SuperSodaSea]

Q	A
Fixed Issues?	Fixes #15154, Fixes #15168
Patch: Bug Fix?	👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

Spec compliance

• Empty array destructuring is transformed into slicedToArray(arr, 0) instead of toArray(arr)
• Empty array destructuring should check the iterator (result of [Symbol.iterator]()) is an object
• Empty array destructuring should not call the next() on the iterator
• Empty array destructuring should call the return() on the iterator, and check if it returns an object
• Iterator's next should be evaluated once and only once, even if the next() is not called

Tests

• babel-plugin-transform-destructuring/test/fixtures/destructuring/check-iterator-return
• babel-plugin-transform-destructuring/test/fixtures/destructuring/empty-array-pattern
• babel-plugin-transform-destructuring/test/fixtures/destructuring/next-eval-once
• babel-plugin-transform-destructuring/test/fixtures/regression/15154
• babel-plugin-transform-destructuring/test/fixtures/regression/15168

[babel-bot]

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

[liuxingbaoyu]

Thanks!

[SuperSodaSea]

Oh, I missed i.return, let me fix it...

[nicolo-ribaudo]

I'm from mobile so this code might be completely wrong, but does it work?

for (_i = _i.call(arr); i < _arr.length && !(_n = (_s = _i.next()).done); _n = true) {
  _arr.push(_s.value);
}

We should try keeping these helpers as small as possible :)

[SuperSodaSea]

I change it to this:

try {
  for (_i = _i.call(arr); _arr.length < i && !(_n = (_s = _i.next()).done); _n = true) {
    _arr.push(_s.value);
  }
  if (i === 0) {
    if (Object(_i) !== _i) return;
    _n = false;
  }
} // ...

Does this looks good?

[SuperSodaSea]

Oh, that seems to be not working since the order of _n = true is changed... I'll revert the change.

try {
  _i = _i.call(arr);
  if (i === 0) {
    if (Object(_i) !== _i) return;
    _n = false;
  } else {
    for (; !(_n = (_s = _i.next()).done); _n = true) {
      _arr.push(_s.value);
      if(_arr.length === i) break;
    }
  }
} // ...

[nicolo-ribaudo]

Use Object(_i) !== _i, which checks for both functions and objects but it excludes null.

[SuperSodaSea]

Fixed!

[liuxingbaoyu]

Can you add a test to cover the problem just now?
Similar to https://github.com/nicolo-ribaudo/babel/blob/b0120972e8cdc350294ba5672194f1b0b25f9c6a/packages/babel-plugin-transform-parameters/test/fixtures/regression/15145/exec.js

[SuperSodaSea]

@liuxingbaoyu Sure, I'll add tests for #15154 and #15168 later.

[nicolo-ribaudo]

I found another related bug:

let gets = 0;
let it = {
  [Symbol.iterator]: () => ({
    nextDone: false,
    get next() {
      gets++;
      return () => {
        if (this.nextDone) return { done: true, value: 2 };
        this.nextDone = true;
        return { done: false, value: 0 }
      };
    }
  }),
}

let [a, b] = it;
expect(gets).toBe(1);

gets = 0;
let [] = it;
expect(gets).toBe(1);

gets is currently first 2 and then 0.

I think we can solve it by pre-reading .next:

    var _arr = [];
    var _n = true;
    var _d = false;
    var _s, _e, _x;
    try {
      for (_x = (_i = _i.call(arr)).next; _arr.length < i && && !(_n = (_s = _x.call(i)).done); _n = true) {
        _arr.push(_s.value);
      }
    }

by doing so we also get "is _i an object?" check automatically when reading _i.next.

[nicolo-ribaudo]

Since the loose helper does not need to be 100% spec compliant, is this enough to avoid infinite loops?

for (_i = _i.call(arr), _step; arr.length < i && !(_step = _i.next()).done;) {
  _arr.push(_step.value);
}

[SuperSodaSea]

The variable declaration of _step seems to be missing in the original implementation (put it in the first expression of for seems to be useless?), so I think the correct code should be this:

var _s;
for (_i = _i.call(arr); arr.length < i && !(_s = _i.next()).done; ) {
  _arr.push(_s.value);
}

Committed in 4f951f3.

[nicolo-ribaudo]

This PR is fixing a lot of test262 failures!

# 0 new failures, 226 new successes

https://github.com/babel/babel/actions/runs/3441979778/jobs/5742297076#step:14:459

[SuperSodaSea]

@nicolo-ribaudo I think we still need the "is an object" check, for example 'foo'.next is undefined instead of throwing TypeError.

By the way, is there a place that shows the overall test262 result of Babel?

[SuperSodaSea]

@liuxingbaoyu Tests for #15154 and #15168 are added!

---

Also I found another bug, we should check whether the return() returns an object:

expect(() => {
  var [] = {
    [Symbol.iterator]: () => {
      return {
        // Should throw TypeError when `return()` returns undefined
        return: () => {},
      };
    },
  };
}).toThrow(TypeError);

expect(() => {
  var [] = {
    [Symbol.iterator]: () => {
      return {
        // Should not throw when `return()` returns an object
        return: () => ({}),
      };
    },
  };
}).not.toThrow();

- if (!_n && _i["return"] != null) _i["return"]();
+ if (!_n && _i["return"] != null) {
+   _r = _i["return"]();
+   if (Object(_r) !== _r) return;
+ }

And we got 226 -> 254 new successes in test262 now!

[liuxingbaoyu]

By the way, is there a place that shows the overall test262 result of Babel?

You can download the artifact in .tap format, or see how many successes and failures there are in CI, there is no visualization yet.
https://github.com/babel/babel/actions/runs/3445535140

[SuperSodaSea]

@nicolo-ribaudo The next is only evaluated only once now :) The code now looks like this:

babel/packages/babel-helpers/src/helpers.ts

Lines 792 to 815 in 4f951f3

	try {
	_x = (_i = _i.call(arr)).next;
	if (i === 0) {
	if (Object(_i) !== _i) return;
	_n = false;
	} else {
	for (; !(_n = (_s = _x.call(_i)).done); _n = true) {
	_arr.push(_s.value);
	if(_arr.length === i) break;
	}
	}
	} catch (err) {
	_d = true;
	_e = err;
	} finally {
	try {
	if (!_n && _i["return"] != null) {
	_r = _i["return"]();
	if (Object(_r) !== _r) return;
	}
	} finally {
	if (_d) throw _e;
	}
	}

Noticing that when break the _n should be false, so _arr.length < i can't be put in the for condition.

[SuperSodaSea]

Are there any further changes needed for this PR?

[liuxingbaoyu]

Thank you for the great work you put into this!

[liuxingbaoyu]

@SuperSodaSea Let's wait, Nicolò is sick, so it might be a while before he can review this.🙂

[SuperSodaSea]

Oh, I'm sorry to hear that. Hope he can make a full recovery soon:pray:

[nicolo-ribaudo]

Thank you! :)

[nicolo-ribaudo]

# 0 new failures, 254 new successes