Do full joins

[hermanventer]

Release note: Rewrote the joining logic to always do a full join at every join point

Closes #1829 #2151 #2222 #2279

I've spent a lot of time in the last few months trying to sort out problems that arise from effects being applied too many or too few times. Fixing these feel a bit like playing wack a mole and in the end no fix goes unpunished.

Stepping back a bit from the fray, it seems to me that the root cause of all this pain is the fact that joins of different kinds of completions get delayed.

Before we had path conditions and the simplifier this seemed like a rather good thing since exceptional paths did not contribute values to the normal paths and we thus had fewer abstract values to deal with and fewer places where Prepack would grind to a halt.

In the current state of things, however, it seems perfectly possible to join in all branches at every join point. I've had to decrease some limits, in particular the number of times we go around a loop with conditional exits. I've also had to make the test runner impose a limit on how many times the simplifier can invoke Path.implies.

Nevertheless, the tests seem to pass and hopefully this will also fix quite a lot of bugs that have been unresolved for many months already.

[sebmarkbage]

Do we need to run an internal test to see what kind of regression we're going to see in existing builds before landing?

Do you have any pointers where we should look closer for reviews?

Are none of the test/residual tests relevant to be moved? I see you deleted all.

[facebook-github-bot]

hermanventer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hermanventer]

I've deleted the entire residual feature. It did not have a lot of tests to begin with and nothing there would add to our other tests.

For review, the most interesting bits of code to read would be the logic for try-catch, for loops and function calls. Most other things are just infrastructure for those.

[trueadm]

This currently fails the internal React bundle. I'll look into it more today and try and come up with a repro.

[NTillmann]

I also want to check what it does to the InstantRender bundle before we land this.

[trueadm]

Here's a repo for the failing issue:

function joinClasses(className) {
  var newClassName = className || "";
  for (
    var _len = arguments.length,
      classes = Array(_len > 1 ? _len - 1 : 0),
      _key = 1;
    _key < _len;
    _key++
  ) {
    classes[_key - 1] = arguments[_key];
  }
  var argLength = classes.length;

  for (var index = 0; index < argLength; index++) {
    var nextClass = classes[index];
    if (nextClass != null && nextClass !== "") {
      newClassName =
        (newClassName ? newClassName + " " : "") + nextClass;
    }
  }
  return newClassName;
}

function fn(_ref) {
  var className = _ref.className;
  var comment = _ref.comment;
  var author = comment.author;
  var authorID = author && author.id;
  var authorName = author && author.name;
  if (!author || !authorID || authorName == null) {
    return null;
  }
  if (author.url) {
    return {
      props: {
        className: joinClasses(
          comment.is_author_weak_reference
            ? "somestring"
            : "",
          className
        ),

        uid: authorID
      },
      children: authorName
    }
  } else {
    return {
      props: { className: className },
      children: authorName
    };
  }
}

this.__optimize && __optimize(fn);

I found this because the createdObjects when evaluating the effects of this internal component was missing all the objects from before calling evaluateForEffects. The above repos nicely and has many missing variables.

[trueadm]

A reduced version also breaks:

function fn(_ref) {
  var className = _ref.className;
  var comment = _ref.comment;
  var author = comment.author;
  var authorID = author && author.id;
  var authorName = author && author.name;
  if (!author || !authorID || authorName == null) {
    return null;
  }
  if (author.url) {
    return {
      props: {
        className: null,

        uid: authorID
      },
      children: authorName
    }
  } else {
    return {
      props: { className: className },
      children: authorName
    };
  }
}

this.__optimize && __optimize(fn);

There are less errors in the output, but still many errors.

[facebook-github-bot]

hermanventer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[trueadm]

Great work! This fixes a whole bunch of issues :)

Also you'll need to run yarn prettier-all to fix up the test case.

[facebook-github-bot]

hermanventer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[trueadm]

@hermanventer it seems you will also need to make some changes on your imported diff for fbsource, as it tries to run test-residual which no longer exists.

[hermanventer]

Working on it. D9258610.

[sb98052]

This condition should be precluded by the check in 1141.

[sb98052]

Precluded by 1187

[sb98052]

Precluded by 1192

[hermanventer]

Indeed. The invariant just makes this more obvious and also serves as a sanity check. In general, we are open to stating redundant invariants if it makes the code more readable and if it makes it harder to accidently break the code.

[sb98052]

I have a rather basic question. Now that we are doing full joins, why do we need to retain the tree structure of completions? Why not make completions a set of summaries for each type, e.g. NormalCompletions, ReturnCompletions etc. and join path conditions in addition to values in this method. The summary would need to include the path condition in addition to the value. Doing so should eliminate a great deal of redundant work later on when path conditions are pushed, i.e. in pushPathConditionsLeadingToCompletionOfType and friends, which traverse the tree repetitively. You could still retain the type of the completion as a predicate on the size of the constituent sets.

[hermanventer]

It is a fair question, but I don't know the answer. It might be an improvement, but it is not entirely obvious to me that it would be. For now, it is probably not the most important thing we have to worry about.