Fixes incorrect warning of side-effects in #2393

[trueadm]

Release notes: none

Fixes #2393. I created the test as a React test because I couldn't see any way of making a serializer test fail if there was a warning without making many other tests fail or without adding another // some config to the test (which I believe we said we would not add anymore as there's already too many). React tests will always fail on side-effect warnings, so this takes better advantage of this.

[NTillmann]

If the loop below would start at env and not env.parent, then you wouldn't need this special case. Or is it important that this check runs before the next check around created objects that can return false? I don't get the full picture of what is being checked here and the significance of ordering.

[trueadm]

That's a good point, I found we can leverage env.destroyed and use that loop and it simplifies the code quite a bit.

[cblappert]

You could change the // expected <Severity>: ... comment so that if you don't specify any error codes it verifies that there are no errors: https://github.com/facebook/prepack/blob/3e8c638071ca0bd429b4a45ebf464276dacd85c4/scripts/test-runner.js#L501

[trueadm]

@cblappert I'm not sure that overloading the // expected comment to have no error really makes a lot of sense to those unfamiliar with that particular comment.

[NTillmann]

What's the intuition of why this is checking for whether "is defined inside pure function"?

[trueadm]

It's checking that the environment record of the binding was that of the function we're in.

[NTillmann]

And why exclude destroyed envs?

[trueadm]

If they're excluded, it means we already executed that and destroyed the bindings in that scope so we don't check if that env anymore as it's bindings are no longer valid.

[NTillmann]

But if the matching lexical environment has been destroyed, then isDefinedInsidePureFn will return false, then below we would report side effect warnings? I am confused...

[trueadm]

How can it be a pure function if the env has been destroyed? It should rightly report that there is a side-effect, as in the case of a pure function the env should never be destroyed.

[NTillmann]

Okay, I think I understand. The destroyed-aspect is the key in determining whether this modified binding represents a side effect.

This simplified version of the function also passes all the public tests in your PR. Anything wrong with that? If it looks right, can we go with this version?

    const isDefinedInsidePureFn = root => {
      let context = this.getRunningContext();
      let { lexicalEnvironment: env } = context;
      while (env) {
        if (env.environmentRecord === root) {
          // We can look at whether the lexical environment of the binding was destroyed to determine if it was defined outside the current pure running context.
          return !env.destroyed;
        }
        env = env.parent;
      }
      return false;
    };

[trueadm]

Updated PR with this code. :)

[hermanventer]

Looks good to me.

[hermanventer]

Not your fault, but while you are here, would it be possible to change this to "env !== null". We avoid implicit conversions to boolean whenever possible.

[facebook-github-bot]

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