Skip to content

Fat arrow leaks generated _this reference to other closures #3143

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

Closed
joshma opened this issue Aug 29, 2013 · 6 comments
Closed

Fat arrow leaks generated _this reference to other closures #3143

joshma opened this issue Aug 29, 2013 · 6 comments

Comments

@joshma
Copy link

joshma commented Aug 29, 2013

Using a fat arrow generates a _this variable for use in the current closure, but due to how the JS engines work it gets leaked to all closures in the function scope:

@foo = 42

printFoo = => # This creates a "_this" variable
  console.log @foo

bar = ->
  debugger # When Chrome's debugger stops here, the "_this" variable is visible under the "Closure Scope" variables
  # console.log (eval '_this').foo # If this is executed, it'll frigging work!

bar()

(Thanks to @epidemian for the example, see end of #3052)

In the example above, printFoo uses a fat arrow to create a closure on _this, but bar accidentally gets a reference through the Closure Scope as well. Even if all references to printFoo are discarded, a reference to bar will indirectly hold a reference to _this and prevent it from being garbage collected.

Given that the use of a closure is an implementation detail and not part of the fat-arrow abstraction, this seems like an undesirable side-effect. As I suggested in #3052, an IIFE approach should provide the same abstraction without this issue:

bar = => console.log(@)
// ...translates to
bar = (function(_this) {
  return function() {
    console.log(_this);
  }
})(this)

(Both @epidemian and @michaelficarra seem to somewhat agree.)

@michaelficarra
Copy link
Collaborator

I forgot about arguments shadowing in #3052. The correct compilation would be

(function(){
  var $this = this;
  return function ...
}.apply(this, arguments))

as I've done in michaelficarra/CoffeeScriptRedux@2eca5c2

@joshma
Copy link
Author

joshma commented Aug 30, 2013

I'm relatively new to these neck of the woods, but for my own enlightenment why do arguments need to be passed in? It seems like it won't reach the inner-most function anyways, since that'll have its own arguments from when it's called.

@michaelficarra
Copy link
Collaborator

Ah crap, you're right. I'm so used to having to watch out for arguments shadowing when IIFE-wrapping things, I forgot that the bound function would shadow arguments anyway. This is why I didn't merge to master yet. Fixing.

michaelficarra added a commit to michaelficarra/CoffeeScriptRedux that referenced this issue Aug 30, 2013
michaelficarra added a commit to michaelficarra/CoffeeScriptRedux that referenced this issue Aug 30, 2013
@jashkenas
Copy link
Owner

It's really a shame that we have to add an extra closure wrapper (like we used to), for this ... but there's no other possible way around it, is there?

@jashkenas
Copy link
Owner

Let me know if that looks alright to y'all, or if it can be simplified...

@joshma
Copy link
Author

joshma commented Nov 2, 2013

👍 Thanks for getting to this! Looks good from my (very basic) understanding of the coffee source.

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

No branches or pull requests

3 participants