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

Try to make logical expression deoptimization more robust #4519

Merged

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented May 31, 2022

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:
Resolves #4517

Description

Turns out that while unused arguments of new expressions were not included, they were still rendered, which was causing issues for certain types of arguments like logical expressions. This is fixed here. As a result, unused arguments of new expressions at the end of the call are now removed, just like unused arguments of call expressions.

@github-actions
Copy link

github-actions bot commented May 31, 2022

Thank you for your contribution! ❤️

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

npm install rollup/rollup#gh4517-improve-logical-expression-deoptimization

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

@codecov
Copy link

codecov bot commented May 31, 2022

Codecov Report

Merging #4519 (aea0df3) into master (0409bf0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4519   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files         208      209    +1     
  Lines        7432     7436    +4     
  Branches     2130     2130           
=======================================
+ Hits         7341     7345    +4     
  Misses         32       32           
  Partials       59       59           
Impacted Files Coverage Δ
src/ast/nodes/CallExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/LogicalExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/NewExpression.ts 100.00% <100.00%> (ø)
src/utils/renderCallArguments.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 0409bf0...aea0df3. Read the comment docs.

@lukastaegert lukastaegert force-pushed the gh4517-improve-logical-expression-deoptimization branch from d164f9b to aea0df3 Compare June 1, 2022 05:02
@lukastaegert lukastaegert merged commit 696ebd3 into master Jun 1, 2022
@lukastaegert lukastaegert deleted the gh4517-improve-logical-expression-deoptimization branch June 1, 2022 12:19
pos777 pushed a commit to pos777/rollup that referenced this pull request Jun 18, 2022
* Try to make logical expression deoptimization more robust

* Share argument rendering logic between call and new expression

Solves an issue where trying to render a non-included argument crashes
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.

Cannot read properties of null (reading 'render')
1 participant