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

path.replaceWith causes stack overflow #9413

Open
FireCubez opened this Issue Jan 26, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@FireCubez
Copy link

FireCubez commented Jan 26, 2019

Bug Report

Current Behavior
A stack overflow.

Input Code
ASTExplorer

a + 1

// Replace binary operators
module.exports = function({ types: t }) {
  return {
    visitor: {
      BinaryExpression(path) {
        path.replaceWith(
          t.binaryExpression("**", path.node.left, t.numberLiteral(2))
        );
      }
    }
  };
};

Expected behavior/code
Expected output of a ** 2

Babel Configuration (.babelrc, package.json, cli command)

{
  "plugins": ["./myplugin.js"]
}

Environment

  • Babel version(s): v7.2.2
  • Node/npm version: Node v10.15.0/npm 6.4.1
  • OS: Windows 10 64 bit
  • Monorepo: ??
  • How you are using Babel: Programmatically (babel.transform)
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Jan 26, 2019

Hey @FireCubez! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community
that typically always has someone willing to help. You can sign-up here
for an invite.

@motiz88

This comment has been minimized.

Copy link
Contributor

motiz88 commented Jan 26, 2019

I don't know that there's much Babel could do in the general case here. You could trivially modify your visitor function to check whether the node needs transforming, and then call replaceWith only as needed rather than unconditionally.

@FireCubez

This comment has been minimized.

Copy link
Author

FireCubez commented Jan 26, 2019

Do you mean this isn't a bug? In the babel handbook, this is given as a working example, yet it doesn't work and throws an error.

@motiz88

This comment has been minimized.

Copy link
Contributor

motiz88 commented Jan 26, 2019

That's a good point. I'm not sure whether that particular snippet was meant to be runnable on its own, and that might be useful to clarify in the handbook. Personally I think the intention was to show how and in what context to use replaceWith. A similar but runnable example might look something like this:

BinaryExpression(path) {
  if (path.node.operator === '**') return;
  path.replaceWith(
    t.binaryExpression("**", path.node.left, t.numberLiteral(2))
  );
}
@FireCubez

This comment has been minimized.

Copy link
Author

FireCubez commented Jan 27, 2019

So I make sure I understand correctly, path.replaceWith calls the handler of the node it replaces? This is my code:

Expression(path) {
  var callee = path.node.callee;
  if (t.isMemberExpression(callee) && t.isIdentifier(callee.object, {
    name: "shup"
  })) return
  path.replaceWith(
    t.callExpression(
      t.memberExpression(
        t.identifier("shup"),
        t.identifier("expression")
      ),
      [
        path.node
      ]
    )
  )
}

But that also causes a stack overflow. If I add path.skip() before the return statement it works, but skips traversing some of the nodes. For example for an input of a().b.c I would expect an output of:

shup.expression(shup.expression(shup.expression(shup.expression(a)()).b).c)

but instead get:

shup.expression(a().b.c)
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 27, 2019

This is what your code does, because shup is itself an expression:

Current code Current visited node Should replace? Replace with Output
a().b.c a().b.c Yes shup.expression(a().b.c) shup.expression(a().b.c)
shup.expression(a().b.c) shup.expression No
shup.expression(a().b.c) shup Yes shup.expression(shup) shup.expression(shup).expression(a().b.c)
shup.expression(shup).expression(a().b.c) shup.expression No
shup.expression(shup).expression(a().b.c) shup (the left one) Yes shup.expression(shup) shup.expression(shup).expression(shup).expression(a().b.c)
...
@FireCubez

This comment has been minimized.

Copy link
Author

FireCubez commented Jan 27, 2019

Ah, thanks. I fixed this by adding a special "__shupIgnore__" property to the replacement and adding a check for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment