Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Commit

Permalink
Don’t record modified bindings for immutable bindings when havocing (#…
Browse files Browse the repository at this point in the history
…2443)

Summary:
Fixes #2442 by not marking immutable bindings as modified. Its very possible that my fix is incorrect. There’s a bunch of related logic for `binding.hasLeaked`/`binding.mutable`/`modifiedBindings` and I certainly didn’t find them all to get a holistic view of what’s happening. From the code I did look at, this fix seems to be safe since there’s no way for a havoced function to change an immutable binding.

I ran into this issue while trying to compile our internal React Native bundle with the React Compiler.

Let me walk through this repro to the invariant.

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

global.result = __evaluatePureFunction(() => {
  const b = __abstract("boolean", "true");

  if (b) {
    var g = function f(i) {
      return i === 0 ? 0 : f(i - 1) + 1;
    };
    havoc(g);
  } else {
    return;
  }

  return g(5);
});
```

1. Create a function expression that refers to itself inside a construct that wraps the evaluation in `evaluateForEffects()`. (I used an if-statement.)
2. Havoc the function expression. This will add the function’s reference to itself to `modifiedBindings`. However, the original [function expression skipped (3rd arg)](https://github.com/facebook/prepack/blob/3eb1e8e8ea91f34b3a7f492b54ca1f029fb35398/src/evaluators/FunctionExpression.js#L108) adding its binding to `modifiedBindings`.
3. Outside of `evaluateForEffects()` call the function.
4. Observe the invariant firing:

https://github.com/facebook/prepack/blob/3eb1e8e8ea91f34b3a7f492b54ca1f029fb35398/src/environment.js#L339

According to `modifiedBindings`, we only initialize the function expression _after_ its definition. So if the function expression wants to recursively refer to itself, it can’t since the effects we’ve recorded/applied puts us in a state where the binding is not available.
Pull Request resolved: #2443

Reviewed By: trueadm

Differential Revision: D9447861

Pulled By: calebmer

fbshipit-source-id: de4460bd184012416e80eb65f86dae7ade9dc34e
  • Loading branch information
calebmer authored and facebook-github-bot committed Aug 22, 2018
1 parent fcdb6eb commit 4027b16
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ export function materializeBinding(realm: Realm, binding: Binding): void {
export function leakBinding(binding: Binding): void {
let realm = binding.environment.realm;
if (!binding.hasLeaked) {
realm.recordModifiedBinding(binding).hasLeaked = true;
if (binding.mutable) {
realm.recordModifiedBinding(binding).hasLeaked = true;
} else {
binding.hasLeaked = true;
}
materializeBinding(realm, binding);
}

Expand Down
20 changes: 20 additions & 0 deletions test/serializer/pure-functions/HavocBindingImmutable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
if (!global.__evaluatePureFunction) global.__evaluatePureFunction = f => f();

const havoc = global.__abstract ? global.__abstract("function", "(() => {})") : () => {};

const result = global.__evaluatePureFunction(() => {
const b = global.__abstract ? global.__abstract("boolean", "true") : true;

if (b) {
var g = function f(i) {
return i === 0 ? 0 : f(i - 1) + 1;
};
havoc(g);
} else {
return;
}

return g(5);
});

global.inspect = () => result;

0 comments on commit 4027b16

Please sign in to comment.