Move generators from PNC normal branches to joined normal effects #2274
Conversation
| let result = this.joinOrForkResults(realm, joinCondition, result1, result2, e1, e2); | ||
| if (result1 instanceof AbruptCompletion) { | ||
| if (!(result2 instanceof AbruptCompletion)) { | ||
| invariant(result instanceof PossiblyNormalCompletion); | ||
| e2.generator = emptyEffects.generator; |
There was a problem hiding this comment.
Hm, mutating shared state? Why is that okay?
There was a problem hiding this comment.
Are we at the point where we can enforce that a generator is only ever directly held by one Effects now?
There was a problem hiding this comment.
It is not OK, but right now it is necessary. Long term, I'd like to fix this. That will not be easy and is going to take quite a bit of preparation.
| let result = this.joinOrForkResults(realm, joinCondition, result1, result2, e1, e2); | ||
| if (result1 instanceof AbruptCompletion) { | ||
| if (!(result2 instanceof AbruptCompletion)) { | ||
| invariant(result instanceof PossiblyNormalCompletion); | ||
| e2.generator = emptyEffects.generator; |
There was a problem hiding this comment.
Are we at the point where we can enforce that a generator is only ever directly held by one Effects now?
|
I don't think we are at the point where there is a 1-1 correspondence between generator and effects. Although that seems highly desirable, it will make it difficult and expensive to do joins and I need to do a lot more thinking about this. |
There was a problem hiding this comment.
@hermanventer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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
Release note: none
Generators that end up in joined effects, need to disappear from the generators in the forked completions from which they have been extracted. Also, they need to be applied to the generator containing the timeline in which the fork occurs, before the fork occurs.
With some luck, this may fix a lot of issues.