Kill the "shadow-functions.js" internal plugin in favor of an explicit helper #5677

loganfsmyth · 2017-04-28T05:40:56Z

Q	A
Patch: Bug Fix?	Y
Major: Breaking Change?	Y, if something was relying on this stuff not being unwrapped immediately, or was using the `_forceShadow`, `.shadow` or `._shadowedFunctionLiteral` properties. Hopefully minimal though.
Minor: New Feature?	Maybe?
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets	Fixes #5597, Fixes #3930
License	MIT
Doc PR
Dependency Changes

This removes the private "shadow"-related properties from the AST and relies on explicit traversal to handle the conversion of arrow functions into non-arrow functions.

I've also added support for handling super and new.target into the generic utility to handle cases that we could not handle properly with the old implementation.

This also has the added benefit of not performing unexpected transformations on the AST when no plugins were enabled by the user, since we've had users reporting this as an annoyance on several occasions. This is also a bit of a blocker for any work trying to make Babel a more general codemod-like tool.

mention-bot · 2017-04-28T05:40:57Z

@loganfsmyth, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @existentialism and @motiz88 to be potential reviewers.

loganfsmyth · 2017-04-28T05:43:13Z

...n-transform-async-generator-functions/test/fixtures/nested/arrows-in-declaration/expected.js

-    (function () {
-      return _this;
-    });
+    () => this;


These changes are from the removed call on https://github.com/babel/babel/pull/5677/files#diff-3953cdd7a44c58e0431a2e202efb64d4L31 that wasn't needed.

loganfsmyth · 2017-04-28T05:43:51Z

...orm-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/expected.js

@@ -0,0 +1,11 @@
+class Foo extends class {} {
+  method() {
+    var _superprop_callMethod = (..._args) => super.method(..._args);


It's not pretty, but it's the only way we can deal with this.

This looks okay to me. Nice job, you use the same reference to the super.method call (instead of two separate) 👍

loganfsmyth · 2017-04-28T05:45:17Z

packages/babel-plugin-transform-es2015-block-scoping/src/index.js

@@ -416,56 +416,79 @@ class BlockScoping {
    // build the closure that we're going to wrap the block with, possible wrapping switch(){}
    const fn = t.functionExpression(null, params,
      t.blockStatement(isSwitch ? [block] : block.body));
-    fn.shadow = true;


This whole set of changes is gross and probably the most likely to be missing edge cases, but it works as far as I can tell.

These changes were all needed so that we can get access to the final NodePath for the function, since it wasn't available anywhere before.

loganfsmyth · 2017-04-28T05:45:56Z

...n-transform-es2015-block-scoping/test/fixtures/general/for-break-continue-return/expected.js

@@ -1,5 +1,5 @@
 (function () {
-  var _loop = function (i) {
+  var _loop2 = function (i) {


A change in ordering in block scoping means these variable names are generated in opposite order now, thus breaking the test.

loganfsmyth · 2017-04-28T05:46:55Z

packages/babel-plugin-transform-es2015-classes/src/index.js

@@ -46,6 +46,10 @@ export default function ({ types: t }) {
        if (state.opts.loose) Constructor = LooseTransformer;

        path.replaceWith(new Constructor(path, state.file).run());
+
+        if (path.isCallExpression() && path.get("callee").isArrowFunctionExpression()) {
+          path.get("callee").arrowFunctionToExpression();


We don't really have a firm policy for when it's cool to insert actual arrows, so I added a few new arrows in place of .shadow but explicitly converted them to keep the tests passing.

Would hope that the dependency system would allow us to add/remove, preferring to instead arrows/newer syntax

Yeah, we can always drop this now if we want, since the plugins are already in a fine order for it AFAIK, I just figured it would be better to leave that for a separate PR.

loganfsmyth · 2017-04-28T05:48:49Z

packages/babel-plugin-transform-es2015-object-super/src/index.js


-      ObjectExpression: {
-        exit(path, file) {


This implementation had to change because the usage of the exit handler meant that the arrow transform would fail for

var obj = { method(){ var fn = () => super.method(); } };

since it can't handle super, meaning that for object-super to work, it needs to run before the arrow transform.

So there's no need for the CONTAINS_SUPER check anymore?

I don't think there's a good reason to have it if we've not using the exit handler, since the main benefit of it was avoiding traversing the methods more than once. and there's no way to avoid that without the exit, but the exit would run it all in the wrong order.

loganfsmyth · 2017-04-28T05:49:53Z

packages/babel-traverse/src/path/replacement.js

@@ -239,6 +238,8 @@ export function replaceExpressionWithStatements(nodes: Array<Object>) {
      }
    }

+    this.get("callee").arrowFunctionToExpression();


Again just to make the tests pass, we could also leave this out and insert real arrows if we want to.

loganfsmyth · 2017-04-28T05:50:16Z

packages/babel-types/src/definitions/es2015.js

@@ -245,10 +245,10 @@ defineType("MetaProperty", {
  fields: {
    // todo: limit to new.target
    meta: {
-      validate: assertValueType("string"),
+      validate: assertNodeType("Identifier"),


These were just wrong :P

codecov · 2017-04-28T05:58:17Z

Codecov Report

Merging #5677 into 7.0 will decrease coverage by 0.03%.
The diff coverage is 92.24%.

@@            Coverage Diff             @@
##              7.0    #5677      +/-   ##
==========================================
- Coverage   84.61%   84.57%   -0.04%     
==========================================
  Files         283      282       -1     
  Lines        9744     9857     +113     
  Branches     2736     2766      +30     
==========================================
+ Hits         8245     8337      +92     
- Misses        986      999      +13     
- Partials      513      521       +8

Impacted Files	Coverage Δ
...bel-plugin-transform-es2015-parameters/src/rest.js	`98.14% <ø> (-0.04%)`	⬇️
packages/babel-types/src/definitions/es2015.js	`96.15% <ø> (ø)`	⬆️
...-plugin-transform-es2015-parameters/src/default.js	`89.09% <ø> (-0.2%)`	⬇️
...ckages/babel-core/src/transformation/file/index.js	`87.14% <ø> (ø)`	⬆️
packages/babel-messages/src/index.js	`61.53% <ø> (ø)`	⬆️
packages/babel-traverse/src/path/ancestry.js	`78.08% <ø> (-2.65%)`	⬇️
...babel-plugin-transform-es2015-classes/src/index.js	`86.36% <100%> (+1.36%)`	⬆️
...bel-plugin-transform-es2015-classes/src/vanilla.js	`90.12% <100%> (-0.47%)`	⬇️
...gin-transform-es2015-modules-commonjs/src/index.js	`94.47% <100%> (-0.03%)`	⬇️
packages/babel-traverse/src/path/replacement.js	`74.33% <100%> (ø)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86a5377...7a9d330. Read the comment docs.

hzoo · 2017-04-28T19:58:20Z

...babel-plugin-transform-es2015-arrow-functions/test/fixtures/arrow-functions/spec/expected.js

@@ -14,7 +14,7 @@ function foo() {

    return function () {
      babelHelpers.newArrowCheck(this, _this2);
-      return this;


👍 looks like this used to be a bug too?

Since it is .bind both work, this way is just a little more consistent with the non-spec arrow transform, since they share more code now.

I'd prefer if we could keep this as this, but it's not like it's incorrect code here. Minifies better too!

Up for expanding on your reasoning? My motivation was mostly that it's less separation between spec and non-spec mode, but it could be made to work either way I guess.

Alright, I added support for spec arrow in subclass constructors, and made this stay this in spec arrows except for the subclass constructor case.

xtuc · 2017-04-29T11:18:04Z

...orm-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/expected.js

@@ -0,0 +1,11 @@
+class Foo extends class {} {
+  method() {
+    var _superprop_callMethod = (..._args) => super.method(..._args);


This looks okay to me. Nice job, you use the same reference to the super.method call (instead of two separate) 👍

xtuc · 2017-04-29T11:27:02Z

packages/babel-traverse/src/path/ancestry.js

- * Please Note, these flags are for private internal use only and should be avoided.
- * Only "shadow" is a public property that other transforms may manipulate.
- */
-


Could we keep this comment. It helped me understand what shadow'ing meant.

Happy to chat if you have suggestions. This PR mostly removes the whole concept of "shadowing" that we had before, in favor of fixing everything up right away, so it's not obvious where we'd put this, or if it would be useful to people at this point.

Yes, I you did great by removing unnecessary complexity. The term variable shadowing as explained in the wiki there is confusing to me. It seems that shadowing wasn't the correct concept for passing this since it's just a transpilation technique. We could maybe chat about that on Slack if you have time?

It looks like the shadow transform was actually used for _un_shadowing 😆

Yeaaah talk about confusing naming.

I talked with @loganfsmyth and now I'm fine with that.

Jessidhia · 2017-05-03T03:36:53Z

packages/babel-helper-remap-async-to-generator/src/index.js

@@ -95,6 +90,9 @@ function classOrObjectMethod(path: NodePath, callId: Object) {
  // Regardless of whether or not the wrapped function is a an async method
  // or generator the outer function should not be
  node.generator = false;
+
+  // Unwrap the wrapper IIFE's environment so super and this and such still work.
+  path.get("body.body.0.argument.callee.arguments.0").unwrapFunctionEnvironment();


wth is body.body.0.argument.callee.arguments.0 😆

It's gross garbage :P It's reaching in to the path where we put the IIFE, since we only get access to the path after it is inserted, so we have to insert it then do this gross stuff to get the reference.

Jessidhia · 2017-05-03T03:40:50Z

...ansform-async-to-generator/test/fixtures/async-to-generator/deeply-nested-asyncs/expected.js

            for (var _len2 = arguments.length, innerArgs = Array(_len2 > 2 ? _len2 - 2 : 0), _key2 = 2; _key2 < _len2; _key2++) {
              innerArgs[_key2 - 2] = arguments[_key2];
            }

-            yield z;


Why did this yield move up?

I forget, but this change seems more like what it's supposed to do, so I didn't worry about it. It's possible I looked into the reason, but I don't remember anymore.

Jessidhia · 2017-05-03T03:43:50Z

packages/babel-plugin-transform-es2015-arrow-functions/src/index.js

-        }
+        path.arrowFunctionToExpression({
+          // While other utils may be fine inserting other arrows to make more transforms possible,
+          // the arrow transform itself absolutely cannot insert new arrow functions.


Because it would leave arrows in the output defeating the purpose, or because it could lead to infinite recursion? 🤔

Why not both? :P

Jessidhia · 2017-05-03T03:51:47Z

packages/babel-plugin-transform-es2015-block-scoping/src/index.js

-        t.variableDeclarator(ref, fn),
-      ]));
-    }
+    const has = this.has;


this.has was just written to a few lines ago in the same function; can it just write directly to const has, or are there things that look into this.has?

I think this was leftover from copy-pasting from https://github.com/babel/babel/pull/5677/files/c8ff06f90661f5e67a93b7e2f2d387a029edd98a#diff-775ae6452de339480d54604c7d217b5dL463

Looks like this.has is used one other place, but otherwise isn't updated, so you're right I could move this up farther.

Jessidhia · 2017-05-03T03:53:04Z

packages/babel-plugin-transform-es2015-block-scoping/src/index.js

    }

    // handlers async functions
    const hasAsync = traverse.hasType(fn.body, this.scope, "AwaitExpression", t.FUNCTION_TYPES);
    if (hasAsync) {
      fn.async = true;
      call = t.awaitExpression(call);
+      basePath = ".argument" + basePath;
    }


How does this break with async iterators?

Though anyone not transforming async iterators are certainly running on an engine where they can skip transforming block scoping... right Safari? <_<

No idea, I didn't realize this logic even did this until inspecting this code.

Jessidhia · 2017-05-03T03:58:36Z

packages/babel-traverse/src/path/ancestry.js

- * Please Note, these flags are for private internal use only and should be avoided.
- * Only "shadow" is a public property that other transforms may manipulate.
- */
-


It looks like the shadow transform was actually used for _un_shadowing 😆

Jessidhia · 2017-05-03T04:01:18Z

packages/babel-traverse/src/path/conversion.js

+        t.thisExpression(),
+        t.identifier(thisBinding),
+      ]))
+    );


Looks like I got you a merge conflict by merging #5620

No worries.

Jessidhia · 2017-05-03T04:03:06Z

...babel-plugin-transform-es2015-arrow-functions/test/fixtures/arrow-functions/spec/expected.js

@@ -14,7 +14,7 @@ function foo() {

    return function () {
      babelHelpers.newArrowCheck(this, _this2);
-      return this;


I'd prefer if we could keep this as this, but it's not like it's incorrect code here. Minifies better too!

Jessidhia · 2017-05-03T04:08:03Z

packages/babel-traverse/src/path/conversion.js

+    ThisExpression(child) {
+      thisPaths.push(child);
+    },
+    JSXIdentifier(child) {


Is this for <this.Foo/>?

It is, I guess I should make this JSXMemberExpression now that you mention it though.

Added some checks to explicitly verify the location of the identifier.

Jessidhia · 2017-05-03T04:11:36Z

packages/babel-traverse/test/arrow-transform.js

+      () => super();
+      () => this;
+    `, `
+      var _supercall = (..._args) => _this = super(..._args),


Was wondering if something would catch multiple super() invocations, but I guess that's the job of the JS engine / class transform.

That I think we should leave up to the class transform, it could certainly add logic for that.

…lugin.

loganfsmyth · 2017-05-03T08:41:59Z

Okay, I rebased to fix the merge conflict with spec function naming, and addressed the few issues people have raised except the one question of this vs _this in arrow spec mode. Happy to chat on that on Slack tomorrow.

…class constructors.

mikemorancodes · 2017-05-29T12:45:24Z

What if I just need to fix the #3930 issue?

loganfsmyth added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Apr 28, 2017

loganfsmyth added this to the Babel 7 milestone Apr 28, 2017

loganfsmyth commented Apr 28, 2017

View reviewed changes

hzoo reviewed Apr 28, 2017

View reviewed changes

xtuc reviewed Apr 29, 2017

View reviewed changes

Jessidhia reviewed May 3, 2017

View reviewed changes

loganfsmyth added 2 commits May 3, 2017 00:48

Handle arrow function processing via shared API rather than default p…

ce1f803

…lugin.

Fix a few small PR comments.

bb7a85e

loganfsmyth force-pushed the kill-shadow-functions branch from c8ff06f to bb7a85e Compare May 3, 2017 08:39

Preserve existing spec arrow 'this' rewrites, and support spec in sub…

7a9d330

…class constructors.

loganfsmyth merged commit 14584c2 into babel:7.0 May 5, 2017

loganfsmyth deleted the kill-shadow-functions branch May 5, 2017 20:27

xtuc mentioned this pull request May 31, 2017

babel-preset-es2015 compile class error #5797

Closed

julien-f mentioned this pull request Jun 20, 2017

super is not working inside methods marked async (T6895) #3930

Closed

This was referenced Aug 26, 2017

Fix block-scoping throwing error when handling loop not in BlockStatement #6160

Closed

Wrong scope in condition without braces with destructor #5023

Closed

lchsam mentioned this pull request Apr 4, 2019

Arrow Function bug fix nuprl/ElementaryJS#135

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Kill the "shadow-functions.js" internal plugin in favor of an explicit helper #5677

Kill the "shadow-functions.js" internal plugin in favor of an explicit helper #5677

Conversation

loganfsmyth commented Apr 28, 2017

mention-bot commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 28, 2017 • edited Loading

Codecov Report

hzoo Apr 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loganfsmyth commented May 3, 2017

mikemorancodes commented May 29, 2017

codecov bot commented Apr 28, 2017 •

edited

Loading

hzoo Apr 28, 2017 •

edited

Loading