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

Function parameters ending with ${int} in source aren't escaped, and shadow higher scoped generated variables #2683

Closed
bloodyowl opened this issue Feb 8, 2019 · 2 comments · Fixed by #2689

Comments

@bloodyowl
Copy link

Context:
BuckleScript generates variables like name$1.

  • Rollup Version: v1.1.2
  • Operating System (or Browser): All
  • Node Version: v10.14.2

How Do We Reproduce?

Input

// main.js
import {x} from "./maths.js";

console.log(x(value));
// maths.js
var value = "1";

export function x(value$1) { return value$1 + value } 

Expected Behavior

var value$1 = "1";

function x(value$1$1) { return value$1$1 + value$1 }

console.log(x(value));

or

var value$1 = "1";

function x(value_$_1) { return value_$_1 + value$1 }

console.log(x(value));

Actual Behavior

var value$1 = "1";

function x(value$1) { return value$1 + value$1 }

console.log(x(value));

https://rollupjs.org/repl?version=1.1.2&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMCU3QnglN0QlMjBmcm9tJTIwJTVDJTIyLiUyRm1hdGhzLmpzJTVDJTIyJTNCJTVDbiU1Q25jb25zb2xlLmxvZyh4KHZhbHVlKSklM0IlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSU3RCUyQyU3QiUyMm5hbWUlMjIlM0ElMjJtYXRocy5qcyUyMiUyQyUyMmNvZGUlMjIlM0ElMjJ2YXIlMjB2YWx1ZSUyMCUzRCUyMCU1QyUyMjElNUMlMjIlM0IlNUNuJTVDbmV4cG9ydCUyMGZ1bmN0aW9uJTIweCh2YWx1ZSUyNDEpJTIwJTdCJTIwcmV0dXJuJTIwdmFsdWUlMjQxJTIwJTJCJTIwdmFsdWUlMjAlN0QlMjAlMjIlMkMlMjJpc0VudHJ5JTIyJTNBZmFsc2UlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXNtJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

@lukastaegert
Copy link
Member

Thanks, the deconflicting logic rewrite I am currently working on should be able to handle this, will definitely include your test case!

@lukastaegert
Copy link
Member

Fix a #2689

lukastaegert added a commit that referenced this issue Feb 14, 2019
lukastaegert added a commit that referenced this issue Feb 17, 2019
lukastaegert added a commit that referenced this issue Feb 17, 2019
* Refactor scope structure

* Initial draft for new deconflicting solution with variable access tracking

* Slightly improve some names

* Deconflict CJS interop function

* Cache findVariable via accessedOutsideVariables

* Get rid of unnecessary parameter and improve name

* Use objects instead of Sets to track used names

* Only track global outside variables on module scopes, re-unify external
variable handling with other import handling

* Move warning about missing exports into Graph

* Test variables are no longer needlessly deconflicted due to globals in other chunks

* Use tree-shaking information when deconflicting

* Deconflict build-in names, resolves #2680

* Make sure it resolves #2683

* Clean up chunk deconflicting code

* Change priorities so that chunk variable names take precedence over
imported variable names

* Fix acorn import fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants