Effects contain NormalCompletion not Value #2110

cblappert · 2018-06-11T18:46:01Z

Release Notes: None

In an effort to unify Completions and Effects (many completions are tied to effects whose result should be the completion), I change evaluateForEffects to return a NormalCompletion whereever it would otherwise return a Value. Now all EvaluationResults are Completions or References.

Fairly mechanical refactor (except slightly tricky because PossiblyNormalCompletions are NormalCompletions, so instead of checking instanceof Value you should check instanceof NormalCompletion && ! instanceof PossiblyNormalCompletion).

gaearon · 2018-06-11T21:22:46Z

NestedThrowEffects runs into an invariant it seems.

test/serializer/additional-functions/NestedThrowEffects.js {"delayInitializations":false,"inlineExpressions":false,"residual":false}
{ Invariant Violation: undefined
This is likely a bug in Prepack, not your code. Feel free to open an issue on GitHub.
    at invariant (/home/circleci/project/lib/invariant.js:10:136)
    at JoinImplementation.joinOrForkResults (/home/circleci/project/lib/methods/join.js:9:41726)
    at JoinImplementation.joinForkOrChoose (/home/circleci/project/lib/methods/join.js:9:30226)
    at composeNestedThrowEffectsWithHandler (/home/circleci/project/lib/evaluators/TryStatement.js:9:8204)
    at exports.default (/home/circleci/project/lib/evaluators/TryStatement.js:9:2306)
    at LexicalEnvironment.evaluateAbstract (/home/circleci/project/lib/environment.js:9:55282)
    at LexicalEnvironment.evaluate (/home/circleci/project/lib/environment.js:9:54439)
    at LexicalEnvironment.evaluateCompletion (/home/circleci/project/lib/environment.js:9:39246)
    at LexicalEnvironment.evaluateCompletionDeref (/home/circleci/project/lib/environment.js:9:38706)
    at FunctionImplementation.EvaluateStatements (/home/circleci/project/lib/methods/function.js:9:47736) name: 'Invariant Violation' }

hermanventer · 2018-06-11T21:50:38Z

src/evaluators/BinaryExpression.js

@@ -230,6 +230,8 @@ export function computeBinary(
      }
      // return or throw completion
      if (completion instanceof AbruptCompletion) throw completion;
+      if (completion instanceof NormalCompletion && !(completion instanceof PossiblyNormalCompletion))


At this point (at least in master) completion is guaranteed be an instance of Value.

hermanventer · 2018-06-12T00:42:09Z

src/completions.js

 }

 // Normal completions are returned just like spec completions
-export class NormalCompletion extends Completion {}
+export class NormalCompletion extends Completion {
+  toValue(): Value {


Probably not needed.

hermanventer · 2018-06-12T00:43:37Z

src/evaluators/CallExpression.js

@@ -290,6 +290,8 @@ function tryToEvaluateCallOrLeaveAsAbstract(
  }
  // return or throw completion
  if (completion instanceof AbruptCompletion) throw completion;
+  if (completion instanceof NormalCompletion && !(completion instanceof PossiblyNormalCompletion))


completion cannot be an instance of PossiblyNormalCompletion at this point.

hermanventer · 2018-06-12T00:45:04Z

src/evaluators/DoWhileStatement.js

@@ -73,6 +73,7 @@ export default function(
  // so instead try to compute a fixpoint for it
  let iteration = () => {
    let bodyResult = env.evaluateCompletion(body, strictCode);
+    if (bodyResult instanceof Value) bodyResult = new NormalCompletion(bodyResult);


I would have expected that evaluateCompletion will never return a Value and that this can be part of its type annotation.

hermanventer · 2018-06-12T00:47:29Z

src/evaluators/ForInStatement.js

@@ -142,7 +142,13 @@ function emitResidualLoopIfSafe(
      strictCode,
      blockEnv
    );
-    if (result instanceof Value && gen.empty() && modifiedBindings.size === 0 && modifiedProperties.size === 1) {
+    if (
+      result instanceof NormalCompletion &&


Come to think of it, why not introduce a new subtype of NormalCompletion called "SimpleNormalCompletion"?

hermanventer · 2018-06-12T00:49:45Z

src/methods/function.js

@@ -1122,6 +1122,7 @@ export class FunctionImplementation {
  // If c is undefined, the result is just realm.savedCompletion.
  // Call this only when a join point has been reached.
  incorporateSavedCompletion(realm: Realm, c: void | AbruptCompletion | Value): void | Completion | Value {


You probably want to remove "| Value" from the type of c.

hermanventer · 2018-06-13T21:39:59Z

src/evaluators/LogicalExpression.js

@@ -117,6 +123,8 @@ export default function(

  // return or throw completion
  if (completion instanceof AbruptCompletion) throw completion;
+  if (completion instanceof SimpleNormalCompletion) completion = completion.value;
+  if (result2 instanceof SimpleNormalCompletion) result2 = result2.value;
  invariant(completion instanceof Value); // references do not survive join


The comment is now out of date. Just delete it.

hermanventer · 2018-06-13T21:43:02Z

src/methods/function.js

@@ -1122,6 +1128,7 @@ export class FunctionImplementation {
  // If c is undefined, the result is just realm.savedCompletion.
  // Call this only when a join point has been reached.
  incorporateSavedCompletion(realm: Realm, c: void | AbruptCompletion | Value): void | Completion | Value {
+    invariant(!(c instanceof SimpleNormalCompletion), "TODO: This function doesn't accept NormalCompletions");


The type annotation already precludes c from being a normal completion. There is no need for it and there is no todo here either.

hermanventer · 2018-06-13T21:44:55Z

src/methods/join.js

@@ -129,13 +130,13 @@ export class JoinImplementation {
  ): PossiblyNormalCompletion | Value {
    if (leftCompletion instanceof PossiblyNormalCompletion) {
      if (rightCompletion instanceof PossiblyNormalCompletion) {
-        this.updatePossiblyNormalCompletionWithValue(realm, rightCompletion, resultValue);
+        this.updatePossiblyNormalCompletionWithValue(realm, rightCompletion, new SimpleNormalCompletion(resultValue));


Given the name of the method, it would be more appropriate to do the wrapping inside the method.

hermanventer · 2018-06-13T21:48:27Z

src/methods/join.js

@@ -341,27 +352,42 @@ export class JoinImplementation {
    realm: Realm,
    joinCondition: AbstractValue,
    pnc: PossiblyNormalCompletion,
-    v: Value
+    nc: SimpleNormalCompletion


Given the name of the method, do the wrapping inside it rather than at the point of call.

hermanventer · 2018-06-13T21:51:43Z

src/methods/join.js

-    invariant(rv instanceof Value);
-    a.value = rv;
-    return new PossiblyNormalCompletion(rv, joinCondition, fc, ce, a, ae, []);
+    let rv = this.collapseResults(realm, joinCondition, this._getNormalCompletion(c), this._getNormalCompletion(a));


Rather join just join the values.

hermanventer · 2018-06-13T21:56:00Z

src/methods/join.js

    if (result1 instanceof AbruptCompletion) {
      let value = result2;
      let savedEffects;
      let savedPathConditions = [];
      if (result2 instanceof PossiblyNormalCompletion) {
-        value = result2.value;
+        value = this._getNormalCompletion(result2);


You probably want to rename value to completion.

hermanventer · 2018-06-13T21:57:09Z

src/methods/properties.js

@@ -341,6 +341,8 @@ export class PropertiesImplementation {

      // return or throw completion
      if (completion instanceof AbruptCompletion) throw completion;
+      if (completion instanceof SimpleNormalCompletion && !(completion instanceof PossiblyNormalCompletion))


At this stage completion cannot be a PossiblyNormalCompletion.

hermanventer · 2018-06-13T21:58:16Z

src/methods/widen.js

    if (result1 instanceof PossiblyNormalCompletion || result2 instanceof PossiblyNormalCompletion) {
      //todo: #1174 figure out how to deal with loops that have embedded conditional exits
      // widen join pathConditions
      // widen normal result and Effects
      // use abrupt part of result2, depend stability to make this safe. See below.
      throw new FatalError();
    }
+    if (result1 instanceof SimpleNormalCompletion && result2 instanceof SimpleNormalCompletion) {


Moving the code around like this makes blame less effective.

hermanventer · 2018-06-13T21:59:02Z

src/realm.js

    invariant(completion instanceof Value);
+    //TODO: fix


hermanventer · 2018-06-13T21:59:53Z

src/realm.js

@@ -1627,7 +1642,7 @@ export class Realm {
          return f();
        } finally {
          for (let priorEffect of priorEffects) {
-            invariant(!priorEffect.canBeApplied);
+            invariant(!priorEffect.canBeApplied, "Prior effects not applied");


The message seem to imply that canBeApplied is true.

hermanventer · 2018-06-14T00:11:02Z

src/methods/join.js

-    invariant(rv instanceof SimpleNormalCompletion);
-    a.value = rv.value;
-    return new PossiblyNormalCompletion(rv.value, joinCondition, fc, ce, a, ae, []);
+    //let rv = this.collapseResults(realm, joinCondition, this._getNormalCompletion(c), this._getNormalCompletion(a));


Did you mean to leave this in?

hermanventer · 2018-06-14T00:11:21Z

src/methods/join.js

@@ -377,6 +396,7 @@ export class JoinImplementation {
  }

  _getNormalCompletion(pnc: PossiblyNormalCompletion): SimpleNormalCompletion {
+    // TODO: unify with getNormalCompletion in PossiblyNormalCompletion


issue number

hermanventer · 2018-06-14T00:13:17Z

src/utils/modules.js

@@ -264,6 +261,9 @@ export class ModuleTracer extends Tracer {
              realm.handleError(warning);
              result = result.value;
              realm.applyEffects(effects, `initialization of module ${moduleIdValue}`);
+            } else if (result instanceof SimpleNormalCompletion) {


This did not have to move.

hermanventer · 2018-06-14T00:13:56Z

src/utils/generator.js

-      output.emitReturnValue(result);
+    if (result instanceof PossiblyNormalCompletion || result instanceof ForkedAbruptCompletion) {
+      output.emitIfThenElse(result, realm);
+    } else if (result instanceof SimpleNormalCompletion) {


You did not have to reorder these.

facebook-github-bot

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

facebook-github-bot

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

…ate (#2134) Summary: Release Notes: None Follow up on #2110, this PR makes progress towards unifying PossiblyNormalCompletion and ForkedAbruptCompletion as well as unifying completion and effects. Introduces `updateConsequentKeepingCurrentEffects` for a common pattern of updating one of the completions for a PNC/FAC and fixing up the effects. Closes #2134 Differential Revision: D8525738 Pulled By: cblappert fbshipit-source-id: c415eea47cd786e39820138f84213c2cc9d5e891

cblappert added 2 commits June 11, 2018 11:28

Effects return NormalCompletion instead of Value

b2c6565

Merge branch 'master' into refactor2

c032fb4

cblappert requested a review from hermanventer June 11, 2018 18:46

facebook-github-bot added the CLA Signed label Jun 11, 2018

Fix test

b240e13

hermanventer reviewed Jun 11, 2018

View reviewed changes

cblappert added the WIP This pull request is a work in progress and not ready for merging. label Jun 11, 2018

Fix style, 1 bug

312efb3

hermanventer reviewed Jun 12, 2018

View reviewed changes

NormalCompletion > SimpleNormalCompletion

f320ffc

trueadm mentioned this pull request Jun 13, 2018

Infinite loop whilst trying to evaluate for-loop logic created by Babel compiling a for-of loop #2053

Closed

cblappert added 3 commits June 13, 2018 10:41

WIP

f0b2734

Fix flow errors/lint

a2c3057

Fix conditions2

4c1f044

hermanventer reviewed Jun 13, 2018

View reviewed changes

cblappert added 2 commits June 13, 2018 16:00

Fixed some comments

179a322

Fixed a bunch of issues

4211705

cblappert removed the WIP This pull request is a work in progress and not ready for merging. label Jun 13, 2018

hermanventer approved these changes Jun 14, 2018

View reviewed changes

Small nits

770e920

facebook-github-bot reviewed Jun 14, 2018

View reviewed changes

cblappert added 2 commits June 18, 2018 12:58

Quick fix to module system

de1f64f

Merge branch 'master' into refactor2

24183cb

facebook-github-bot reviewed Jun 18, 2018

View reviewed changes

facebook-github-bot closed this in b59e918 Jun 18, 2018

cblappert mentioned this pull request Jun 19, 2018

Store consequentEffects and alternateEffects in consequent and alternate #2134

Closed

hermanventer deleted the refactor2 branch June 25, 2018 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Effects contain NormalCompletion not Value #2110

Effects contain NormalCompletion not Value #2110

cblappert commented Jun 11, 2018

gaearon commented Jun 11, 2018

hermanventer Jun 11, 2018

hermanventer Jun 12, 2018

hermanventer Jun 12, 2018

hermanventer Jun 12, 2018

hermanventer Jun 12, 2018

hermanventer Jun 12, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 14, 2018

hermanventer Jun 14, 2018

hermanventer Jun 14, 2018

hermanventer Jun 14, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

Effects contain NormalCompletion not Value #2110

Effects contain NormalCompletion not Value #2110

Conversation

cblappert commented Jun 11, 2018

gaearon commented Jun 11, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this 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