PURE annotations ignored if plugin parses the ast

[yannayl]

• Rollup Version: 2.40.0
• Operating System (or Browser): any
• Node Version (if applicable):
• Link to reproduction (IMPORTANT, read below): https://repl.it/@yannayl/rollup-repro-plugin-ignore-pure-annotation

Expected Behavior

A plugin which calls this.parse (i.e. hooking load or transform) and doesn't modify anything should not affect the tree-shaking.

Actual Behavior

If a plugin calls this.parse rollup ignores the /*@__PURE__*/ annotation.

Root Cause

Accounting for /*@__PURE__*/ annotation happens in the Module.setSource function in conjunction with the tryParse function in the same file. However, this function is not called if ast already exists.

[lukastaegert]

Good catch. As we are actually adding this information as non-standard properties to the AST nodes, we should actually be able to do this in this.parse as well. Hope to find some time soon to look into this.

[yannayl]

I started working on it, hope to submit a PR soon

[yannayl]

I have submitted a PR, currently without tests with tests. However, I think the fix is incomplete.

(1) There is a second problem related to the same root cause - the removal of sourceMapping comment is also done only in Module.setSource if the ast is not provided. Should we open a different issue for this? I am not sure how to solve it.

(2) How can we know the user call to this.parse(someCode) is actually parsing the current module's code and not just random code? Currently the documentation states:

this.parse(code: string, acornOptions?: AcornOptions) => ESTree.Program

Use Rollup's internal acorn instance to parse code to an AST.

and in the hooks documentation it says:

To prevent additional parsing overhead in case e.g. this hook already used this.parse to generate an AST for some reason, this hook can optionally return a { code, ast, map } object.

But nothing guarantees this.parse is used exclusively for the modules code. It can be used multiple times or not to be used at all (some other third-party parser for that matter).

I have a few possible solutions to this problem, ordered from best to worst IMHO:

1. Expose some kind of Node Annotations function in the PluginContext - this.annotateNode(ast: acorn.Node, node: acorn.Node, annotation: enum {pure}) which would add pure annotation to this specific node.
   This has the huge benefit that we may add more possible annotations (e.g. keep, noTreeShake) and support features like Ability to mark a code block as non-removable. #3870
2. Add a new PluginContext_ method named parseModule(code: string, options: any): acorn.Node which should be used only for parsing the module's code and MUST be used. Should the users call this method, they must return the code passed to it and the ast returned from it (possibly after manipulation) from the hook. We could change the documentation for this.parse to honor this requirement, but it's incompatible change to the API.

[lukastaegert]

There is a second problem related to the same root cause - the removal of sourceMapping comment is also done only in Module.setSource if the ast is not provided. Should we open a different issue for this? I am not sure how to solve it.

Yes, separate

How can we know the user call to this.parse(someCode) is actually parsing the current module's code

We don't, so it should be done in a way that this does not matter. Will try to have a look tomorrow.

[lukastaegert]

What is the problem if the non-standard fields "pure" and "annotations" are added to other code?

[yannayl]

What is the problem if the non-standard fields "pure" and "annotations" are added to other code?

1. It's not documented anywhere and if we document it it will pose a very restrictive design for future changes (the annotation must be carried inside the ast nodes)
2. markPureNode also annotates the expression of an ExpressionStatement - i.e. it's not trivial for a user to implement.

If we use a well-defined annotateNode function we can overcome these problems.
Of course this method will initially just add pure and annotations as currently implemented.

[yannayl]

There is a second problem related to the same root cause - the removal of sourceMapping comment is also done only in Module.setSource if the ast is not provided. Should we open a different issue for this? I am not sure how to solve it.

Yes, separate

Done: #3982