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

plugin-transform-block-scoping renames function declaration #10046

Open
mischnic opened this issue May 30, 2019 · 8 comments

Comments

Projects
None yet
3 participants
@mischnic
Copy link

commented May 30, 2019

Bug Report

Current Behavior
@babel/plugin-transform-block-scoping (and maybe some other plugin in preset-env) incorrectly renames a FunctionDeclaration in a nested block even though it should behave like a var declaration.

Input Code
REPL

if(true) {
  function run() {
    return true;
  }
}

function test() {
  return run();
}

Note: turning function run() { into var run = function() { behaves correctly.

Expected behavior/code
Same as Input

Actual output

if (true) {
  function _run() { // <-------
    return true;
  }
}

function test() {
  return run();
}

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

{
  "plugins": ["@babel/plugin-transform-block-scoping"]
}

or

{
  "presets": ["@babel/preset-env"]
}

Environment

  • Babel version(s): 7.4.0
  • Node/npm version: Node 10
  • OS: macOS
  • Monorepo: -
  • How you are using Babel: via babel-core
@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented May 30, 2019

Hey @mischnic! 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.

@mischnic

This comment has been minimized.

Copy link
Author

commented May 30, 2019

Not sure if this is related: in the example above, the function inside the if block isn't on the program scope

@mischnic

This comment has been minimized.

Copy link
Author

commented May 30, 2019

How I would expect it to behave:

diff --git a/packages/babel-plugin-transform-block-scoping/src/index.js b/packages/babel-plugin-transform-block-scoping/src/index.js
index cc63c554..cdf9b863 100644
--- a/packages/babel-plugin-transform-block-scoping/src/index.js
+++ b/packages/babel-plugin-transform-block-scoping/src/index.js
@@ -701,11 +701,7 @@ class BlockScoping {
 
     const addDeclarationsFromChild = (path, node) => {
       node = node || path.node;
-      if (
-        t.isClassDeclaration(node) ||
-        t.isFunctionDeclaration(node) ||
-        isBlockScoped(node)
-      ) {
+      if (t.isClassDeclaration(node) || isBlockScoped(node)) {
         if (isBlockScoped(node)) {
           convertBlockScopedToVar(path, node, block, this.scope);
         }
diff --git a/packages/babel-traverse/src/scope/index.js b/packages/babel-traverse/src/scope/index.js
index 375d5827..2d1e4149 100644
--- a/packages/babel-traverse/src/scope/index.js
+++ b/packages/babel-traverse/src/scope/index.js
@@ -131,6 +131,13 @@ const collectorVisitor = {
     scope.getBlockParent().registerDeclaration(path);
   },
 
+  FunctionDeclaration(path) {
+    let scope =
+      path.parentPath.scope.getFunctionParent() ||
+      path.parentPath.scope.getProgramParent();
+    scope.registerDeclaration(path);
+  },
+
   ClassDeclaration(path) {
     const id = path.node.id;
     if (!id) return;
diff --git a/packages/babel-types/src/validators/isBlockScoped.js b/packages/babel-types/src/validators/isBlockScoped.js
index 51ae6e07..a02e53c9 100644
--- a/packages/babel-types/src/validators/isBlockScoped.js
+++ b/packages/babel-types/src/validators/isBlockScoped.js
@@ -6,5 +6,5 @@ import isLet from "./isLet";
  * Check if the input `node` is block scoped.
  */
 export default function isBlockScoped(node: Object): boolean {
-  return isFunctionDeclaration(node) || isClassDeclaration(node) || isLet(node);
+  return isClassDeclaration(node) || isLet(node);
 }
@mischnic

This comment has been minimized.

Copy link
Author

commented May 31, 2019

I would love to make a PR out of this, but without any comment on whether FunctionDeclaration is actually a BlockScoped or not....

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Block scoped functions are hard to get right, since they behave differently in loose and strict mode.
Does that patch break any of our existing tests?

@mischnic

This comment has been minimized.

Copy link
Author

commented May 31, 2019

Yes it does 😐 :

scope › duplicate bindings › let and function in sub scope
scope › duplicate bindings › const and function in sub scope

(let foo;{function foo() {}} and const foo;{function foo() {}} seem to throw "Duplicate declaration" now -- the first works in v8 and foo is undefined, the second throws Missing initializer in const declaration)

babel-plugin-transform-block-scoping/general › issue 1051
babel-plugin-transform-block-scoping/general › issue 4363
babel-plugin-transform-block-scoping/general › switch
babel-plugin-transform-parameters/parameters › default iife 4253
babel-plugin-transform-parameters/parameters › default iife self
Details
Summary of all failing tests
 FAIL  packages/babel-traverse/test/scope.js
  ● scope › duplicate bindings › let and function in sub scope

    expect(received).not.toThrow()

    Error name:    "Error"
    Error message: "Duplicate declaration \"foo\""

          37 |   NodePath.get({
          38 |     hub: {
        > 39 |       buildError: (_, msg) => new Error(msg),
             |                               ^
          40 |     },
          41 |     parentPath: null,
          42 |     parent: ast,

          at Object.buildError (packages/babel-traverse/test/scope.js:39:31)
          at Scope.checkBlockScopedCollisions (packages/babel-traverse/lib/scope/index.js:349:22)
          at Scope.registerBinding (packages/babel-traverse/lib/scope/index.js:506:16)
          at Scope.registerDeclaration (packages/babel-traverse/lib/scope/index.js:441:12)
          at Object.FunctionDeclaration (packages/babel-traverse/lib/scope/index.js:192:11)
          at NodePath._call (packages/babel-traverse/lib/path/context.js:53:20)
          at NodePath.call (packages/babel-traverse/lib/path/context.js:40:17)
          at NodePath.visit (packages/babel-traverse/lib/path/context.js:88:12)
          at TraversalContext.visitQueue (packages/babel-traverse/lib/context.js:118:16)
          at TraversalContext.visitMultiple (packages/babel-traverse/lib/context.js:85:17)

      324 |         ];
      325 |
    > 326 |         expect(() => getPath(ast)).not.toThrow();
          |                                        ^
      327 |       });
      328 |     });
      329 |

      at Object.toThrow (packages/babel-traverse/test/scope.js:326:40)

  ● scope › duplicate bindings › const and function in sub scope

    expect(received).not.toThrow()

    Error name:    "Error"
    Error message: "Duplicate declaration \"foo\""

          37 |   NodePath.get({
          38 |     hub: {
        > 39 |       buildError: (_, msg) => new Error(msg),
             |                               ^
          40 |     },
          41 |     parentPath: null,
          42 |     parent: ast,

          at Object.buildError (packages/babel-traverse/test/scope.js:39:31)
          at Scope.checkBlockScopedCollisions (packages/babel-traverse/lib/scope/index.js:349:22)
          at Scope.registerBinding (packages/babel-traverse/lib/scope/index.js:506:16)
          at Scope.registerDeclaration (packages/babel-traverse/lib/scope/index.js:441:12)
          at Object.FunctionDeclaration (packages/babel-traverse/lib/scope/index.js:192:11)
          at NodePath._call (packages/babel-traverse/lib/path/context.js:53:20)
          at NodePath.call (packages/babel-traverse/lib/path/context.js:40:17)
          at NodePath.visit (packages/babel-traverse/lib/path/context.js:88:12)
          at TraversalContext.visitQueue (packages/babel-traverse/lib/context.js:118:16)
          at TraversalContext.visitMultiple (packages/babel-traverse/lib/context.js:85:17)

      324 |         ];
      325 |
    > 326 |         expect(() => getPath(ast)).not.toThrow();
          |                                        ^
      327 |       });
      328 |     });
      329 |

      at Object.toThrow (packages/babel-traverse/test/scope.js:326:40)

 FAIL  packages/babel-plugin-transform-block-scoping/test/index.js (10.67s)
  ● babel-plugin-transform-block-scoping/general › issue 1051

    Expected ..../babel/packages/babel-plugin-transform-block-scoping/test/fixtures/general/issue-1051/output.js to match transform output.
    To autogenerate a passing version of this file, delete the file and re-run the tests.

    Diff:

    - Expected
    + Received

      foo.func1 = function () {
        if (cond1) {
          for (;;) {
            if (cond2) {
    -         var _ret = function () {
    -           function func2() {}
    +         var func2 = function () {};

    -           function func3() {}
    +         var func3 = function () {};

    -           func4(function () {
    -             func2();
    -           });
    -           return "break";
    -         }();
    -
    -         if (_ret === "break") break;
    +         func4(function () {
    +           func2();
    +         });
    +         break;
            }
          }
        }
      };

      333 |
      334 |       try {
    > 335 |         expect(actualCode).toEqualFile({
          |                            ^
      336 |           filename: expected.loc,
      337 |           code: expectCode
      338 |         });

      at run (packages/babel-helper-transform-fixture-test-runner/lib/index.js:335:28)
      at runTask (packages/babel-helper-transform-fixture-test-runner/lib/index.js:412:13)
      at Object.<anonymous> (packages/babel-helper-transform-fixture-test-runner/lib/index.js:436:15)

  ● babel-plugin-transform-block-scoping/general › issue 4363

    Expected ..../babel/packages/babel-plugin-transform-block-scoping/test/fixtures/general/issue-4363/output.js to match transform output.
    To autogenerate a passing version of this file, delete the file and re-run the tests.

    Diff:

    - Expected
    + Received

      function WithoutCurlyBraces() {
    -   var _this = this;
    +   if (true) for (var k in kv) {
    +     var foo = function () {
    +       return this;
    +     };

    -   if (true) {
    -     var _loop = function (k) {
    -       function foo() {
    -         return this;
    -       }
    -
    -       function bar() {
    -         return foo.call(this);
    -       }
    -
    -       console.log(_this, k); // => undefined
    +     var bar = function () {
    +       return foo.call(this);
          };

    -     for (var k in kv) {
    -       _loop(k);
    -     }
    +     console.log(this, k); // => undefined
        }
      }

      function WithCurlyBraces() {
    -   var _this2 = this;
    -
        if (true) {
    -     var _loop2 = function (k) {
    -       function foo() {
    +     for (var k in kv) {
    +       var foo = function () {
              return this;
    -       }
    +       };

    -       function bar() {
    +       var bar = function () {
              return foo.call(this);
    -       }
    +       };

    -       console.log(_this2, k); // => 777
    -     };
    -
    -     for (var k in kv) {
    -       _loop2(k);
    +       console.log(this, k); // => 777
          }
        }
      }

      333 |
      334 |       try {
    > 335 |         expect(actualCode).toEqualFile({
          |                            ^
      336 |           filename: expected.loc,
      337 |           code: expectCode
      338 |         });

      at run (packages/babel-helper-transform-fixture-test-runner/lib/index.js:335:28)
      at runTask (packages/babel-helper-transform-fixture-test-runner/lib/index.js:412:13)
      at Object.<anonymous> (packages/babel-helper-transform-fixture-test-runner/lib/index.js:436:15)

  ● babel-plugin-transform-block-scoping/general › switch

    Expected ..../babel/packages/babel-plugin-transform-block-scoping/test/fixtures/general/switch/output.js to match transform output.
    To autogenerate a passing version of this file, delete the file and re-run the tests.

    Diff:

    - Expected
    + Received

    @@ -25,8 +25,8 @@
          var _d = 5;

        case false:
          class _e {}

    -     var _f = function () {};
    +     var f = function () {};

      }

      333 |
      334 |       try {
    > 335 |         expect(actualCode).toEqualFile({
          |                            ^
      336 |           filename: expected.loc,
      337 |           code: expectCode
      338 |         });

      at run (packages/babel-helper-transform-fixture-test-runner/lib/index.js:335:28)
      at runTask (packages/babel-helper-transform-fixture-test-runner/lib/index.js:412:13)
      at Object.<anonymous> (packages/babel-helper-transform-fixture-test-runner/lib/index.js:436:15)

 FAIL  packages/babel-plugin-transform-parameters/test/index.js (14.516s)
  ● babel-plugin-transform-parameters/parameters › default iife 4253

    Expected ..../babel/packages/babel-plugin-transform-parameters/test/fixtures/parameters/default-iife-4253/output.js to match transform output.
    To autogenerate a passing version of this file, delete the file and re-run the tests.

    Diff:

    - Expected
    + Received

    @@ -3,12 +3,14 @@
      function () {
        "use strict";

        function Ref() {
          var id = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : ++Ref.nextID;
    -     babelHelpers.classCallCheck(this, Ref);
    -     this.id = id;
    +     return function () {
    +       babelHelpers.classCallCheck(this, Ref);
    +       this.id = id;
    +     }.apply(this);
        }

        return Ref;
      }();


      333 |
      334 |       try {
    > 335 |         expect(actualCode).toEqualFile({
          |                            ^
      336 |           filename: expected.loc,
      337 |           code: expectCode
      338 |         });

      at run (packages/babel-helper-transform-fixture-test-runner/lib/index.js:335:28)
      at Object.<anonymous> (packages/babel-helper-transform-fixture-test-runner/lib/index.js:430:30)

  ● babel-plugin-transform-parameters/parameters › default iife self

    Expected ..../babel/packages/babel-plugin-transform-parameters/test/fixtures/parameters/default-iife-self/output.js to match transform output.
    To autogenerate a passing version of this file, delete the file and re-run the tests.

    Diff:

    - Expected
    + Received

    @@ -3,12 +3,14 @@
      function () {
        "use strict";

        function Ref() {
          var ref = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : Ref;
    -     babelHelpers.classCallCheck(this, Ref);
    -     this.ref = ref;
    +     return function () {
    +       babelHelpers.classCallCheck(this, Ref);
    +       this.ref = ref;
    +     }.apply(this);
        }

        return Ref;
      }();


      333 |
      334 |       try {
    > 335 |         expect(actualCode).toEqualFile({
          |                            ^
      336 |           filename: expected.loc,
      337 |           code: expectCode
      338 |         });

      at run (packages/babel-helper-transform-fixture-test-runner/lib/index.js:335:28)
      at Object.<anonymous> (packages/babel-helper-transform-fixture-test-runner/lib/index.js:430:30)

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 31, 2019

The second one should be something like const foo = 2; { function foo() {} }. I'm suprised that it worked before 🤔

This seems to be a deeper problem in @babel/traverse (which will probably be harder to fix), or something else is buggy in the transform-block-scoping plugin.
If you are interseted in knowing the exact semantics of block-level functions, you can read tis part of the spec: https://tc39.github.io/ecma262/#sec-block-level-function-declarations-web-legacy-compatibility-semantics

If you want, you could open a WIP pr 😉

@mischnic mischnic referenced a pull request that will close this issue May 31, 2019

Draft

FunctionDeclaration shouldn't be BlockScoped #10050

@mischnic

This comment has been minimized.

Copy link
Author

commented May 31, 2019

If you want, you could open a WIP pr 😉

😄, done: #10050

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.