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 over-zealous traversal by object-rest-spread #7388

Merged
merged 1 commit into from Feb 17, 2018

Conversation

@jamesreggio
Copy link
Contributor

jamesreggio commented Feb 16, 2018

Q                       A
Fixed Issues?
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? 👍
Documentation PR
Any Dependency Changes?
License MIT

Prior to this change, we'd conduct an open-ended traversal on the 'id' of any VariableDeclarator to find a RestElement. The 'id' of a VariableDeclarator can contain an AssignmentPattern (to supply
a default value), and if the right-hand side of the AssignmentPattern contained a RestElement, we'd transform it.

The problem here is that the right-hand side of an AssignmentPattern can be any Expression. If the right-hand side is a function body, we'd traverse the entire function body, and if a RestElement occurred anywhere in that function body, we'd transform it and emit the transformations wherever we began the traversal (at least one scope outside its usage).

The fix is to stop the inner traversal if we encounter an AssignmentPattern. The outer traversal will still visit the AssignmentPattern, so RestElements within the right-hand side of an AssignmentPattern will be properly transformed at that time.

This is my first time ever working with the Babel/Bablyon codebase, so I'd appreciate some extra scrutiny on my logic here. I'm afraid there may be some edge-case I've wrongly short-circuited.

Repro

In

function fn0(obj0) {
  const {
    fn1 = (obj1 = {}) => {
      const {
        fn2 = (obj2 = {}) => {
          const {a, ...rest} = obj2;
          console.log(rest);
        }
      } = obj1;
    }
  } = obj0;
}

Broken output

Notice that the rest = babelHelpers.objectWithoutProperties(obj0, ["a"]); call is on the outermost scope, where the inner traversal began.

function fn0(obj0) {
  const {
    fn1 = (obj1 = {}) => {
      const {
        fn2 = (obj2 = {}) => {
          const {
            a
          } = obj2;
          console.log(rest);
        }
      } = obj1;
    }
  } = obj0,
        rest = babelHelpers.objectWithoutProperties(obj0, ["a"]);
}

Fixed output

function fn0(obj0) {
  const {
    fn1 = (obj1 = {}) => {
      const {
        fn2 = (obj2 = {}) => {
          const {
            a
          } = obj2,
                rest = babelHelpers.objectWithoutProperties(obj2, ["a"]);
          console.log(rest);
        }
      } = obj1;
    }
  } = obj0;
}
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Feb 16, 2018

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

Prior to this change, we'd conduct an open-ended traversal on the 'id'
of any VariableDeclarator to find a RestElement. The 'id' of
a VariableDeclarator can contain an AssignmentPattern (to supply
a default value), and if the right-hand side of the AssignmentPattern
contained a RestElement, we'd transform it.

The problem here is that the right-hand side of an AssignmentPattern can
be *any* Expression. If the right-hand side is a function body, we'd
traverse the entire function body, and if a RestElement occurred
anywhere in that function body, we'd transform it and emit the
transformations wherever we began the traversal (at least one scope
outside its usage).

The fix is to stop the inner traversal if we encounter an
AssignmentPattern. The outer traversal will still visit the
AssignmentPattern, so RestElements within the right-hand side of an
AssignmentPattern will be properly transformed at that time.
@jamesreggio jamesreggio force-pushed the jamesreggio:fix-object-rest-spread branch from cd61bab to 7a7841a Feb 16, 2018
@jridgewell jridgewell merged commit 6cbc585 into babel:master Feb 17, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87.29% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Feb 17, 2018

Good catch!

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Feb 17, 2018

This PR introduces a regression:

const { x: { ...a } = {} } = {};

Also, it doesn't account for rest elements in computed properties name:

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

They are both fixed by #7364

@jamesreggio

This comment has been minimized.

Copy link
Contributor Author

jamesreggio commented Feb 19, 2018

Sorry about that, @nicolo-ribaudo. This was my first contribution to Babel and I explicitly asked for additional scrutiny because I was afraid that I may have been over-zealous in my traversal skipping.

I appreciate you fixing this quickly.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Feb 19, 2018

No worries 😉

@jamesreggio jamesreggio deleted the jamesreggio:fix-object-rest-spread branch Feb 19, 2018
aminmarashi-binary added a commit to aminmarashi-binary/babel that referenced this pull request Mar 17, 2018
Prior to this change, we'd conduct an open-ended traversal on the 'id'
of any VariableDeclarator to find a RestElement. The 'id' of
a VariableDeclarator can contain an AssignmentPattern (to supply
a default value), and if the right-hand side of the AssignmentPattern
contained a RestElement, we'd transform it.

The problem here is that the right-hand side of an AssignmentPattern can
be *any* Expression. If the right-hand side is a function body, we'd
traverse the entire function body, and if a RestElement occurred
anywhere in that function body, we'd transform it and emit the
transformations wherever we began the traversal (at least one scope
outside its usage).

The fix is to stop the inner traversal if we encounter an
AssignmentPattern. The outer traversal will still visit the
AssignmentPattern, so RestElements within the right-hand side of an
AssignmentPattern will be properly transformed at that time.
@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.