Do not tree-shake children of unknown nodes

[devsnek]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
#2506

Description

Unknown nodes may have any behaviour, as they are unknown. It is
therefore unsafe to assume the unknown node, or any child of the unknown
node, is able to be removed from the render.

[devsnek]

anyone willing to merge this?

[lukastaegert]

Sorry for letting you wait as I was quite occupied with personal things for the last weeks.
I am hesitant to merge this because it should not make any difference. You did not add any test and I would expect that it is not possible to write one simply because unknown nodes are always retained due to the basic hasEffects returning true. If there is something I overlook please let me know and add a test.

[devsnek]

@lukastaegert for the following input:

    px = do {
      const _hygenicTemp = ToPrimitive(x, 'Number');

      if (_hygenicTemp instanceof AbruptCompletion) {
        return _hygenicTemp;
      } else if (_hygenicTemp instanceof Completion) {
        _hygenicTemp.Value;
      } else {
        _hygenicTemp;
      }
    };

without this change it becomes

    px = do {
      const _hygenicTemp = ToPrimitive$$1(x, 'Number');

      if (_hygenicTemp instanceof AbruptCompletion) {
        return _hygenicTemp;
      } else if (_hygenicTemp instanceof Completion) {
        _hygenicTemp.Value; // not removed because it could be an accessor
      } // final `else` block has been pruned
    };

applying this change stops this from happening.

I would be happy to add a test if you can explain the layout of your tests, as its an actual labyrinth.

[lukastaegert]

Which acorn plugin are you using? With this plugin, we could actually turn this into a test. I am very hesitant to merge new code without tests because it is very easy to break this again.

I still wonder why your change would fix this, though. The code inside the do expression looks like normal JavaScript so why would an identifier-as-expression-statement suddenly have an effect 🤔

[devsnek]

@lukastaegert it is the same as how eval('if (true) { 5 }') returns 5.

so in a do expression:

const x = do {
  if (condition) {
    return 5; // this returns the function containing this do expression. x is not set to 5
  } else if (condition2) {
    'hi'; // x will be 'hi'
  } else {
    'bye'; // x will be 'bye'
  }
};

my config looks like this right now:

    acorn: {
      plugins: { doExpressions: true },
    },
    acornInjectPlugins: [(acorn) => {
      acorn.plugins.doExpressions = function doExpressions(instance) {
        instance.extend('parseExprAtom', (superF) => function parseExprAtom(...args) {
          if (this.type === acorn.tokTypes._do) { // eslint-disable-line no-underscore-dangle
            this.eat(acorn.tokTypes._do); // eslint-disable-line no-underscore-dangle
            const node = this.startNode();
            node.body = this.parseBlock();
            return this.finishNode(node, 'DoExpression');
          }
          return Reflect.apply(superF, this, args);
        });
      };
      return acorn;
    }],

[devsnek]

but the point here is that we don't know what magical things future versions of js will do, so it's unsafe to perform anything on the contents of unknown nodes.

[lukastaegert]

Sure, but you will also understand that instead of adding potentially dead code that "looks right", I strive to add code that is proven to be right, also considering that at the moment I am more or less doing by far most of the maintenance of rollup alone. I started to look into writing a test but as I suspected, this fix does not seem to solve your dilemma.

The generated AST structure of a do expression is this:

{
  type: 'DoExpression',
  body: {
     type: 'BlockStatement',
     ...
  },
  ...
}

so the do expression just contains a plain block statement which means the usual tree-shaking logic of a block statement applies. How I understand the semantic of do expressions, we would need to treat the last statement of any possible execution branch inside the block statement the same way we would treat a return expression. For that again, we would need some kind of special flag that we are inside a do expression to apply that special logic. As this is still a stage 1 feature, I would not feel confident adding such deep changes to rollup but rather hope that people who want to use do expressions would turn to babel for now.

Nevertheless, I will check if we cannot add a generic test for a generic acorn plugin to validate the added logic.

[devsnek]

@lukastaegert the changes i made seemed to work locally for me, but i might have done something else, as i'm not familiar at all with this codebase. whatever changes need to be made should be made. children of unknown node types should not be subject to tree shaking. because that BlockStatement belongs to a node rollup doesn't understand, it should ignore it.

[lukastaegert]

children of unknown node types should not be subject to tree shaking

Now this is something I can get behind. For now, I have added a test to your branch checking for any kind of undetected side-effects of unknown expressions that do not have children and as I suspected, all was already working correctly.

But the latter is definitely correct and would solve your issue. I will not have more time today but I hope can look into this tomorrow. Until then if you can add a test for this (take my test as inspiration), that would be cool! If you have ideas how to solve the entire issue, you are of course welcome as well!

[devsnek]

@lukastaegert added new test case and its passing now 👍

[lukastaegert]

Very good!

Unfortunately, cutting off all logic for unknown nodes has some unintended side-effects, namely: Inside the do-expression, there is still "normal" JavaScript which may still use variables outside the expression. These variables will not be properly included if the logic is suppressed. I modified the test to reflects this (and changed the existing test cases to better reflect how we expect do expressions to work as I believe this makes them more readable).

I fear this will require a little more work but I am on it now. In essence, all child nodes need to be included using the "proper" inclusion logic. I will check if we can manage this via a flag.

[lukastaegert]

I implemented a more comprehensive fix now that also refactors the treeshake: false mode to use the same code path as the new logic, which also means we need to test things only once and also reduce the danger of new features not working properly when tree-shaking is off.

To make sure we do not forget this when extending the tree-shaking logic I made the new flag mandatory, which should also be optimized better by the JS runtime.

Please have a look, if this works for you then I would merge this.

[devsnek]

merge at your convenience