Remove simple closures.

Summary:
Release notes: Removing --simpleClosures option.

This feature was never fully implemented and remained buggy.
Instead of having two distinct modes, let's rather try to improve
pointwise what we perceive as inacceptible for the regular way of
why closures work in the serializer.
Closes https://github.com/facebook/prepack/pull/1876

Differential Revision: D7889689

Pulled By: NTillmann

fbshipit-source-id: 782a6754408a9b250d45691274902c6bb2ed6924

scripts/debug-fb-www.js

@@ -75,7 +75,6 @@ let prepackOptions = {
   reactVerbose: true,
   inlineExpressions: true,
   invariantLevel: 0,
-  simpleClosures: true,
   abstractValueImpliesMax: 1000,
 };
 let inputPath = path.resolve("fb-www/input.js");

scripts/test-runner.js

@@ -365,16 +365,14 @@ function runTest(name, code, options: PrepackOptions, args) {
     }
     let copiesToFind = new Map();
     const copyMarker = "// Copies of ";
-    if (!options.simpleClosures) {
-      let searchStart = code.indexOf(copyMarker);
-      while (searchStart !== -1) {
-        let searchEnd = code.indexOf(":", searchStart);
-        let value = code.substring(searchStart + copyMarker.length, searchEnd);
-        let newline = code.indexOf("\n", searchStart);
-        let count = parseInt(code.substring(searchEnd + 1, newline), 10);
-        copiesToFind.set(new RegExp(value.replace(/[[\]]/g, "\\$&"), "gi"), count);
-        searchStart = code.indexOf(copyMarker, newline);
-      }
+    let searchStart = code.indexOf(copyMarker);
+    while (searchStart !== -1) {
+      let searchEnd = code.indexOf(":", searchStart);
+      let value = code.substring(searchStart + copyMarker.length, searchEnd);
+      let newline = code.indexOf("\n", searchStart);
+      let count = parseInt(code.substring(searchEnd + 1, newline), 10);
+      copiesToFind.set(new RegExp(value.replace(/[[\]]/g, "\\$&"), "gi"), count);
+      searchStart = code.indexOf(copyMarker, newline);
     }
     let addedCode = "";
     let injectAtRuntime = "// add at runtime:";
@@ -475,7 +473,7 @@ function runTest(name, code, options: PrepackOptions, args) {
           break;
         }
         // Test the number of clone functions generated with the inital prepack call
-        if (i === 0 && functionCloneCountMatch && !options.simpleClosures) {
+        if (i === 0 && functionCloneCountMatch) {
           let functionCount = parseInt(functionCloneCountMatch[1], 10);
           if (serialized.statistics && functionCount !== serialized.statistics.functionClones) {
             console.error(
@@ -614,7 +612,7 @@ function run(args) {
     }
     if (args.fast) flagPermutations = [[false, false, undefined, isSimpleClosureTest]];
     let lastFailed = failed;
-    for (let [delayInitializations, inlineExpressions, lazyObjectsRuntime, simpleClosures] of flagPermutations) {
+    for (let [delayInitializations, inlineExpressions, lazyObjectsRuntime] of flagPermutations) {
       if ((skipLazyObjects || args.noLazySupport) && lazyObjectsRuntime) {
         continue;
       }
@@ -623,7 +621,6 @@ function run(args) {
         delayInitializations,
         inlineExpressions,
         lazyObjectsRuntime,
-        simpleClosures,
         residual: args.residual,
       };
       if (runTest(test.name, test.file, options, args)) passed++;

src/options.js

@@ -87,7 +87,6 @@ export type SerializerOptions = {
   logModules?: boolean,
   profile?: boolean,
   inlineExpressions?: boolean,
-  simpleClosures?: boolean,
   trace?: boolean,
   heapGraphFormat?: "DotLanguage" | "VISJS",
 };

src/prepack-cli.js

@@ -72,7 +72,6 @@ function run(
     --heapGraphFilePath      The name of the output file where heap graph will be written to.
     --inlineExpressions      When generating code, tells prepack to avoid naming expressions when they are only used once,
                              and instead inline them where they are used.
-    --simpleClosures         When generating code, tells prepack to not defer initializing closures
     --invariantLevel         0: no invariants (default); 1: checks for abstract values; 2: checks for accessed built-ins; 3: internal consistency
     --invariantMode          Whether to throw an exception or call a console function to log an invariant violation; default = throw.
     --emitConcreteModel      Synthesize concrete model values for abstract models(defined by __assumeDataProperty).
@@ -108,7 +107,6 @@ function run(
     debugNames: false,
     emitConcreteModel: false,
     inlineExpressions: false,
-    simpleClosures: false,
     logStatistics: false,
     logModules: false,
     delayInitializations: false,

src/prepack-options.js

@@ -50,7 +50,6 @@ export type PrepackOptions = {|
   serialize?: boolean,
   check?: Array<number>,
   inlineExpressions?: boolean,
-  simpleClosures?: boolean,
   sourceMaps?: boolean,
   initializeMoreModules?: boolean,
   statsFile?: string,
@@ -122,7 +121,6 @@ export function getSerializerOptions({
   logModules = false,
   profile = false,
   inlineExpressions = false,
-  simpleClosures = false,
   initializeMoreModules = false,
   trace = false,
 }: PrepackOptions): SerializerOptions {
@@ -138,7 +136,6 @@ export function getSerializerOptions({
     logModules,
     profile,
     inlineExpressions,
-    simpleClosures,
     trace,
   };
   if (lazyObjectsRuntime !== undefined) {

src/serializer/Referentializer.js

@@ -192,37 +192,27 @@ export class Referentializer {
   }
 
   referentializeBinding(residualBinding: ResidualFunctionBinding, name: string, instance: FunctionInstance): void {
-    if (this._options.simpleClosures) {
-      // When simpleClosures is enabled, then space for captured mutable bindings is allocated upfront.
-      let serializedBindingId = t.identifier(this._referentializedNameGenerator.generate(name));
-      let serializedValue = residualBinding.serializedValue;
-      invariant(serializedValue);
-      let declar = t.variableDeclaration("var", [t.variableDeclarator(serializedBindingId, serializedValue)]);
-      instance.initializationStatements.push(declar);
-      residualBinding.serializedValue = serializedBindingId;
-    } else {
-      // When simpleClosures is not enabled, then space for captured mutable bindings is allocated lazily.
-      let scope = this._getSerializedBindingScopeInstance(residualBinding);
-      let capturedScope = "__captured" + scope.name;
-      // Save the serialized value for initialization at the top of
-      // the factory.
-      // This can serialize more variables than are necessary to execute
-      // the function because every function serializes every
-      // modified variable of its parent scope. In some cases it could be
-      // an improvement to split these variables into multiple
-      // scopes.
-      const variableIndexInScope = scope.initializationValues.length;
-      invariant(residualBinding.serializedValue);
-      scope.initializationValues.push(residualBinding.serializedValue);
-      scope.capturedScope = capturedScope;
+    // Space for captured mutable bindings is allocated lazily.
+    let scope = this._getSerializedBindingScopeInstance(residualBinding);
+    let capturedScope = "__captured" + scope.name;
+    // Save the serialized value for initialization at the top of
+    // the factory.
+    // This can serialize more variables than are necessary to execute
+    // the function because every function serializes every
+    // modified variable of its parent scope. In some cases it could be
+    // an improvement to split these variables into multiple
+    // scopes.
+    const variableIndexInScope = scope.initializationValues.length;
+    invariant(residualBinding.serializedValue);
+    scope.initializationValues.push(residualBinding.serializedValue);
+    scope.capturedScope = capturedScope;
 
-      // Replace binding usage with scope references
-      residualBinding.serializedValue = t.memberExpression(
-        t.identifier(capturedScope),
-        t.numericLiteral(variableIndexInScope),
-        true // Array style access.
-      );
-    }
+    // Replace binding usage with scope references
+    residualBinding.serializedValue = t.memberExpression(
+      t.identifier(capturedScope),
+      t.numericLiteral(variableIndexInScope),
+      true // Array style access.
+    );
 
     this.getStatistics().referentialized++;
   }
@@ -257,7 +247,7 @@ export class Referentializer {
         // Initialize captured scope at function call instead of globally
         if (!residualBinding.declarativeEnvironmentRecord) residualBinding.referentialized = true;
         if (!residualBinding.referentialized) {
-          if (!this._options.simpleClosures) this._getSerializedBindingScopeInstance(residualBinding);
+          this._getSerializedBindingScopeInstance(residualBinding);
           residualBinding.referentialized = true;
         }
 

src/serializer/ResidualFunctions.js

@@ -87,7 +87,6 @@ export class ResidualFunctions {
       if (!additionalFunctionValueInfos.has(instance.functionValue)) this.addFunctionInstance(instance);
     }
     this.additionalFunctionValueNestedFunctions = additionalFunctionValueNestedFunctions;
-    this.simpleClosures = !!options.simpleClosures;
   }
 
   realm: Realm;
@@ -108,7 +107,6 @@ export class ResidualFunctions {
   additionalFunctionValueInfos: Map<FunctionValue, AdditionalFunctionInfo>;
   additionalFunctionValueNestedFunctions: Set<FunctionValue>;
   referentializer: Referentializer;
-  simpleClosures: boolean;
 
   getStatistics() {
     invariant(this.realm.statistics instanceof SerializerStatistics, "serialization requires SerializerStatistics");
@@ -144,7 +142,7 @@ export class ResidualFunctions {
     let functionInfo = this.residualFunctionInfos.get(funcBody);
     invariant(functionInfo);
     let { usesArguments } = functionInfo;
-    return !shouldInlineFunction() && instances.length > 1 && !usesArguments && !this.simpleClosures;
+    return !shouldInlineFunction() && instances.length > 1 && !usesArguments;
   }
 
   // Note: this function takes linear time. Please do not call it inside loop.
@@ -629,8 +627,7 @@ export class ResidualFunctions {
             usesThis ||
             hasFunctionArg ||
             (firstUsage !== undefined && !firstUsage.isNotEarlierThan(insertionPoint)) ||
-            this.functionPrototypes.get(functionValue) !== undefined ||
-            this.simpleClosures
+            this.functionPrototypes.get(functionValue) !== undefined
           ) {
             let callArgs: Array<BabelNodeExpression | BabelNodeSpreadElement> = [t.thisExpression()];
             for (let flatArg of flatArgs) callArgs.push(flatArg);

src/serializer/ResidualHeapSerializer.js

@@ -710,11 +710,7 @@ export class ResidualHeapSerializer {
     let referencingOnlyAdditionalFunction = this.isReferencedOnlyByAdditionalFunction(val);
     if (generators.length === 0) {
       // This value is only referenced from residual functions.
-      if (
-        referencingOnlyAdditionalFunction === undefined &&
-        this._options.delayInitializations &&
-        !this._options.simpleClosures
-      ) {
+      if (referencingOnlyAdditionalFunction === undefined && this._options.delayInitializations) {
         // We can delay the initialization, and move it into a conditional code block in the residual functions!
         let body = this.residualFunctions.residualFunctionInitializers.registerValueOnlyReferencedByResidualFunctions(
           functionValues,

test/serializer/basic/SimpleClosures.js

@@ -1,9 +0,0 @@
-// does not contain:__scope
-// simple closures
-(function () {
-    let x = 0;
-    let f = function() {
-        return x++;
-    }
-    inspect = function() { return f() + f(); };
-})();

test/serializer/basic/SimpleClosures2.js

@@ -1,7 +0,0 @@
-// does not contain:__scope
-// simple closures
-(function() {
-    var obj = { x: 42 };
-    let f = function() { let ret = obj; obj = undefined; return ret; };
-    inspect = function() { return f().x + (f() === undefined); }
-})();