Thanks!

Can you add a test case with mixed arrays?

Just pushed up a commit. Thanks for the recommendation, it caught an edge case with the implementation.

Can you add a new test case?

const { x: [...{ x, ...y }] } = z();

A RestElement's argument can be a pattern so we have to recurse into it.

Great point! I added the test case and confirmed the output looks as I'd expect.

We can not return early when properties is falsy, instead we should invoke the recursive function on .argument. For example,

const { x: [...{ ...y }] } = z();

has only one binding identifier but doesAnyChildNodeHaveMoreThanOneProperty returns true.

Hey @JLHwung - thanks for the the detailed case. I looked through https://astexplorer.net/ and believe I was able to diagnose the issue you called out with more robust test cases for array destructuring.

Happy to make any other adjustments. Thank you for the thorough review and prompt responses!

Unrelated to this PR: I think we should use originalPath.scope.isStatic(originalPath.node.init) here, because

1. A ThisExpression is safe to reuse, e.g.

const { ...x } = this;

2. Even if node.init is an identifier, it can not be reused if not a constant binding. e.g.

let z = { v: 1 };
function sideEffect() {
  z = { v: 2 };
}
const { x = sideEffect(), ...y } = z;
y // { v: 1 }

y becomes { v: 2 } after transformed because z can be modified in sideEffect().

These two issues will be fixed by scope.isStatic, they can be addressed in a subsequent PR.

Similar to RestElement, we need to recurse into node.left.

innerNode ??= node.left

So I originally added that but the output did not end up as I expected. Specifically the RHS was duplicated. I believe the way that we handle assignment patterns also duplicates the RHS.

See this diff that I branched off this branch for: https://github.com/dan-kez/babel/pull/1/files

Oh I was meant to replace the whole "if" branch with innerNode ??= node.left, which is similar to how we handle the array / object patterns (note that in your posted branch it was changed to innerNode = innerNode.left). The innerNode is essentially the arguments of next recursive call, so the algorithm can be summarized as

function hasMoreThanOneBinding(node) {
  if (node is ArrayPattern ) {
    return true if node has more than one elements
    else return hasMoreThanOneBinding(node's first element)
  } else if (node is ObjectPattern) {
    return true if node has more than one properties
    else return hasMoreThanOneBinding(node's first property value)
  } else if (node is AssignmentPattern) {
    return hasMoreThanOneBinding(node's left)
  } else if (node is RestElement) {
    return hasMoreThanOneBinding(node's argument)
  } else { // node is Identifier or MemberExpression
    return true
  }
}

Ah I understand now. Apologies for all the back and forth here - I really appreciate you taking the time on this PR!

I've refactored the function to be more in line with you example. I did add one additional case ObjectProperty. This was because an ObjectPattern could have the first element a RestElement (for example const { x15: { ...y15 } } = z();). In this case node.properties[0].value would be undefined which would fall through the final else clause.

To be more specific I added an ObjectProperty case where I test on the presence of node?.value and then recurse on the given value.

If this is wrong please let me know - I'm happy to change it.

I also used optional chaining just in case I missed a case were node could be falsey (this was how I found the above issue when stepping through the test case). I am also happy to add an early return if node is undefined to return true or other cases that could be present in this LHS assignment.

This branch is reachable because visitRestElements applies to rest elements in array patterns, too. E.g.

const { x: [ ...{} ] } = { x: [] }

Q: Since this is a single identifier, shouldn't it return false? (it's one binding)

It looks like this test case fails if I make this false

input:

const { x15: [...{ ...y15 }] } = z();

output:

const {} = z(),
      y15 = babelHelpers.extends({}, z().x15);

Ah I believe I have a fix. In the event node.argument is an Identifier then we can early return true and this these test cases work out.

Actually, I think we don't need to return true for rest but the fix needs to live somewhere else.

In the const { x15: [...{ ...y15 }] } = z(); case, we don't need to cache z(): we can just skip the {} = z() one.

Would it be ok to consider making this change in a separate PR as an optimization? My main goal for this is to mitigate duplicating the right hand assignment. I'd prefer avoiding doing a larger refactor of the greater plugin if possible.

Yes, that's ok!

It should return false, the source contains only one binding y4. Also we don't have to add number suffix since every snippet is parsed independently.

So I think hasMoreThanOneBinding is an invalid name for the function. The end goal of this is to strictly stop the right hand side being duplicated

I am happy to rename this function or do something else but in the end the main goal is to mitigate the active bug.

These two cases are almost identical, we can remove one of them.

Should return false.

Should return false.

Should return false.

Should return false and almost identical examples.

Should return false.

Should return false

These four examples also contains only one binding, and thus should return false.

This is a very interesting case. It shows that we can't simply return true if node.properties > 1, since the property value may be an empty object pattern, or nested pattern.

We may need a new algorithm similar to

Essentially the hasMoreThanOneBinding should be equivalent to

Object.keys(getBindingIdentifiers(node)).length > 1