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

Add tests for nested conditional regressions #2288

Conversation

calebmer
Copy link
Contributor

@calebmer calebmer commented Jul 18, 2018

@gaearon found a regression to the cases I fixed in #2255. The following code:

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:

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?

@trueadm
Copy link
Contributor

trueadm commented Jul 19, 2018

I looked into this too and feel the right strategy is to probably revert #2274. What do you think @hermanventer?

@calebmer
Copy link
Contributor Author

calebmer commented Jul 27, 2018

Reverting #2274 to demonstrate the tests added in this PR pass. (Including the case added in #2274.)

@hermanventer
Copy link
Contributor

I suspect that landing this would create merge conflicts for #2402. Once that is landed, I expect the tests to pass and you can land them without having to revert #2274.

@calebmer calebmer force-pushed the abstract-conditional-regression-tests branch from b570293 to 607957a Compare August 24, 2018 17:11
@calebmer
Copy link
Contributor Author

Finally rebased past @hermanventer’s changes. These tests are fixed! We can land them as regression tests.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@calebmer calebmer deleted the abstract-conditional-regression-tests branch August 29, 2018 16:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants