Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore invalid trailing pure annotations #4068

Merged
merged 2 commits into from May 14, 2021
Merged

Ignore invalid trailing pure annotations #4068

merged 2 commits into from May 14, 2021

Conversation

kzc
Copy link
Contributor

@kzc kzc commented May 6, 2021

Ignore invalid trailing pure annotations that were incorrectly associated with subsequent calls.

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:

fixes #4062

Description

Ignore invalid trailing pure annotations that were incorrectly associated with subsequent calls causing those calls to be erroneously dropped.

A valid pure annotation comment must precede a call or new expression and only whitespace characters may appear between the annotation comment and the following call.

@github-actions
Copy link

github-actions bot commented May 6, 2021

Thank you for your contribution! ❤️

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

npm install kzc/rollup#fix-invalid-trailing-pure-annotations

or load it into the REPL:
https://rollupjs.org/repl/?pr=4068

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #4068 (40c6782) into master (2bee261) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 40c6782 differs from pull request most recent head 6a2f658. Consider uploading reports for the commit 6a2f658 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4068   +/-   ##
=======================================
  Coverage   97.49%   97.49%           
=======================================
  Files         193      193           
  Lines        6818     6824    +6     
  Branches     2002     2005    +3     
=======================================
+ Hits         6647     6653    +6     
  Misses         84       84           
  Partials       87       87           
Impacted Files Coverage Δ
src/Graph.ts 100.00% <100.00%> (ø)
src/utils/pureComments.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3345267...6a2f658. Read the comment docs.

@kzc
Copy link
Contributor Author

kzc commented May 6, 2021

@lukastaegert Can you please take over this PR?

I have no idea why npm audit would fail on only Tests / Node 12 + Extra Tests (Linux) as this PR did not change or introduce any packages:

npm ERR! rollup@2.47.0 security: `npm audit`
npm ERR! Exit status 1

Nor do I understand why coverage would decrease when both code paths of the newly introduced if statement are exercised by the tests.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I have no idea why npm audit would fail on only Tests / Node 12 + Extra Tests (Linux) as this PR did not change or introduce any packages

Well, that is the nature of security alerts: They pop up even though you did not change anything just because you triggered the test pipeline. I will see what I can do to fix this on master.

I have no idea why npm audit would fail on only Tests / Node 12 + Extra Tests (Linux) as this PR did not change or introduce any packages

Your changes apparently removed coverage for the case where a Node has more than one annotation.

It seems your code breaks the existing logic to remove trailing annotations if a subsequent node is removed, see my other comment.

console.log('should remain impure');
console.log('should remain impure');

console.log('code');
console.log('code');/*@__PURE__*///@__PURE__
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in latest PR by parsing the gap between the pure comment and the call with acorn.parse instead of just relying on whitespace being there. If it's just whitespace or comments, then there will be zero statements.

@lukastaegert
Copy link
Member

Dependencies are now fixed on master

@kzc
Copy link
Contributor Author

kzc commented May 7, 2021

It seems your code breaks the existing logic to remove trailing annotations if a subsequent node is removed, see my other comment.

Ah, okay. I missed that.

In addition, a proper fix ought to remove all invalid pure annotations, such as the ones introduced in the additional tests in this PR, as well as cases like the first annotation in the example below:

$ cat example.js 
var a = /*@__PURE__*/console.log;
var b = /*@__PURE__*/bar();
a(42, b);
$ cat example.js | rollup --silent
var a = /*@__PURE__*/console.log;
var b = /*@__PURE__*/bar();
a(42, b);
$ cat example.js | esbuild
var a = console.log;
var b = /* @__PURE__ */ bar();
a(42, b);
$ cat example.js | uglify-js -mc --annotations
var a=console.log,b=/*@__PURE__*/bar();a(42,b);

Notice the invalid pure annotation before a non-call was not emitted by esbuild and uglify.

My intention was to also remove such invalid pure annotations, but I was not familiar enough with the comment removal logic.

I just put together the original patch to advance the bug fix and don't have the desire to pursue it further. If you could take over this PR, that'd be great.

@kzc
Copy link
Contributor Author

kzc commented May 7, 2021

It seems your code breaks the existing logic to remove trailing annotations if a subsequent node is removed, see my other comment.

As noted in #4062, in some cases invalid annotations trailing a call incorrectly removed the following call which coincidentally would have been tree shaken anyway and the previous tests masked that issue. Further reason to just remove all invalid annotations in the input.

@kzc
Copy link
Contributor Author

kzc commented May 7, 2021

I didn't plan to refine the patch, but I had an epiphany to use acorn.parse to solve the problem rather easily.

The PR still does not remove invalid pure annotations however. Perhaps this could be done in a future PR.

@kzc
Copy link
Contributor Author

kzc commented May 9, 2021

Unrelated issue in 2.47.0 discovered when adding more tests: repl

$ cat main.js
/*@__PURE__*/ a(),
/*@__PURE__*/ b(),
/*@__PURE__*/ c(),
/*@__PURE__*/ d();
$ cat main.js | rollup --silent
/*@__PURE__*/ a(),
/*@__PURE__*/ d();
$ cat main.js | uglify-js -c
$
$ cat main.js | esbuild --minify
$

that were incorrectly associated with subsequent calls
@kzc
Copy link
Contributor Author

kzc commented May 12, 2021

This PR is good to merge.

The unrelated suboptimal tree shaking issue above and removing pure comments not associated with function calls and new can be addressed in a future PR.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I like your comprehensive tests! Interesting idea with the acorn parse. My naive approach would have been to use "findFirstOccurrenceOutsideComment", but that one can only search for strings and would not have been able to handle sequence expressions as well with a single call.

@lukastaegert
Copy link
Member

Ok, seems GitHub Actions is.. broken? Going to ignore this for now as the previous run was successful.

@lukastaegert lukastaegert merged commit c40d206 into rollup:master May 14, 2021
@kzc
Copy link
Contributor Author

kzc commented May 14, 2021

https://www.githubstatus.com shows:

May 14, 2021
Unresolved incident: Incident with GitHub Actions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pure annotation bug
2 participants