Provide a basic recovery mechanism for `for` and `while` loops #2118

trueadm · 2018-06-13T17:01:13Z

Release notes: standard for loops and while loops have a recovery mechanism in pure scope

This PR provides a bail-out recovery mechanism in pure scope for FatalErrors thrown and caught from for/while loops. Until now, if a FatalError occurs from trying to evaluate abstract for/while loops, we'd have to recover at a higher point in the callstack, which wasn't always possible (the loop may be at the root of a function/component).

The ideal long-term strategy is to properly model out the different cases for loops, but this is a complex and time-consuming process. This PR adds a recovery mechanism that serializes out the original for loop, but within a newly created wrapper function containing the loop logic and a function call to that newly created function wrapper. This allows us to still run the same code at runtime, where we were unable to evaluate and optimize it at build time.

For now, this PR only adds recovery support for standard for and while loops (as they go through the same code path). We already have some basic evaluation for do while loops, but trying to adapt that code to work with the failing test case (#2055) didn't work for me – we have so many strange problems to deal with first before we can properly handle that issue.

In cases where the loop uses this, the context is found and correctly called with the wrapper function. In cases of usage of return, arguments and labelled break/continue we bail out again as this is not currently supported in the scope of this PR (can be added in a follow up PR, but I wanted to keep the scope of this PR limited).

For example, take this failing case on master:

function fn(props, splitPoint) {
  var text = props.text || "";

  text = text.replace(/\s*$/, "");

  if (splitPoint !== null) {
    while (text[splitPoint - 1] === "\n") {
      splitPoint--;
    }
  }
  return splitPoint;
}

This serializes out to:

  var _0 = function (props, splitPoint) {
    var __scope_0 = new Array(1);

    var __get_scope_binding_0 = function (__selector) {
      var __captured;

      switch (__selector) {
        case 0:
          __captured = [_G, _E];
          break;
      }

      __scope_0[__selector] = __captured;
      return __captured;
    };

    var _C = function () {
      var __captured__scope_1 = __scope_0[0] || __get_scope_binding_0(0);

      for (; __captured__scope_1[1][__captured__scope_1[0] - 1] === "\n";) {
        __captured__scope_1[0]--;
      }
    };

    var _$0 = props.text;

    var _$2 = (_$0 || "").replace(/\s*$/, "");

    var _6 = splitPoint !== null;

    var _G = _6 ? void 0 : splitPoint;

    var _E = _6 ? void 0 : _$2;

    if (_6) {
      (__scope_0[0] || __get_scope_binding_0(0))[0] = splitPoint;
      (__scope_0[0] || __get_scope_binding_0(0))[1] = _$2;

      var _$5 = _C();
    }

    var _$6 = (__scope_0[0] || __get_scope_binding_0(0))[0];

    return _$6;
  };

Furthermore, an idea that might be a good follow up PR would be to break the wrapper function into two functions, depending on some heuristics. If we can detect that the loop body does not have any unsupported side-effects like writing to variables that are havoced etc, we can then tell Prepack to optimize the inner function wrapper for the loop body. That way, at least some parts of the loop get optimized (the outer bindings will be havoced before this point, so it should work okay). I think I suggested this in person with @hermanventer and @sebmarkbage in Seattle as a potential way to optimize complex loops that we can't compute the fixed point for right now.

Fixes #2055

NTillmann · 2018-06-13T17:11:26Z

src/evaluators/ForStatement.js

+
+  if (functionInfo.usesThis) {
+    let thisRef = env.evaluate(t.thisExpression(), strictCode);
+    if (thisRef instanceof Value) {


And if it's not a value, why is it okay to just ignore it?

NTillmann · 2018-06-13T17:21:52Z

src/evaluators/ForStatement.js

 // ECMA262 13.7.4.7
 export default function(
  ast: BabelNodeForStatement,
  strictCode: boolean,
  env: LexicalEnvironment,
  realm: Realm,
  labelSet: ?Array<string>
+): Value {
+  if (realm.isInPureScope()) {
+    return tryToEvaluateForStatementOrLeaveAsAbstract(ast, strictCode, env, realm, labelSet);


Not a change request, just a thought: There's a growing set of bail-outs from just regular JavaScript code. It would be good to have some kind of logging or account for when these things happen, maybe combined with some configuration as to what kind of things are allowed. (We don't want to allow bailing out for loops or arbitrary functions in InstantRender, and I guess eventually you don't want that for the React Compiler either.) Also possibly just to give de-optimization hints to users.

For the React Compiler this okay (for now), long term we do want to support loops fully as I stated in my summary. Maybe if you have some good ideas on how to configure such a thing, you could make an issue for it with some suggestions and we could implement it – as you're right, we definitely can't do this for IR.

On a side note though, we might be able to support parts of this for the React Compiler. When we create this wrapper function, we can also check to see if the for loop is pure or has few side-effects and contain the side-effects in the wrapper function, then create another function for the loop body, then tell Prepack to optimize the inner function for the body. I thought of doing that as part of this PR, but it would really convolute the contents for now. It's best as a follow up PR.

NTillmann

Looks great!

hermanventer · 2018-06-13T17:54:57Z

src/evaluators/ForStatement.js

+  if (functionInfo.usesReturn || functionInfo.usesArguments || functionInfo.usesGotoToLabel) {
+    // We do not have support for these yet
+    // TODO: issue number upon creation of issue
+    throw new FatalError("TODO: #? handle more loop bail out cases");


You should never throw a FatalError unless you have first reported a diagnostic message.

It’s a todo. Once everyone is happy I’ll change the todo into a proper fatal

hermanventer · 2018-06-13T18:02:55Z

src/evaluators/ForStatement.js

+): AbstractValue {
+  let wrapperFunction = new ECMAScriptSourceFunctionValue(realm);
+  let body = t.blockStatement([ast]);
+  ((body: any): FunctionBodyAstNode).uniqueOrderedTag = realm.functionBodyUniqueTagSeed++;


Not for this PR, but we really should encapsulate this in a helper.

hermanventer · 2018-06-13T18:17:24Z

src/evaluators/ForStatement.js

+  if (functionInfo.usesThis) {
+    let thisRef = env.evaluate(t.thisExpression(), strictCode);
+    let thisVal = Environment.GetValue(realm, thisRef);
+    Havoc.value(realm, thisVal);


This seems a bit like papering over a huge crack in the wall. The loop can modify many other things in a way that is totally opaque to Prepack. Yet, only the this value is havoced. Better than nothing, but not much better.

Havocing is transitive. So everything reachable gets havoced.

The loop body may access locations that are not reachable from this.

What locations are you referring to? The havoc logic recursively goes through every binding and object in the AST of the loop and its body. If something isn't being reached, we need to fix it in the havoc logic. If you mean the loop body is accessing something Prepack doesn't know about, then this isn't safe for pure scope.

hermanventer · 2018-06-13T18:18:18Z

src/evaluators/ForStatement.js

+    args.push(thisVal);
+  }
+
+  Havoc.value(realm, wrapperFunction);


Can you add a comment to explain why the wrapper needs to get havoced? Preferably, there should be a test that fails if it is not havoced.

The wrapper references all relevant bindings, which in turn get havoced.

Which answers my previous question. It would be good to point this out in the actual code. It is really not that obvious.

hermanventer · 2018-06-13T18:19:46Z

src/evaluators/ForStatement.js

+  wrapperFunction.$ECMAScriptCode = body;
+  wrapperFunction.$FormalParameters = [];
+  wrapperFunction.$Environment = env;
+  // We need to scan to AST looking for "this", "return", labels and "arguments"


Why not throw?

Throw where and with what? Sorry, not sure I get what you mean here.

If the loop body throws, it terminates early. That seems similar to a loop body with a return in it. I don't see why you check for the latter, but not the former.

I understand now. Thanks, added that case.

hermanventer · 2018-06-13T22:04:37Z

src/evaluators/ForStatement.js

-    // TODO: issue number upon creation of issue
-    throw new FatalError("TODO: #? handle more loop bail out cases");
+    let diagnostic = new CompilerDiagnostic(
+      `"failed to recover from a for/while loop bail-out due to unsupported logic in loop body`,


What is with the starting "?

hermanventer · 2018-06-13T22:06:43Z

src/evaluators/ForStatement.js

+  if (functionInfo.usesThis) {
+    let thisRef = env.evaluate(t.thisExpression(), strictCode);
+    let thisVal = Environment.GetValue(realm, thisRef);
+    Havoc.value(realm, thisVal);


The loop body may access locations that are not reachable from this.

hermanventer

I'd like to see some comments before you land. Fix up the funky string. Perhaps actually do something about loop bodies with a throw. (At the very least comment in the code why not.)

facebook-github-bot

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

sebmarkbage

What happens when there's a var declaration inside the loop and the value is access outside the loop?

for (...) {
  var item = ...;
}
return item;

The var has to be stripped and treated as part of the outer environment. Can you add a test?

trueadm · 2018-06-14T00:01:22Z

@sebmarkbage I was just looking into the exact problem. :) Thanks for the example too.

trueadm · 2018-06-21T10:35:24Z

I'm seeing one last issue with this PR and leaked bindings. Will try and make a repro for it.

gaearon · 2018-06-21T13:33:49Z

Here is a reduced repro:

(function() {

  global._DateFormatConfig = {
    formats: {
      'l, F j, Y': 'l, F j, Y',
    },
  }

  var DateFormatConfig = global.__abstract ?  __abstract({}, "(global._DateFormatConfig)") : global._DateFormatConfig;
  global.__makeSimple && __makeSimple(DateFormatConfig);

  var MONTH_NAMES = void 0;
  var WEEKDAY_NAMES = void 0;

  var DateStrings = {
    getWeekdayName: function getWeekdayName(weekday) {
      if (!WEEKDAY_NAMES) {
        WEEKDAY_NAMES = [
          "Sunday",
          "Monday",
          "Tuesday",
          "Wednesday",
          "Thursday",
          "Friday",
          "Saturday"
        ];
      }

      return WEEKDAY_NAMES[weekday];
    },
    _initializeMonthNames: function _initializeMonthNames() {
      MONTH_NAMES = [
        "January",
        "February",
        "March",
        "April",
        "May",
        "June",
        "July",
        "August",
        "September",
        "October",
        "November",
        "December"
      ];
    },

    getMonthName: function getMonthName(month) {
      if (!MONTH_NAMES) {
        DateStrings._initializeMonthNames();
      }

      return MONTH_NAMES[month - 1];
    },
  };

  function formatDate(date, format, options) {
    options = options || {};

    if (typeof date === "string") {
      date = parseInt(date, 10);
    }
    if (typeof date === "number") {
      date = new Date(date * 1000);
    }
    var localizedFormat = DateFormatConfig.formats[format];
    var prefix = "getUTC";
    var dateDay = date[prefix + "Date"]();
    var dateDayOfWeek = date[prefix + "Day"]();
    var dateMonth = date[prefix + "Month"]();
    var dateYear = date[prefix + "FullYear"]();

    var output = "";
    for (var i = 0; i < localizedFormat.length; i++) {
      var character = localizedFormat.charAt(i);

      switch (character) {
        case "j":
          output += dateDay;
          break;
        case "l":
          output += DateStrings.getWeekdayName(dateDayOfWeek);
          break;
        case "F":
        case "f":
          output += DateStrings.getMonthName(dateMonth + 1);
          break;
        case "Y":
          output += dateYear;
          break;
        default:
          output += character;
      }
    }

    return output;
  }

  function fn(a, b) {
    return formatDate(a, "l, F j, Y");
  }

  global.fn = fn;

  global.__optimize && __optimize(fn);

  global.inspect = function() {
    return JSON.stringify(global.fn("1529579851072"));
  }

})();

trueadm · 2018-06-21T22:45:21Z

@NTillmann Please can you look at this failing test? I've attached the same test to this PR, so you can check it out locally and run the serializer test to see the same failure. It looks like we're close, just this last failing lazy binding case to solve.

NTillmann · 2018-06-22T17:28:33Z

Looking at it...

NTillmann · 2018-06-22T17:56:39Z

Figured out the (or at least a) root cause of why this is not working. See here: #2153. It's about Prepack and Dates, and likely not related to what you are doing in this PR.

facebook-github-bot

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

facebook-github-bot

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

trueadm added 2 commits June 13, 2018 17:40

Add for statement bail out

329dcbb

add regression tests

5590923

trueadm requested review from sebmarkbage, gaearon, hermanventer and NTillmann June 13, 2018 17:01

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

NTillmann reviewed Jun 13, 2018

View reviewed changes

addressed feedback

521d995

NTillmann reviewed Jun 13, 2018

View reviewed changes

NTillmann approved these changes Jun 13, 2018

View reviewed changes

hermanventer suggested changes Jun 13, 2018

View reviewed changes

hermanventer reviewed Jun 13, 2018

View reviewed changes

trueadm added 2 commits June 13, 2018 19:51

change fatal to a compilerdiagnostic

fcf8efe

fix typo

aa31e33

hermanventer reviewed Jun 13, 2018

View reviewed changes

trueadm added 4 commits June 13, 2018 23:08

handle detection of throw

7be0160

handle detection of throw

9e0c20d

fixed diagnostic

2687df5

add comment

fb1a773

hermanventer approved these changes Jun 13, 2018

View reviewed changes

change PP code

7e16b86

facebook-github-bot reviewed Jun 13, 2018

View reviewed changes

sebmarkbage reviewed Jun 13, 2018

View reviewed changes

Merge branch 'master' into fix-2055

132383e

trueadm mentioned this pull request Jun 14, 2018

Issue havocing value in conditional #2122

Closed

add support for var bindings

90b6438

This comment has been minimized.

Sign in to view

Merge branch 'master' into fix-2055

0fe551d

trueadm added 2 commits June 21, 2018 11:15

Update logic to use SimpleCompletions

577fc53

use same code style

620bfa1

add failing test

3309059

Merge branch 'master' into fix-2055

6dd3b1d

facebook-github-bot reviewed Jun 25, 2018

View reviewed changes

facebook-github-bot closed this in 5a5a100 Jun 25, 2018

ahmad2smile mentioned this pull request Nov 1, 2018

FatalError recovery fallback missing for AbstractValue.throwIfNotConcrete case #2055

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a basic recovery mechanism for `for` and `while` loops #2118

Provide a basic recovery mechanism for `for` and `while` loops #2118

trueadm commented Jun 13, 2018 •

edited

Loading

NTillmann Jun 13, 2018

NTillmann Jun 13, 2018

trueadm Jun 13, 2018

NTillmann left a comment

hermanventer Jun 13, 2018

trueadm Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

NTillmann Jun 13, 2018

hermanventer Jun 13, 2018

trueadm Jun 13, 2018

hermanventer Jun 13, 2018

NTillmann Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

trueadm Jun 13, 2018

hermanventer Jun 13, 2018 •

edited

Loading

trueadm Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer Jun 13, 2018

hermanventer left a comment

facebook-github-bot left a comment

sebmarkbage left a comment

trueadm commented Jun 14, 2018

This comment has been minimized.

trueadm commented Jun 21, 2018

gaearon commented Jun 21, 2018

trueadm commented Jun 21, 2018

NTillmann commented Jun 22, 2018

NTillmann commented Jun 22, 2018

facebook-github-bot left a comment

facebook-github-bot left a comment

Provide a basic recovery mechanism for for and while loops #2118

Provide a basic recovery mechanism for for and while loops #2118

Conversation

trueadm commented Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTillmann 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

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

hermanventer Jun 13, 2018 • edited Loading

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

hermanventer left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

trueadm commented Jun 14, 2018

This comment has been minimized.

trueadm commented Jun 21, 2018

gaearon commented Jun 21, 2018

trueadm commented Jun 21, 2018

NTillmann commented Jun 22, 2018

NTillmann commented Jun 22, 2018

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

Provide a basic recovery mechanism for `for` and `while` loops #2118

Provide a basic recovery mechanism for `for` and `while` loops #2118

trueadm commented Jun 13, 2018 •

edited

Loading

hermanventer Jun 13, 2018 •

edited

Loading