Incorrect closure serialization when object referenced through different functions. #2497
Conversation
CyrusNajmabadi
changed the title
| @@ -4801,7 +4801,7 @@ return function () { console.log(o.b()); console.log(o.b.name); }; | |||
| expectText: `exports.handler = __f0; | |||
|
|
|||
| var __o2 = {}; | |||
| var __o2_b = {d: 3, c: 2}; | |||
| var __o2_b = {c: 2, d: 3}; | |||
There was a problem hiding this comment.
note: this is equivalent to before. t he change in behavior is due to how we now clear and re-add. because of that, we restore this to the same order as the original object (i.e. 'c' then 'd', instead of 'd' then 'c').
|
@swgillespie @lukehoban PTAL. Thanks! |
| const objectInfo: ObjectInfo = entry.object || { env: new Map() }; | ||
| entry.object = objectInfo; | ||
| const environment = entry.object.env; | ||
| entry.object = entry.object || { env: new Map() }; |
There was a problem hiding this comment.
this is just simplification.
| return false; | ||
| } else { | ||
| return await serializeSomeObjectPropertiesAsync(environment, localCapturedPropertyChains); | ||
| return await serializeSomeObjectPropertiesAsync(entry.object, localCapturedPropertyChains); |
There was a problem hiding this comment.
i'm passing the higher up object to prevent unnecessary null checks below.
| } | ||
| object.env.set(keyEntry, <any>undefined); |
There was a problem hiding this comment.
this is the core change to what we had before. previously, we said "ah, we've marker this prop, don't go into it again". This was done both to prevent infinite recursion, and to avoid serializing a prop we thought we'd already serialized.
Now, all we do is set the marking to prevent infinite recursion. if we see the prop a second time otherwise, then we still re-serialize it. This way, if we have a differnet set of sub-props to serialize that will ensure that that happens.
| const proto = Object.getPrototypeOf(obj); | ||
| if (proto !== Object.prototype) { | ||
| entry.object!.proto = await getOrCreateEntryAsync( | ||
| object.proto = await getOrCreateEntryAsync( |
There was a problem hiding this comment.
i could avoid the null checks here because entry.proto is the value passed in, and is known to be non-null.
| continue; | ||
| } | ||
| object.env.set(keyEntry, <any>undefined); |
There was a problem hiding this comment.
same fix as above. we prevent infinite recursion. but we don't stop re-examination of props we've already seen.
| } | ||
| `, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Added several different permutations of visiting different property chains (of differing lengths), from different contexts. Def found several issues. But all these cases now work properly.
Fixes #2429
Shoudl review this as: https://github.com/pulumi/pulumi/pull/2497/files?w=1