Fix havoced binding not in optimized function

[calebmer]

Fixes #2419 and #2386. I ran into this issue while testing an internal React Native bundle. This invariant assumes that all property bindings emitted in a pure scope are also in an optimized function. This is not true for __evaluatePureFunction which the React Compiler wraps around initialization code so that globals outside of the closure are not havoced.

[cblappert]

This code deals with bindings (e.g. var/let declarations) not properties. I am a little dubious that everything is scoped correctly because I think visitBindingAssignment assumes it's inside of an optimized function.

[cblappert]

Won't this only be true if we're inside an optimized function? Also not sure if _enqueueWithUnrelatedScope does something reasonable when passed undefined.

[calebmer]

Won't this only be true if we're inside an optimized function?

Nope.

In my example this will be true for non-local references in a havoced function. We havoc functions in pure scope. __evaluatePureFunction creates a pure scope without needing __optimize. Consider:

__evaluatePureFunction(() => {
  const x = {};

  var havoc = __abstract("function", "(() => {})");
  havoc(() => x);
});

No __optimize, but residualBinding.potentialReferentializationScopes.size === 0 is true.

Also not sure if _enqueueWithUnrelatedScope does something reasonable when passed undefined.

I think you misunderstood the change? _enqueueWithUnrelatedScope is not passed undefined. I moved this invariant to the if condition. So the invariant must still be true for _enqueueWithUnrelatedScope to run.

[cblappert]

So looking back at why I added those bits of code, they're only an issue for optimized functions (it prevents a case where something is only reachable from an optimized function and we didn't end up visiting it). We should be fine if it has to do with global/residual code.

I guess what I was trying to say is that I'm surprised that we use visitBindingAssignment outside of optimized function cases (I wasn't aware of evaluatePureFunction) and that it wasn't designed for that, and that I wasn't sure that it would do the right thing.

[calebmer]

visitBindingAssignment assumes it's inside of an optimized function.

Which is an incorrect assumption as my test shows since we have __evaluatePureFunction. Should we still call this.visitBinding but with a different scope? What’s a test that will fail if we don’t? I can’t come up with one.

Re-requesting review since I don’t understand the change you’re looking for.

Re-review after comments?

[NTillmann]

Does this fix #2386 as well?

[calebmer]

@NTillmann thanks for the test case! It does fix it. Although I had to update the PR to fully fix the issue. I had fixed the invariant, but that case output:

let result;
(function () {
  var _$4 = this;

  var _3 = function () {
    var __leaked_1; // <------------------ __leaked_1 declared

    var _$2 = __leaked_1;

    var _$3 = _$2 + 42;

    __leaked_1 = _$3;
    return void 0;
  };

  __leaked_1 = 23; // <------------------- __leaked_1 used outside of function!

  var _$0 = (() => {})(_3);

  var _$1 = __leaked_1; // <-------------- __leaked_1 used again outside of function!

  var _6 = () => {
    return result;
  };

  _$4.inspect = _6;
  result = _$1;
}).call(this);

This issue was because we weren’t visiting the binding in the global scope we wouldn’t referentialize it there. So I updated visitBinding to take a Scope instead of a FunctionValue so we could visit the binding in pure scope instead of just in an optimized function.

I updated the PR so now your case in #2386 is fixed.

@NTillmann and @cblappert, you should probably give this PR another review with the updates.

[NTillmann]

This if statement was already there before your change, but I am not sure it's correct. What if you have an object that gets assigned to in both an optimized function and from a pure scope in global code?

[calebmer]

I don’t know enough about this to know whether or not it is correct. I tried producing a failing test case based on your comment but didn’t have success. I’ll leave to @cblappert who has better context on this code and this condition.

The defense for this condition based on my understanding of the code is that we only need to execute the code under this condition once per binding in the “largest” possible scope. Perhaps accidentally if this is in a pure scope it will be visited first since optimized functions are only evaluated at the very end of global scope? Under my understanding, if __optimize evaluated the function in place (instead of at the end of global scope) the following example would referentialize i in f.

const havoc = __abstract("function", "(() => {})");

global.result = __evaluatePureFunction(() => {
  const f = () => i++;
  __optimize(f);

  let i = 0;

  havoc(f);

  return f;
});

Although I might completely misunderstand 🙂

[NTillmann]

Looks good, except that per my comment I suspect that there are more bugs hiding here.

[facebook-github-bot]

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