fix builtins plugins from leaking vars

packages/babel-plugin-minify-builtins/__tests__/__snapshots__/minify-builtins.js.snap

@@ -1,24 +1,72 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`minify-builtins should collect and minify in segments if there is no common ancestor 1`] = `
+Object {
+  "_source": "function a(){
+  function d(){
+    Math.floor(as, bb);
+  }
+}
+function b(){
+  Math.floor(as, bb);
+  function d(){
+    Math.floor(as, bb);
+  }
+}
+function c(){
+  Math.floor(as, bb);
+  function d(){
+    Math.floor(as, bb);
+  }
+}",
+  "expected": "function a() {
+  function d() {
+    Math.floor(as, bb);
+  }
+}
+function b() {
+  var _Mathfloor = Math.floor;
+
+  _Mathfloor(as, bb);
+  function d() {
+    _Mathfloor(as, bb);
+  }
+}
+function c() {
+  var _Mathfloor2 = Math.floor;
+
+  _Mathfloor2(as, bb);
+  function d() {
+    _Mathfloor2(as, bb);
+  }
+}",
+}
+`;
+
 exports[`minify-builtins should collect and minify no matter any depth 1`] = `
 Object {
-  "_source": "Math.max(a, b) + Math.max(a, b);
-function a (){
+  "_source": "function a (){
   Math.max(b, a);
-  return function b() {
+  function b() {
     const a = Math.floor(c);
     Math.min(b, a) * Math.floor(b);
+    function c() {
+      Math.floor(c) + Math.min(b, a)
+    }
   }
 }",
-  "expected": "var _Mathfloor = Math.floor;
-var _Mathmax = Math.max;
-_Mathmax(a, b) + _Mathmax(a, b);
-function a() {
-  _Mathmax(b, a);
-  return function b() {
+  "expected": "function a() {
+  Math.max(b, a);
+  function b() {
+    var _Mathmin = Math.min;
+    var _Mathfloor = Math.floor;
+
     const a = _Mathfloor(c);
-    Math.min(b, a) * _Mathfloor(b);
-  };
+    _Mathmin(b, a) * _Mathfloor(b);
+    function c() {
+      _Mathfloor(c) + _Mathmin(b, a);
+    }
+  }
 }",
 }
 `;
@@ -38,32 +86,34 @@ foo(x);",
 
 exports[`minify-builtins should minify standard built in methods 1`] = `
 Object {
-  "_source": "Math.max(a, b) + Math.max(b, a);
-function c() {
+  "_source": "function c() {
   let a = 10;
   const d = Number.isNaN(a);
+  Math.max(a, b) + Math.max(b, a);
   return d && Number.isFinite(a);
 }",
-  "expected": "var _Mathmax = Math.max;
-_Mathmax(a, b) + _Mathmax(b, a);
-function c() {
+  "expected": "function c() {
+  var _Mathmax = Math.max;
+
   let a = 10;
   const d = false;
+  _Mathmax(a, b) + _Mathmax(b, a);
   return d && true;
 }",
 }
 `;
 
 exports[`minify-builtins should minify standard built in properties 1`] = `
 Object {
-  "_source": "Number.NAN + Number.NAN;
-function a () {
+  "_source": "function a () {
+  Number.NAN + Number.NAN;
   return Math.PI + Math.PI + Number.EPSILON + Number.NAN;
 }",
-  "expected": "var _MathPI = Math.PI;
-var _NumberNAN = Number.NAN;
-_NumberNAN + _NumberNAN;
-function a() {
+  "expected": "function a() {
+  var _MathPI = Math.PI;
+  var _NumberNAN = Number.NAN;
+
+  _NumberNAN + _NumberNAN;
   return _MathPI + _MathPI + Number.EPSILON + _NumberNAN;
 }",
 }
@@ -90,13 +140,29 @@ Math[max](1.5);",
 exports[`minify-builtins should take no of occurences in to account 1`] = `
 Object {
   "_source": "function a() {
-  return Math.floor(a) + Math.floor(b) + Math.min(a, b);
+  Math.floor(a) + Math.floor(b) + Math.min(a, b);
+}",
+  "expected": "function a() {
+  var _Mathfloor = Math.floor;
+
+  _Mathfloor(a) + _Mathfloor(b) + Math.min(a, b);
+}",
 }
-Math.floor(a) + Math.max(a, b);",
-  "expected": "var _Mathfloor = Math.floor;
+`;
+
+exports[`minify-builtins shouldn't minify builtins in the program scope to avoid leaking 1`] = `
+Object {
+  "_source": "Math.max(c, d)
+function a (){
+  Math.max(b, a) + Math.max(c, d);
+}
+Math.max(e, f)",
+  "expected": "Math.max(c, d);
 function a() {
-  return _Mathfloor(a) + _Mathfloor(b) + Math.min(a, b);
+  var _Mathmax = Math.max;
+
+  _Mathmax(b, a) + _Mathmax(c, d);
 }
-_Mathfloor(a) + Math.max(a, b);",
+Math.max(e, f);",
 }
 `;

packages/babel-plugin-minify-builtins/__tests__/minify-builtins.js

@@ -14,10 +14,10 @@ describe("minify-builtins", () => {
   it("should minify standard built in methods", () => {
     const source = unpad(
       `
-      Math.max(a, b) + Math.max(b, a);
       function c() {
         let a = 10;
         const d = Number.isNaN(a);
+        Math.max(a, b) + Math.max(b, a);
         return d && Number.isFinite(a);
       }
     `
@@ -29,8 +29,8 @@ describe("minify-builtins", () => {
   it("should minify standard built in properties", () => {
     const source = unpad(
       `
-      Number.NAN + Number.NAN;
       function a () {
+        Number.NAN + Number.NAN;
         return Math.PI + Math.PI + Number.EPSILON + Number.NAN;
       }
     `
@@ -42,9 +42,8 @@ describe("minify-builtins", () => {
     const source = unpad(
       `
       function a() {
-        return Math.floor(a) + Math.floor(b) + Math.min(a, b);
+        Math.floor(a) + Math.floor(b) + Math.min(a, b);
       }
-      Math.floor(a) + Math.max(a, b);
     `
     );
     expect({ _source: source, expected: transform(source) }).toMatchSnapshot();
@@ -53,12 +52,52 @@ describe("minify-builtins", () => {
   it("should collect and minify no matter any depth", () => {
     const source = unpad(
       `
-      Math.max(a, b) + Math.max(a, b);
       function a (){
         Math.max(b, a);
-        return function b() {
+        function b() {
           const a = Math.floor(c);
           Math.min(b, a) * Math.floor(b);
+          function c() {
+            Math.floor(c) + Math.min(b, a)
+          }
+        }
+      }
+    `
+    );
+    expect({ _source: source, expected: transform(source) }).toMatchSnapshot();
+  });
+
+  it("shouldn't minify builtins in the program scope to avoid leaking", () => {
+    const source = unpad(
+      `
+      Math.max(c, d)
+      function a (){
+        Math.max(b, a) + Math.max(c, d);
+      }
+      Math.max(e, f)
+    `
+    );
+    expect({ _source: source, expected: transform(source) }).toMatchSnapshot();
+  });
+
+  it("should collect and minify in segments if there is no common ancestor", () => {
+    const source = unpad(
+      `
+      function a(){
+        function d(){
+          Math.floor(as, bb);
+        }
+      }
+      function b(){
+        Math.floor(as, bb);
+        function d(){
+          Math.floor(as, bb);
+        }
+      }
+      function c(){
+        Math.floor(as, bb);
+        function d(){
+          Math.floor(as, bb);
         }
       }
     `

packages/babel-plugin-minify-builtins/src/index.js

@@ -10,6 +10,7 @@ module.exports = function({ types: t }) {
   class BuiltInReplacer {
     constructor(program) {
       this.program = program;
+      // map<expr_name, path[]>;
       this.pathsToUpdate = new Map();
     }
 
@@ -27,13 +28,13 @@ module.exports = function({ types: t }) {
             return;
           }
 
-          if (!isComputed(path) && isBuiltin(path)) {
+          if (
+            !isComputed(path) &&
+            isBuiltin(path) &&
+            !path.getFunctionParent().isProgram()
+          ) {
             const expName = memberToString(path.node);
-
-            if (!context.pathsToUpdate.has(expName)) {
-              context.pathsToUpdate.set(expName, []);
-            }
-            context.pathsToUpdate.get(expName).push(path);
+            addToMap(context.pathsToUpdate, expName, path);
           }
         },
 
@@ -53,13 +54,9 @@ module.exports = function({ types: t }) {
               // Math.floor(1) --> 1
               if (result.confident && hasPureArgs(path)) {
                 path.replaceWith(t.valueToNode(result.value));
-              } else {
+              } else if (!callee.getFunctionParent().isProgram()) {
                 const expName = memberToString(callee.node);
-
-                if (!context.pathsToUpdate.has(expName)) {
-                  context.pathsToUpdate.set(expName, []);
-                }
-                context.pathsToUpdate.get(expName).push(callee);
+                addToMap(context.pathsToUpdate, expName, callee);
               }
             }
           }
@@ -71,20 +68,28 @@ module.exports = function({ types: t }) {
 
     replace() {
       for (const [expName, paths] of this.pathsToUpdate) {
-        // Should only transform if there is more than 1 occurence
-        if (paths.length > 1) {
+        // transform only if there is more than 1 occurence
+        if (paths.length <= 1) {
+          continue;
+        }
+
+        const segmentsMap = getSegmentedSubPaths(paths);
+        for (const [parent, subpaths] of segmentsMap) {
+          if (subpaths.length <= 1) {
+            continue;
+          }
           const uniqueIdentifier = this.program.scope.generateUidIdentifier(
             expName
           );
           const newNode = t.variableDeclaration("var", [
-            t.variableDeclarator(uniqueIdentifier, paths[0].node)
+            t.variableDeclarator(uniqueIdentifier, subpaths[0].node)
           ]);
 
-          for (const path of paths) {
+          for (const path of subpaths) {
             path.replaceWith(uniqueIdentifier);
           }
-          // hoist the created var to top of the program
-          this.program.unshiftContainer("body", newNode);
+          // hoist the created var to the top of the function scope
+          parent.get("body").unshiftContainer("body", newNode);
         }
       }
     }
@@ -126,6 +131,43 @@ module.exports = function({ types: t }) {
   }
 };
 
+function addToMap(map, key, value) {
+  if (!map.has(key)) {
+    map.set(key, []);
+  }
+  map.get(key).push(value);
+}
+
+// Creates a segmented map that contains the earliest common Ancestor
+// as the key and array of subpaths that are descendats of the LCA as value
+function getSegmentedSubPaths(paths) {
+  let segments = new Map();
+
+  // Get earliest Path in tree where paths intersect
+  paths[
+    0
+  ].getDeepestCommonAncestorFrom(paths, (lastCommon, index, ancestries) => {
+    // we found the LCA
+    if (!lastCommon.isProgram()) {
+      lastCommon = !lastCommon.isFunction()
+        ? lastCommon.getFunctionParent()
+        : lastCommon;
+      segments.set(lastCommon, paths);
+      return;
+    }
+    // Deopt and construct segments otherwise
+    for (const ancestor of ancestries) {
+      const parentPath = ancestor[index + 1];
+      const validDescendants = paths.filter(p => {
+        return p.isDescendant(parentPath);
+      });
+      segments.set(parentPath, validDescendants);
+    }
+  });
+
+  return segments;
+}
+
 function hasPureArgs(path) {
   const args = path.get("arguments");
   for (const arg of args) {