Incorrect closure serialization when object referenced through different functions. (#2497)

2019-02-28 11:11:43 -08:00 · 2019-02-28 11:11:43 -08:00 · 3d1df982de
parent aa3c6d0ba6
commit 3d1df982de
3 changed files with 366 additions and 34 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -7,6 +7,8 @@
  to help with diagnosing cryptic problems that can occur when Outputs are accidentally
  concatenated into a string in some part of the program.

+- Fixes incorrect closure serialization issue (https://github.com/pulumi/pulumi/pull/2497)
+
 ## 0.16.17 (Released February 27th, 2019)

 ### Improvements
--- a/sdk/nodejs/runtime/closure/createClosure.ts
+++ b/sdk/nodejs/runtime/closure/createClosure.ts
@ -933,40 +933,47 @@ async function getOrCreateEntryAsync(
    // Returns 'true' if the caller (serializeObjectAsync) should call this again, but without any
    // property filtering.
    async function serializeObjectWorkerAsync(localCapturedPropertyChains: CapturedPropertyChain[]): Promise<boolean> {
-        const objectInfo: ObjectInfo = entry.object || { env: new Map() };
-        entry.object = objectInfo;
-        const environment = entry.object.env;
+        entry.object = entry.object || { env: new Map() };

        if (localCapturedPropertyChains.length === 0) {
-            await serializeAllObjectPropertiesAsync(environment);
+            await serializeAllObjectPropertiesAsync(entry.object);
            return false;
        } else {
-            return await serializeSomeObjectPropertiesAsync(environment, localCapturedPropertyChains);
+            return await serializeSomeObjectPropertiesAsync(entry.object, localCapturedPropertyChains);
        }
    }

    // Serializes out all the properties of this object.  Used when we can't prove that
    // only a subset of properties are used on this object.
-    async function serializeAllObjectPropertiesAsync(environment: PropertyMap) {
+    async function serializeAllObjectPropertiesAsync(object: ObjectInfo) {
        // we wanted to capture everything (including the prototype chain)
        const descriptors = await getOwnPropertyDescriptors(obj);

        for (const descriptor of descriptors) {
-            // we're about to recurse inside this object.  In order to prever infinite
-            // loops, put a dummy entry in the environment map.  That way, if we hit
-            // this object again while recursing we won't try to generate this property.
            const keyEntry = await getOrCreateEntryAsync(getNameOrSymbol(descriptor), undefined, context, serialize, logInfo);
-            if (!environment.has(keyEntry)) {
-                environment.set(keyEntry, <any>undefined);

-                const propertyInfo = await createPropertyInfoAsync(descriptor);
-                const prop = await getOwnPropertyAsync(obj, descriptor);
-                const valEntry = await getOrCreateEntryAsync(
-                    prop, undefined, context, serialize, logInfo);
-
-                // Now, replace the dummy entry with the actual one we want.
-                environment.set(keyEntry, { info: propertyInfo, entry: valEntry });
+            // We're about to recurse inside this object.  In order to prevent infinite loops, put a
+            // dummy entry in the environment map.  That way, if we hit this object again while
+            // recursing we won't try to generate this property.
+            //
+            // Note: we only stop recursing if we hit exactly our sentinel key (i.e. we're self
+            // recursive).  We *do* want to recurse through the object again if we see it through
+            // non-recursive paths.  That's because we might be hitting this object through one
+            // prop-name-path, but we created it the first time through another prop-name path.
+            //
+            // By processing the object again, we will add the different members we need.
+            if (object.env.has(keyEntry) && object.env.get(keyEntry) === undefined) {
+                continue;
            }
+            object.env.set(keyEntry, <any>undefined);
+
+            const propertyInfo = await createPropertyInfoAsync(descriptor);
+            const prop = await getOwnPropertyAsync(obj, descriptor);
+            const valEntry = await getOrCreateEntryAsync(
+                prop, undefined, context, serialize, logInfo);
+
+            // Now, replace the dummy entry with the actual one we want.
+            object.env.set(keyEntry, { info: propertyInfo, entry: valEntry });
        }

        // If the object's __proto__ is not Object.prototype, then we have to capture what it
@ -974,10 +981,10 @@ async function getOrCreateEntryAsync(
        // things up properly.
        //
        // We don't need to capture the prototype if the user is not capturing 'this' either.
-        if (!entry.object!.proto) {
+        if (!object.proto) {
            const proto = Object.getPrototypeOf(obj);
            if (proto !== Object.prototype) {
-                entry.object!.proto = await getOrCreateEntryAsync(
+                object.proto = await getOrCreateEntryAsync(
                    proto, undefined, context, serialize, logInfo);
            }
        }
@ -986,7 +993,7 @@ async function getOrCreateEntryAsync(
    // Serializes out only the subset of properties of this object that we have seen used
    // and have recorded in localCapturedPropertyChains
    async function serializeSomeObjectPropertiesAsync(
-            environment: PropertyMap, localCapturedPropertyChains: CapturedPropertyChain[]): Promise<boolean> {
+            object: ObjectInfo, localCapturedPropertyChains: CapturedPropertyChain[]): Promise<boolean> {

        // validate our invariants.
        for (const chain of localCapturedPropertyChains) {
@ -1017,16 +1024,22 @@ async function getOrCreateEntryAsync(
            // Now, make an entry just for this name.
            const keyEntry = await getOrCreateNameEntryAsync(propName, undefined, context, serialize, logInfo);

-            if (environment.has(keyEntry)) {
+            // We're about to recurse inside this object.  In order to prevent infinite loops, put a
+            // dummy entry in the environment map.  That way, if we hit this object again while
+            // recursing we won't try to generate this property.
+            //
+            // Note: we only stop recursing if we hit exactly our sentinel key (i.e. we're self
+            // recursive).  We *do* want to recurse through the object again if we see it through
+            // non-recursive paths.  That's because we might be hitting this object through one
+            // prop-name-path, but we created it the first time through another prop-name path.
+            //
+            // By processing the object again, we will add the different members we need.
+            if (object.env.has(keyEntry) && object.env.get(keyEntry) === undefined) {
                continue;
            }
+            object.env.set(keyEntry, <any>undefined);

-            // we're about to recurse inside this object.  In order to prevent infinite
-            // loops, put a dummy entry in the environment map.  That way, if we hit
-            // this object again while recursing we won't try to generate this property.
-            environment.set(keyEntry, <any>undefined);
            const objPropValue = await getPropertyAsync(obj, propName);
-
            const propertyInfo = await getPropertyInfoAsync(obj, propName);
            if (!propertyInfo) {
                if (objPropValue !== undefined) {
@ -1036,7 +1049,7 @@ async function getOrCreateEntryAsync(
                // User code referenced a property not actually on the object at all.
                // So to properly represent that, we don't place any information about
                // this property on the object.
-                environment.delete(keyEntry);
+                object.env.delete(keyEntry);
            } else {
                // Determine what chained property names we're accessing off of this sub-property.
                // if we have no sub property name chain, then indicate that with an empty array
@ -1061,14 +1074,14 @@ async function getOrCreateEntryAsync(
                    // the referenced function captured 'this'.  Have to serialize out
                    // this entire object.  Undo the work we did to just serialize out a
                    // few properties.
-                    environment.clear();
+                    object.env.clear();

                    // Signal our caller to serialize the entire object.
                    return true;
                }

                // Now, replace the dummy entry with the actual one we want.
-                environment.set(keyEntry, { info: propertyInfo, entry: valEntry });
+                object.env.set(keyEntry, { info: propertyInfo, entry: valEntry });
            }
        }

--- a/sdk/nodejs/tests/runtime/tsClosureCases.ts
+++ b/sdk/nodejs/tests/runtime/tsClosureCases.ts
@ -5017,6 +5017,322 @@ return function () { console.log(o1); console.log(o2.b.d); console.log(o3.b.d);
        });
    }

+    {
+        const defaultsForThing = { config: { x: "x", y: "y" } };
+        function getX() { return defaultsForThing.config.x }
+        function getAll() { const x = getX(); return { x, y: defaultsForThing.config.y } }
+
+        cases.push({
+            title: "Analyze property chain #22",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __defaultsForThing = {};
+var __defaultsForThing_config = {x: "x", y: "y"};
+__defaultsForThing.config = __defaultsForThing_config;
+
+function __getX() {
+  return (function() {
+    with({ defaultsForThing: __defaultsForThing, getX: __getX }) {
+
+return function /*getX*/() { return defaultsForThing.config.x; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __getAll() {
+  return (function() {
+    with({ getX: __getX, defaultsForThing: __defaultsForThing, getAll: __getAll }) {
+
+return function /*getAll*/() { const x = getX(); return { x, y: defaultsForThing.config.y }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
+    {
+        const defaultsForThing = { config: { x: "x", y: "y" } };
+        function getAll() { return { y: defaultsForThing.config.y } }
+
+        cases.push({
+            title: "Analyze property chain #23",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __defaultsForThing = {};
+var __defaultsForThing_config = {y: "y"};
+__defaultsForThing.config = __defaultsForThing_config;
+
+function __getAll() {
+  return (function() {
+    with({ defaultsForThing: __defaultsForThing, getAll: __getAll }) {
+
+return function /*getAll*/() { return { y: defaultsForThing.config.y }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
+    {
+        const config = { x: "x", y: "y" };
+        function getX() { return config.x }
+        function getAll() { const x = getX(); return { x, y: config.y } }
+
+        cases.push({
+            title: "Analyze property chain #24",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __config = {x: "x", y: "y"};
+
+function __getX() {
+  return (function() {
+    with({ config: __config, getX: __getX }) {
+
+return function /*getX*/() { return config.x; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __getAll() {
+  return (function() {
+    with({ getX: __getX, config: __config, getAll: __getAll }) {
+
+return function /*getAll*/() { const x = getX(); return { x, y: config.y }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
+    {
+        const defaultsForThing = { config: { x: "x", y: "y" } };
+        function getX() { return defaultsForThing }
+        function getAll() { const x = getX(); return { y: defaultsForThing.config.y } }
+
+        cases.push({
+            title: "Analyze property chain #25",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __defaultsForThing = {};
+var __defaultsForThing_config = {x: "x", y: "y"};
+__defaultsForThing.config = __defaultsForThing_config;
+
+function __getX() {
+  return (function() {
+    with({ defaultsForThing: __defaultsForThing, getX: __getX }) {
+
+return function /*getX*/() { return defaultsForThing; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __getAll() {
+  return (function() {
+    with({ getX: __getX, defaultsForThing: __defaultsForThing, getAll: __getAll }) {
+
+return function /*getAll*/() { const x = getX(); return { y: defaultsForThing.config.y }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
+    {
+        const defaultsForThing = { config: { x: "x", y: "y" } };
+        function getX() { return defaultsForThing.config }
+        function getAll() { const x = getX(); return { y: defaultsForThing.config.y } }
+
+        cases.push({
+            title: "Analyze property chain #26",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __defaultsForThing = {};
+var __defaultsForThing_config = {x: "x", y: "y"};
+__defaultsForThing.config = __defaultsForThing_config;
+
+function __getX() {
+  return (function() {
+    with({ defaultsForThing: __defaultsForThing, getX: __getX }) {
+
+return function /*getX*/() { return defaultsForThing.config; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __getAll() {
+  return (function() {
+    with({ getX: __getX, defaultsForThing: __defaultsForThing, getAll: __getAll }) {
+
+return function /*getAll*/() { const x = getX(); return { y: defaultsForThing.config.y }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
+    {
+        const defaultsForThing = { config: { x: "x", y: "y" } };
+        function getX() { return defaultsForThing.config.x }
+        function getAll() { const x = getX(); return { y: defaultsForThing } }
+
+        cases.push({
+            title: "Analyze property chain #27",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __defaultsForThing = {};
+var __defaultsForThing_config = {x: "x", y: "y"};
+__defaultsForThing.config = __defaultsForThing_config;
+
+function __getX() {
+  return (function() {
+    with({ defaultsForThing: __defaultsForThing, getX: __getX }) {
+
+return function /*getX*/() { return defaultsForThing.config.x; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __getAll() {
+  return (function() {
+    with({ getX: __getX, defaultsForThing: __defaultsForThing, getAll: __getAll }) {
+
+return function /*getAll*/() { const x = getX(); return { y: defaultsForThing }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
+    {
+        const defaultsForThing = { config: { x: "x", y: "y" } };
+        function getX() { return defaultsForThing.config.x }
+        function getAll() { const x = getX(); return { y: defaultsForThing.config } }
+
+        cases.push({
+            title: "Analyze property chain #28",
+            func: function () { console.log(getAll()); },
+            expectText: `exports.handler = __f0;
+
+var __defaultsForThing = {};
+var __defaultsForThing_config = {x: "x", y: "y"};
+__defaultsForThing.config = __defaultsForThing_config;
+
+function __getX() {
+  return (function() {
+    with({ defaultsForThing: __defaultsForThing, getX: __getX }) {
+
+return function /*getX*/() { return defaultsForThing.config.x; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __getAll() {
+  return (function() {
+    with({ getX: __getX, defaultsForThing: __defaultsForThing, getAll: __getAll }) {
+
+return function /*getAll*/() { const x = getX(); return { y: defaultsForThing.config }; };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+
+function __f0() {
+  return (function() {
+    with({ getAll: __getAll }) {
+
+return function () { console.log(getAll()); };
+
+    }
+  }).apply(undefined, undefined).apply(this, arguments);
+}
+`,
+        });
+    }
+
    {
        cases.push({
            title: "Capture non-built-in module",
@ -5143,9 +5459,9 @@ return function () { const v = testConfig.get("TestingKey1"); console.log(v); };
 }
 `,
       });
-   }
+    }

-   {
+    {
        deploymentOnlyModule.setConfig("test:TestingKey2", "TestingValue2");

        cases.push({
@ -5228,7 +5544,7 @@ return function () { const v = new deploymentOnlyModule.Config("test").get("Test
 }
 `,
       });
-   }
+    }

    {
        cases.push({
@ -5485,7 +5801,8 @@ return function () { console.log(regex); foo(); };
            return;
        }

-        // if (test.title !== "Fail to capture non-deployment module due to native code") {
+        // if (test.title.indexOf("Analyze property chain #2") < 0) {
+        // //if (test.title !== "Analyze property chain #23") {
        //     continue;
        // }