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

Clarify / fix nested generators #1829

Closed
NTillmann opened this issue May 1, 2018 · 1 comment
Closed

Clarify / fix nested generators #1829

NTillmann opened this issue May 1, 2018 · 1 comment

Comments

@NTillmann
Copy link
Contributor

Currently, the front end may produces generator trees that are actually DAGs --- the same generator may appear multiple times.

The back end was designed assuming that generators form a tree, not a DAG. Note this currently disabled invariant:
https://github.com/facebook/prepack/blob/c3a1fd3c46a5181dc1288f121b576cc4001d002f/src/serializer/ResidualHeapVisitor.js#L1055

With the invariant disabled, the visitor and serializer will simply visit each multi-referenced generator multiple times. I think this might just work fine, as long as such a generator doesn't declare derived abstract values - if it does, I suspect that it will confuse the waiting logic in the emitter.

The way things are right now, the generated code is suboptimal, and looking at it, it's not clear to me if the front-end produces this by design or by error.

Consider the following program, a variation of test/serializer/abstract/Throw6.js:

(function() {
    let x = global.__abstract ? __abstract("boolean", "true") : true;
    function foo(b) {
        if (b) throw new Error("" + Date.now());
        return "is false";
    }
    let z = foo(!x);
    inspect = function() { return z; }
})();

It currently "works", prepacking to the following code:

(function () {
  var $$0 = { enumerable: false, configurable: true, writable: true };
  var _$1 = this;
  var _$2 = _$1.Error;
  var _$3 = _$2.prototype;
  var _$4 = _$1.Object;
  var _$5 = _$4.defineProperty;
  var _8 = function () { return "is false"; };
  var __constructor = function () {};
  var _1 = true;
  var _0 = !_1;
  if (_0) {
    var _$0 = _$1.Date.now();
  }
  if (_0) {
    var _$0 = _$1.Date.now();
  } else {
    inspect = _8;
  }
  var _7 = _$3;
  var _2 = (__constructor.prototype = _7, new __constructor());
  $$0.value = "Error\n    at foo (unknown)\n    at /tmp/test.js:7:13\n    at /tmp/test.js:1:1", _$5(_2, "stack", $$0);
  var _4 = "" + _$0;
  $$0.value = _4, _$5(_2, "message", $$0);
  if (_0) throw _2;else ;
}).call(this);

Note the redundant if (_0) checks and the redundant Date.now() calls. Is this really what the front-end wanted to generate?

@NTillmann NTillmann changed the title Clarify / fix nested generations Clarify / fix nested generators May 1, 2018
@hermanventer
Copy link
Contributor

Since there is no catch or finally handler in the picture, no generator should appear more than once, so this is clearly not what we want.

facebook-github-bot pushed a commit that referenced this issue May 2, 2018
Summary:
Release note: Fixed code duplication bug

Issue: #1829

When joining up a saved possibly abrupt completion with the current completion (in Functions.incorporateSavedCompletion), the state must be updated with the effects of the normal part of the join. The normal part of the join does not include the joined generator, so that must be explicitly excluded when applying the effects.

The subtle reason for this is that the type of join point is only known to the caller of incorporateSavedCompletion and, depending on the type of join point, some parts of the joined completion may be excised from the join and put back into a saved completion. This means that only the caller knows exactly which parts of the abrupt completions should have their effects (and generators) reflected in the current state.
Closes #1834

Differential Revision: D7848979

Pulled By: hermanventer

fbshipit-source-id: 80e61692ea8f3b57930b5125178b560fa3d47af5
facebook-github-bot pushed a commit that referenced this issue Sep 7, 2018
Summary:
Release note: none

Closes #2435
Closes #1829

Join.composeWithEffects composes a forked completion with subsequent effects. When two or more forks could end normally, this could result in shallow copies of the subsequent effects. These were then joined together and applied, so it was mostly OK. The generator of the subsequent effects, however, ended up being joined with itself and thus transformed the generator tree to a DAG, which is not desirable for the serializer.

The new approach is to extract a join condition from the forked completion and using it to join the subsequent effects with a newly constructed empty effects. The condition ensures that the subsequent effects are applied only in situations where the forked completion is not abrupt.
Extracting this condition makes for complicated abstract expressions and this uncovered some existing bugs and limitations that are also addressed in this pull request. As a side effect, path conditions are now longer and the time to compile unrolled loops with conditional abrupt completions inside their bodies has gone up so much that the unroll limit had to be lowered.

Please note that the expected output React tests has changed because of re-ordering. I'm none too sure that this re-ordering is necessarily benign, so please review carefully.
Pull Request resolved: #2523

Differential Revision: D9623729

Pulled By: hermanventer

fbshipit-source-id: 737096bba54a7a2ad300dc29882ea1b7829ac745
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants