feat: support #__NO_SIDE_EFFECTS__ annotation for function declaration

[antfu]

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

Description

Currently, the #__PURE__ notation only supported on the function callee site, where it's usually functions' nature to be side-effect free. While Rollup already auto check if there would be side-effects inside a function to mark it pure, sometime the detection may not be perfect (#2962), especially for very complex functions.

This PR brings the capability of manually marking a function as pure on declarations, by introducing a new annotation syntax #__NO_SIDE_EFFECTS__ along side #__PURE__. When using with a function declaration, it automatically makes all calls of the function side-effect free.

All the following examples work:

/*#__NO_SIDE_EFFECTS__*/
export function fnA (args) {
  // ...
}

export const fnB = /*#__NO_SIDE_EFFECTS__*/ (args) => {
  // ...
}

/*#__NO_SIDE_EFFECTS__*/ 
export const fnC = (args) => {
  // ...
}


/**
 * Some jsdocs
 *
 * @__NO_SIDE_EFFECTS__
 */
export const fnC = (args) => {
  // ...
}

References

Existing Discussions:

• esbuild: Pure annotation for functions evanw/esbuild#1883
• UglifyJS: Extending the pure annotation for functions mishoo/UglifyJS#5234

Existing workaround:

• https://github.com/danielroe/rollup-plugin-pure - injecting /*#__PURE__*/ before callee
• https://github.com/fairysword/rollup-plugin-pure-annotation - the same

Solutions from other tools:

• https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#nosideeffects-modifies-thisarguments

Potentially solves:

• fix Make /*#__PURE__*/ not only for call, but also for callable value? #2960
• fix Annotation for pure getters (discussion) #3334
• Why post function declaration would not be treeshake? #2962 (requires manually adding the annotation)
• [tree-shaking] function with no side-effects & args included in output #2539 (requires manually adding the annotation)

References:

• my tweet: https://twitter.com/antfu7/status/1664320045797851147

TODO

• Documentation - I'll write them when the direction gets approved
• Use a more unambiguous annotation
  • https://twitter.com/lukastaegert/status/1664348230220492805
  • Standard JSDoc annotations for Pure/Unpure #3122
  • This can take some time to draft and discuss to find a good convention. Which this PR will focus on the logic handle using the existing PURE syntax. It should be addressed in another PR.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
rollup	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 6, 2023 4:38am

[codecov]

Codecov Report

Merging #5024 (16ded62) into master (b5ac3e5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5024   +/-   ##
=======================================
  Coverage   98.97%   98.97%           
=======================================
  Files         222      222           
  Lines        8180     8205   +25     
  Branches     2253     2256    +3     
=======================================
+ Hits         8096     8121   +25     
  Misses         30       30           
  Partials       54       54           

Impacted Files	Coverage Δ
src/Graph.ts	100.00% <ø> (ø)
src/ast/nodes/ArrowFunctionExpression.ts	95.23% <100.00%> (+0.50%)	⬆️
src/ast/nodes/CallExpression.ts	100.00% <100.00%> (ø)
src/ast/nodes/NewExpression.ts	100.00% <100.00%> (ø)
src/ast/nodes/shared/FunctionNode.ts	97.43% <100.00%> (+0.29%)	⬆️
src/ast/nodes/shared/Node.ts	100.00% <100.00%> (ø)
src/utils/commentAnnotations.ts	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

[Andarist]

As mentioned here by @skyrpex it seems that there is an existing @nosideeffects annotation that perhaps would be a better choice here.

[skyrpex]

As mentioned here by @skyrpex it seems that there is an existing @nosideeffects annotation that perhaps would be a better choice here.

Exactly. What we care about is about side effects, not whether the function is pure. Eg, a pure function wouldn't read global state, but if we don't use the data it returns and didn't cause any side effect, we can safely remove it.

BTW, I love where this is heading ❤️

[antfu]

I'd be happy to adapt to google-closure-compiler's convention. Which also looks much more systematic. I am just a bit hesitant because then we will end up mixing with two different conventions for /*#__PURE__*/ and /*@nosideeffects*/ - maybe we should support @nosideeffects everywhere __PURE__ is supported?

[lukastaegert]

Which this PR will focus on the logic handle using the existing PURE syntax. It should be addressed in another PR.

While I appreciate the notion, I hope you will understand that I would not want to release this with the existing annotation. If we release this, then we do this to allow people to use it, and then we created precedence and basically changed the meaning of PURE even if we did not want to.

I'd be happy to adapt to google-closure-compiler's convention. Which also looks much more systematic. I am just a bit hesitant because then we will end up mixing with two different conventions for /#PURE/ and /@nosideeffects/ - maybe we should support @nosideeffects everywhere PURE is supported?

I would actually be super happy to support existing annotations that are well-defined, regardless of style. If there are existing code-bases that use these annotations, they would immediately profit from it, and we can always extend annotation styles later.

In this case, however, @nosideeffects does not really work this way, as you can easily test yourself: https://closure-compiler.appspot.com/home#code%3D%252F%252F%2520%253D%253DClosureCompiler%253D%253D%250A%252F%252F%2520%2540compilation_level%2520SIMPLE_OPTIMIZATIONS%250A%252F%252F%2520%2540output_file_name%2520default.js%250A%252F%252F%2520%2540formatting%2520pretty_print%250A%252F%252F%2520%253D%253D%252FClosureCompiler%253D%253D%250A%250A%252F%252F%2520ADD%2520YOUR%2520CODE%2520HERE%250A%252F**%2520%2540nosideeffects%2520*%252F%250Afunction%2520hello(name)%2520%257B%250A%2520%2520alert('Hello%252C%2520'%2520%252B%2520name)%253B%250A%257D%250A%250Ahello('New%2520user')%253B%250A%250A

To my limited knowledge of Closure Compiler, this annotation only has an effect in @externs files (with the corresponding header), which are like declaration files for external dependencies and can only contain empty declarations. I.e. those files would not be processed by Rollup anyway and Rollup would never see those annotations. On the other hand, supporting them in non-@externs files would be a change of meaning.

Here is my suggestion how to move forward without being blocked:
Just use a new annotation name that sounds nice without alignment across tools. Then we present the idea to all relevant tools, and either they say "fine" or come up with something better. In that case, we just support both annotations.

Taking a leaf out of Closure Compiler's book, nosideeffects is actually a better description than pure (i.e. always has the same output for same input AND has no side effects).

So how about

• /*#__NO_SIDE_EFFECTS__*/
• /*#__NOSIDEEFFECTS__*/
• /*#__NOEFFECTS__*/

For brevity, I would be leaning towards the latter, but you may have even better ideas.

[antfu]

I have updated the implementation to use #__NO_SIDE_EFFECTS__.

• Renamed pureComments.ts to commentAnnotations.ts
• Add two new props to NodeBase, annotationPure and annotationNoSideEffects

The reason I chose it over __NOSIDEEFFECTS__ is I consider it more readable. For __NOEFFECTS__ it sounds good but I am not sure if "effects" over "side effects" could be too general. But in any case, I am open to changing it if there is a better option.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install antfu/rollup#pure-declaration

or load it into the REPL:
https://rollup-10415ttmf-rollup-js.vercel.app/repl/?pr=5024

[lukastaegert]

Looks great, just some minor comments

[lukastaegert]

Looks great, just some minor comments

[lukastaegert]

Awesome, looks great! Will release this by tomorrow if there are no further comments.

[fabiosantoscode]

If this gets implemented in Terser, I'll make sure to use the same syntax <3

[rollup-bot]

This PR has been released as part of [email protected]. You can test it via npm install rollup.

[antfu]

A side note, if you are using esbuild or rollup-plugin-esbuild to transpile your TypeScript file, the /* @__NO_SIDE_EFFECTS__ */ comment might not be preserved (evanw/esbuild#221).

A workaround to that is to add ! to your comment block as /*! @__NO_SIDE_EFFECTS__ */ (evanw/esbuild@d7679fc). Note that this would only work at the statement level.

Also, if you'd like to see this supported by esbuild natively, please vote or provide insights in this issue: evanw/esbuild#3149