Ensure we clone temporalAlias values when we clone ReactElements

[trueadm]

Release notes: none

When working on #2107, I noticed that we weren't cloning temporalAlias values, which is the wrong approach. We instead were re-using them on cloned ReactElements. The current approach worked fine with concrete ReactElement values, but when we introduce temporal ReactElement values that have dependencies on a shared temporalAlias, we run into very complex problems.

This PR makes it so we can clone temporalAlias values on ObjectValues (used for snapshotting). This only used for React code – so it should not affect the other parts of Prepack. I've tested this on our internal bundle and our tests and they all pass, we create more code – but this is a better approach. We can remove the bloated code at a later point, but this aims to keep our approach consistent.

[gaearon]

This looks suspicious to me. We're mutating the source here? What if the same source is used in multiple calls, or even many times in a single call? We'd lose this information for all but the last call. So it seems inconsistent.

[trueadm]

This is actually another correctness thing, that wasn't done before but should have been. See the original snapshot logic from Object.assign:

https://github.com/facebook/prepack/blob/master/src/intrinsics/ecma262/Object.js#L122

[gaearon]

Do we know it improves correctness or is it a guess? It could have been a bug in the original code too. Can you explain in more detail what specifically does doing this solve?

[trueadm]

Sure, happy to explain as it took me a while to understand it too. What I meant by a correctness change, was that I wanted the React logic to 1:1 match the existing logic of Object.assign. When we snapshot an object via getSnapshot we first check if the object already has a temporalAlias value, if so we return that value otherwise we do some work and create a temporal of that object at that point in time. This is important, because _temporalAlias is a tracked value on the object, which means that it's bindings can change between different scopes/effects.

So at a later point, if we try to call getSnapshot again and the object has already had it's temporalAlias set, then we use that value. That value may have been created in some effects with a conditional, so the value might be a conditional abstract representing two possible temporal values at different points. If we overwrote the temporal alias and removed the existing conditional, we'd potentially break things.

[gaearon]

What's the difference in the output, briefly? Can you give a few line example of before/after?

[trueadm]

@gaearon Sure, here's simple render with jsx spread 10.

Before:

  var _5 = function (props, context) {
    var _S = props;
    var _$0 = _S.key;

    var _$1 = "" + _$0;

    var _$2 = _S.ref;
    var _T = _$E;
    var _8 = {};

    var _$3 = _T(_8, props);

    var _$4 = _U(_$3, _2);

    var _$5 = _8.item1;
    var _$6 = _8.item2;
    var _$7 = _8.key;
    var _$8 = _8.ref;
    var _B = {};

    var _$9 = _T(_B, _$4);

    var _E = <_0 text={_$5} />;

    var _G = <_0 text={_$6} />;

    _B.children = [_E, _G];

    var _L = <span>{_$5}</span>;

    var _N = <span>{_$6}</span>;

    var _I = <div {..._$9}>{_L}{_N}</div>;

    return _I;
  };

After:

  var _5 = function (props, context) {
    var _S = props;
    var _$0 = _S.key;

    var _$1 = "" + _$0;

    var _$2 = _S.ref;
    var _T = _$G;
    var _8 = {};

    var _$3 = _T(_8, props);

    var _$4 = _U(_$3, _2);

    var _$5 = _8.item1;
    var _$6 = _8.item2;
    var _$7 = _8.key;
    var _$8 = _8.ref;
    var _B = {};

    var _$9 = _T(_B, _$4);

    var _E = <_0 text={_$5} />;

    var _G = <_0 text={_$6} />;

    _B.children = [_E, _G];
    var _I = {};

    var _$A = _T(_I, _$4);

    var _L = <span>{_$5}</span>;

    var _N = <span>{_$6}</span>;

    var _J = <div {..._I}>{_L}{_N}</div>;

    return _J;
  };

In the future, we should somehow detect this deadcode block, this was the original Object.assign that we cloned:

    var _B = {};

    var _$9 = _T(_B, _$4);

    var _E = <_0 text={_$5} />;

    var _G = <_0 text={_$6} />;

    _B.children = [_E, _G];

But that's a separate issue. We can actually remove most of it by adding isPure on the Object.assign in question above, but then the visitor takes about 10mins to run on our internal bundle :/

[gaearon]

but this is a better approach

Can you explain in more detail what's better about it? What problems were you running into, and why does this approach solve them?

[gaearon]

Can we just expose them as a property on the temporal so we don't have to remember to set them in a global map?

[trueadm]

We can do, but I thought this way was better because we're not introducing another property onto AbstractObjectValue.

[trueadm]

@gaearon Do you feel that we should move to using properties? I'm happy to make this change now, but I don't have any strong preference. I actually thought of doing that originally but then thought feedback would be "can't you move this into a Map, rather than adding another property". Heh.

[gaearon]

Do you expect that there will be more places where we want to reconstruct a temporal? Or is this more React-specific?

I don't care strongly either way. I think I'd like to have a property so we don't have to remember to track them. But if it's only relevant for our props objects then meh.

[trueadm]

It's only for our props. If we have other cases, I'll move it out in a follow up PR. :)

[trueadm]

@gaearon This is a better approach because we've run into many issues in the past with immutable objects (such as props and ReactElement). After spending a significant amount of time trying to get temporal entries for ReactElements, the thing that was always breaking was when a temporal ReactElement had a dependency on a props object that had a temporalAlias. Given we didn't clone the temporalAlias, and rather we shared the temporal alias, it meant that a ReactElement in its own effects but was referencing a temporal that was created in another effects, it would leave to variables being declared in one place, but not the other. This doesn't happen on master with non-lazy ReactElements because they're not temporal and always concrete and in the top scope, so having a props with a shared temporal actually works fine as the ReactElements always serialize at the end of execution – so all the temporals will already have been defined.

[gaearon]

Thanks for explaining. I think this makes sense.

[facebook-github-bot]

@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.