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

Correctly create outside variable when shadowed by catch parameter #4178

Merged
merged 1 commit into from Jul 15, 2021

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jul 14, 2021

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 #4176

Description

Apparently, a var declared in a catch clause will always create a hoisted variable, even if the variable name matches the clause parameter. If that is the case, then the initializer goes to the parameter instead of the variable...
This handles this correctly by always creating a hoisted variable while deoptimizing the catch parameter if there is a conflict.

@github-actions
Copy link

@github-actions github-actions bot commented Jul 14, 2021

Thank you for your contribution! ❤️

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

npm install rollup/rollup#gh-4176-wrong-catch-scope

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

@codecov
Copy link

@codecov codecov bot commented Jul 14, 2021

Codecov Report

Merging #4178 (3098d55) into master (89b9370) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4178   +/-   ##
=======================================
  Coverage   98.33%   98.33%           
=======================================
  Files         202      202           
  Lines        7220     7221    +1     
  Branches     2111     2111           
=======================================
+ Hits         7100     7101    +1     
  Misses         58       58           
  Partials       62       62           
Impacted Files Coverage Δ
src/ast/nodes/CatchClause.ts 83.33% <ø> (ø)
src/ast/scopes/CatchScope.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 89b9370...3098d55. Read the comment docs.

@kzc
Copy link
Contributor

@kzc kzc commented Jul 14, 2021

This parse issue may be beyond rollup's control...

Given:

$ cat catch-shadows-function.js 
"use strict";
function c() {
    console.log("PASS");
}
try {} catch (c) {
    var c;
}
c();

Expected output on all NodeJS versions I tried (agrees with Chrome and Firefox):

$ cat catch-shadows-function.js | node
PASS

Actual:

$ cat catch-shadows-function.js | rollup --silent
[!] Error: Identifier 'c' has already been declared (Note that you need plugins to import files that are not JavaScript)
- (6:8)

The error comes from acorn:

$ cat catch-shadows-function.js | node_modules/.bin/acorn --module --ecma2020
Identifier 'c' has already been declared (6:8)
$ grep version node_modules/acorn/package.json 
  "version": "8.4.0",

@kzc
Copy link
Contributor

@kzc kzc commented Jul 14, 2021

Never mind. Evidently if the file suffix is changed from js to mjs or node --input-type=module is used then recent versions of NodeJS will produce the same error as acorn and rollup. And sure enough, if <script type="module"> is used in a browser it will also produce the same error.

I take it that Rollup implicitly assumes all input is a module?

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Jul 15, 2021

Exactly, there is an option passed to acorn with that assumption

@lukastaegert lukastaegert merged commit ccdf124 into master Jul 15, 2021
9 checks passed
@lukastaegert lukastaegert deleted the gh-4176-wrong-catch-scope branch Jul 15, 2021
ovr added a commit to cube-js/cube.js that referenced this issue Jul 20, 2021
There is a regression in latest release, in PR rollup/rollup#4178
ovr added a commit to cube-js/cube.js that referenced this issue Jul 20, 2021
There is a regression in latest release, which caused a OOM. Issue: rollup/rollup#4181, Probably this PR rollup/rollup#4178 caused a regression.
ovr added a commit to cube-js/cube.js that referenced this issue Jul 20, 2021
There is a regression in latest release, which caused a OOM. Issue: rollup/rollup#4181, Probably this PR rollup/rollup#4178 caused a regression.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants