Nested abstract conditional fixes #2255

calebmer · 2018-07-13T17:45:37Z

Fixes #1867 and fixes #2058 which are issues blocking the React compiler.

Test case from #1867 copied below:

(function() {
  function fn(arg) {
    if (arg.foo) {
      if (arg.bar) {
        return 42;
      }
    }
  }

  if (global.__optimize) {
    __optimize(fn);
  }

  global.inspect = function() {
    return JSON.stringify([
        fn(null),
        fn({}),
        fn({foo: true}),
    ]);
  };
})();

The fix is almost entirely in joinOrForkResults where I remove the call to updatePossiblyNormalCompletionWithConditionalSimpleNormalCompletion and replace it with a new PossiblyNormalCompletion. Here’s my understanding of the issue and the previous strategy. Hopefully @hermanventer can correct me if I’m wrong or explain why it needs to be the way it was.

In our program above when we are joining the arg.foo consequent with its alternate we have the following completion for the arg.foo consequent:

PossiblyNormalCompletion (join condition is arg.bar)
- ReturnCompletion(42) (value of 42)
- SimpleNormalCompletion(empty) (there is no else)

We are joining this with an alternate of SimpleNormalCompletion since there is no else on the arg.foo if-statement. Before we would try and “sneak into” the two branches and add an abstract conditional to the result. However, the implementation was limited so we we only updated SimpleNormalCompletion. This meant we ended up changing our above completion to:

PossiblyNormalCompletion (join condition is arg.bar)
- ReturnCompletion(42)
- SimpleNormalCompletion(arg.foo ? empty : empty)

The only change being the second SimpleNormalCompletion is now arg.foo ? empty : empty. This would serialize into roughly:

function f() {
  return _$1 ? 42 : undefined;
}

Where _$1 is not declared! Since we were serializing a PossiblyNormalCompletion with a join condition of arg.bar and the arg.foo ? empty : empty abstract conditional simplified to just empty which means we never emit arg.foo so never emit arg.bar which has a dependency on arg.foo.

My strategy was to nest the PossiblyNormalCompletion in another PossiblyNormalCompletion. So the final completion for the code sample at the top of this PR is:

PossiblyNormalCompletion (join condition is arg.foo)
- consequent: PossiblyNormalCompletion (join condition is arg.bar)
  - consequent: ReturnCompletion(42)
  - alternate: SimpleNormalCompletion(empty)
- alternate: SimpleNormalCompletion(empty)

I’m confused as to why this was not done in the first place and instead an update strategy was used.

I then wrote a test (now in test/serializer/optimized-functions/NestedConditions.js) to enumerate a bunch of nested conditional cases. The rest of the fixes come from there. Notably the refactor of replacePossiblyNormalCompletionWithForkedAbruptCompletion which did not produce the same output when consequent and alternate were flipped.

hermanventer · 2018-07-13T17:50:55Z

Nice work, but it seems that we have a race condition here. See PR #2250. If you rebase on top of that, it would be easier to do a detailed review.

calebmer · 2018-07-13T17:45:59Z

src/methods/join.js

+    };
+
+    let cloneEffects = () => {
+      // TODO (hermanv) if we use e as is, it ends up being applied twice when the join of the normal


@hermanventer: is this reuse of your TODO logic ok?

I'm currently working on this code, so there will be merge conflicts for one of us.

calebmer · 2018-07-13T17:52:04Z

src/values/AbstractObjectValue.js

-            skipInvariant: true,
-            isPure: true,
-          }
+          { skipInvariant: true }


@trueadm I noticed that some property accesses were not emitted in my NestedConditions test and found your discussion in:

https://github.com/facebook/prepack/pull/2097/files/225590fe2e7c4e1dd40a0ea4b3d12d790900e609#r194060187

Getters could have a “pure” side-effect like console.log, correct? If a function call cannot be removed in pure scope, then a getter probably shouldn’t be as well?

This change is only tangentially related. I thought my serializer tests would snapshot the serialized output, but they don’t. Happy to remove if you disagree or move to another PR.

Here’s a gist with the serialized output without this “fix”:

https://gist.github.com/calebmer/3ba090cda321ce67576c8d3c73ec5670

console.log is technically a side-effectful thing right? Why were you using getters for in your test? Minifiers like google closure compiler will also strip these too, so we added the same assumptions to our wiki doc. Let's talk about this in a separate issue – I don't want to side track the work you've done in this PR :)

Thanks for the context. I thought this was a bug in my code 👍

calebmer · 2018-07-13T20:52:10Z

Rebased onto #2250. Here are the two cases this diff fixes before and after.

The first is fixed by passing effects to joinNormalCompletions.

The second is fixed by refactoring replacePossiblyNormalCompletionWithForkedAbruptCompletion so that consequent/alternate code paths are symmetric. In short, it was caused by composing effects too early in the (AbruptCompletion, PossiblyNormalCompletion) code path.

Input

function fn1(arg) {
  if (arg.foo) {
    if (arg.bar) {
      return 42;
    }
  }
}

function fn2(arg) {
  if (arg.foo) {
    return 1;
  } else {
    if (arg.bar) {
      return 2;
    }
  }
}

__optimize(fn1);
__optimize(fn2);

Bad Ouput

var _1 = function (arg) {
  return _$1 ? 42 : void 0; // <------------- `_$1` is not defined
};

var _6 = function (arg) {
  var _$2 = arg.foo;

  if (!_$2) {
    var _$3 = arg.bar;

    if (!_$3) {
      var _$3 = arg.bar; // <------------- `arg.bar` is defined twice, could also be `arg.bar()`
    }
  }

  return _$2 ? 1 : _$3 ? 2 : void 0;
};

After #2250

var _1 = function (arg) {
  var _$0 = arg.foo;
  return _$0 ? _$1 ? 42 : void 0 : void 0; // <------------- `_$1` is still not defined
};

var _8 = function (arg) {
  var _$2 = arg.foo;

  if (!_$2) {
    var _$3 = arg.bar;

    if (!_$3) {
      var _$3 = arg.bar; // <------------- `arg.bar` is defined twice, could also be `arg.bar()`
    }
  }

  return _$2 ? 1 : _$3 ? 2 : void 0;
};

After this diff

var _1 = function (arg) {
  var _$0 = arg.foo;

  if (_$0) {
    var _$1 = arg.bar;
  }

  return _$0 ? _$1 ? 42 : void 0 : void 0;
};

var _8 = function (arg) {
  var _$2 = arg.foo;

  if (!_$2) {
    var _$3 = arg.bar;
  }

  return _$2 ? 1 : _$3 ? 2 : void 0;
};

hermanventer

Looks great.

hermanventer · 2018-07-13T20:43:06Z

src/methods/join.js

+    };
+
+    let cloneEffects = () => {
+      // TODO (hermanv) if we use e as is, it ends up being applied twice when the join of the normal


I'm currently working on this code, so there will be merge conflicts for one of us.

hermanventer · 2018-07-13T20:49:57Z

src/methods/join.js

-      let ae = pnc.alternateEffects;
-      let nae = new Effects(na, ae.generator, ae.modifiedBindings, ae.modifiedProperties, ae.createdObjects);
+      invariant(pnca instanceof PossiblyNormalCompletion);
+      let [na, nae] = recurse(pnca, pnc.alternateEffects);


It took me a while to figure out that recurse captures and e and sneaks it into nae. I would make it more explicit if it were up to me.

hermanventer · 2018-07-13T20:54:08Z

src/methods/join.js

+      return [nx, nxe];
+    };
+
+    let cloneEffects = () => {


I would prefer a function, rather than a procedure with side effects.

I removed the side effects. Since you’ll be working on this code, I’ll let you move it to the appropriate scope 👍

trueadm

Looks great, just a few CI things to sort out :)

facebook-github-bot

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

facebook-github-bot

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

gaearon · 2018-07-18T13:05:21Z

Awesome work!

I'm seeing this output on master:

(function () {
  var _$2 = this;

  var _1 = function (arg) {
    var _$0 = arg.foo;

    if (_$0) {
      var _$1 = arg.bar;
    }

    if (_$0) {
      var _$1 = arg.bar;
    }

    return _$0 ? _$1 ? 42 : void 0 : void 0;
  };

  var _0 = function () {
    return JSON.stringify([_1(null), _1({}), _1({
      foo: true
    })]);
  };

  _$2.inspect = _0;
}).call(this);

Note _$1 is duplicated. Is that normal?

gaearon · 2018-07-18T14:32:48Z

Sent #2282 as a first step towards avoiding such regressions. We still need to fix them before we can merge it though.

Summary: gaearon found a regression to the cases I fixed in #2255. The following code: ```js function fn1(arg) { if (arg.foo()) { if (arg.bar()) { return 42; } } } function fn2(arg) { if (arg.foo()) { if (arg.bar()) { return 1; } } else { return 2; } } if (global.__optimize) { __optimize(fn1); __optimize(fn2); } ``` Now compiles to: ```js var fn1, fn2; (function() { var _$4 = this; var _0 = function(arg) { var _$0 = arg.foo(); if (_$0) { var _$1 = arg.bar(); } if (_$0) { var _$1 = arg.bar(); // <----------------------------- `arg.bar()` is redeclared. } return _$0 ? (_$1 ? 42 : void 0) : void 0; }; var _8 = function(arg) { var _$2 = arg.foo(); var _$3 = arg.bar(); // <------------------------------- `arg.bar()` should be inside `if (_$2)` return _$2 ? (_$3 ? 1 : void 0) : 2; }; _$4.fn1 = _0; _$4.fn2 = _8; }.call(this)); ``` After looking through the commits, I found that #2274 regressed this case. After reverting that PR I found that these cases were fixed again, but interestingly the test added in that PR also passed after I reverted the change. I’m opening this PR with the failing tests so hermanventer can take a look. I haven’t looked too deeply into this case, so I’m probably missing something important. Maybe some race condition between the landing of #2255 and #2274? Pull Request resolved: #2288 Differential Revision: D9540424 Pulled By: calebmer fbshipit-source-id: 3ef03068cb575617d64d73fdc1f9431188effdda

calebmer requested review from sebmarkbage, trueadm, hermanventer and NTillmann July 13, 2018 17:45

facebook-github-bot added the CLA Signed label Jul 13, 2018

calebmer commented Jul 13, 2018

View reviewed changes

calebmer force-pushed the nested-abstract-test-fixes branch 3 times, most recently from c5c198a to 3d7e72e Compare July 13, 2018 20:51

hermanventer approved these changes Jul 13, 2018

View reviewed changes

calebmer force-pushed the nested-abstract-test-fixes branch from 3d7e72e to bc39bb4 Compare July 13, 2018 21:17

trueadm approved these changes Jul 14, 2018

View reviewed changes

calebmer force-pushed the nested-abstract-test-fixes branch from bc39bb4 to 8fb1788 Compare July 16, 2018 19:59

facebook-github-bot reviewed Jul 16, 2018

View reviewed changes

calebmer force-pushed the nested-abstract-test-fixes branch 2 times, most recently from 70a2027 to cc0c3bc Compare July 16, 2018 21:30

nested abstract conditional fixes

d73d560

calebmer force-pushed the nested-abstract-test-fixes branch from cc0c3bc to d73d560 Compare July 17, 2018 18:35

facebook-github-bot reviewed Jul 17, 2018

View reviewed changes

facebook-github-bot closed this in b0516b4 Jul 17, 2018

gaearon mentioned this pull request Jul 18, 2018

Lint against redeclared variables #2282

Open

calebmer deleted the nested-abstract-test-fixes branch July 18, 2018 16:18

calebmer mentioned this pull request Jul 18, 2018

Add tests for nested conditional regressions #2288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested abstract conditional fixes #2255

Nested abstract conditional fixes #2255

calebmer commented Jul 13, 2018 •

edited

Loading

hermanventer commented Jul 13, 2018

calebmer Jul 13, 2018

hermanventer Jul 13, 2018

calebmer Jul 13, 2018

trueadm Jul 13, 2018

calebmer Jul 13, 2018

calebmer commented Jul 13, 2018 •

edited

Loading

hermanventer left a comment

hermanventer Jul 13, 2018

hermanventer Jul 13, 2018

calebmer Jul 13, 2018

hermanventer Jul 13, 2018

calebmer Jul 13, 2018

trueadm left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

gaearon commented Jul 18, 2018 •

edited

Loading

gaearon commented Jul 18, 2018

Nested abstract conditional fixes #2255

Nested abstract conditional fixes #2255

Conversation

calebmer commented Jul 13, 2018 • edited Loading

hermanventer commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebmer commented Jul 13, 2018 • edited Loading

Input

Bad Ouput

After #2250

After this diff

hermanventer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trueadm left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

gaearon commented Jul 18, 2018 • edited Loading

gaearon commented Jul 18, 2018

calebmer commented Jul 13, 2018 •

edited

Loading

calebmer commented Jul 13, 2018 •

edited

Loading

gaearon commented Jul 18, 2018 •

edited

Loading