Skip to content

Commit

Permalink
experimental commit: derive names for function expressions when we can
Browse files Browse the repository at this point in the history
Related issues:

* jashkenas/coffeescript#15
* jashkenas/coffeescript#366
* jashkenas/coffeescript#758

an many more. See also https://github.com/jashkenas/coffee-script/wiki/FAQ

Any objections before I merge this into `master`?
  • Loading branch information
michaelficarra committed Dec 30, 2012
1 parent a4a4c7a commit 45c5d56
Show file tree
Hide file tree
Showing 10 changed files with 115 additions and 107 deletions.
8 changes: 4 additions & 4 deletions lib/coffee-script/command.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

68 changes: 35 additions & 33 deletions lib/coffee-script/compiler.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

25 comments on commit 45c5d56

@devinrhode2
Copy link

Choose a reason for hiding this comment

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

Exactly what sort of js will this output?

@vendethiel
Copy link
Contributor

Choose a reason for hiding this comment

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

Click on the "Diff suppressed. Click to show." ;).

@devinrhode2
Copy link

Choose a reason for hiding this comment

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

This will indeed cloud things up with every function a having a reflective a$ function being defined (and a !== $a !!) in oldIE

@kangax has the ultimate know how when it comes to this. Could you provide a recommendation?

@devinrhode2
Copy link

Choose a reason for hiding this comment

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

To prevent clouding the namespace, prevent memory leaks, etc, this style should probably be used:

var someFn = (function() {
  function someFn() { 
    //vanilla function declaration!
  }
  return someFn;
})();

@vendethiel
Copy link
Contributor

Choose a reason for hiding this comment

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

See the different comments of @michaelficarra

@devinrhode2
Copy link

Choose a reason for hiding this comment

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

I suppose var f = function f$(){}; isnt' that bad, I have some js that uses this style same style var a = function aFn(){};

But the style I suggest 2 comments up would probably less error prone, I'm not 100% sure.

One factor is that var f = function f$(){} opens the door for people to recurse in strange scenarios with f$

In many scenarios, a regular function declaration is a clean cross browser way to define a function with a name (jQuery has 55 function declarations) But of course function declarations only work in certain scenarios

@michaelficarra
Copy link
Owner Author

Choose a reason for hiding this comment

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

@devinrhode2: f$ is a fresh name, and is therefore never referenced elsewhere.

@devinrhode2
Copy link

@devinrhode2 devinrhode2 commented on 45c5d56 Dec 31, 2012 via email

Choose a reason for hiding this comment

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

@devinrhode2
Copy link

@devinrhode2 devinrhode2 commented on 45c5d56 Dec 31, 2012 via email

Choose a reason for hiding this comment

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

@devinrhode2
Copy link

@devinrhode2 devinrhode2 commented on 45c5d56 Dec 31, 2012 via email

Choose a reason for hiding this comment

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

@kangax
Copy link

@kangax kangax commented on 45c5d56 Jan 2, 2013

Choose a reason for hiding this comment

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

Why not just use var inspect = function inspect() { ... }? If assignment is already happening in local scope, the JScript NFE identifier leakage is not a concern.

@michaelficarra
Copy link
Owner Author

Choose a reason for hiding this comment

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

@kangax:

$ node
> var inspect = function inspect(self){ return inspect === self; }
undefined
> inspect(inspect)
true
> 

That would be false in IE6, no? If not, that's actually an option. We just have to watch out for this case:

obj.default = -> "what should this function's name be?"

We could always make the name safe by prepending an underscore or something in those cases, though. Fresh names (as in this implementation) also solve that problem.

There's also this problem:

obj = {}
name = "not a function"
obj.name = -> # "name" in here would now refer to the NFE, not the `name` var

So in that case, the function name does need to be fresh.

@vendethiel
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember microsoft's old ajax framework. They used this style : (iirc)
a.b = function a$b (){}

@michaelficarra
Copy link
Owner Author

Choose a reason for hiding this comment

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

@Nami-Doc: Compile this for me: a[some arbitrary complex expression].b = ->. Also, it still needs to be fresh so it doesn't conflict with the outside user-defined var a$b.

@epidemian
Copy link

Choose a reason for hiding this comment

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

I may be completely mistaken, but i was under the impression that the problem with IE was that the name of the named function expression was hoisted into the enclosing scope, therefore making this code behave differently across browsers:

console.log(foo); // undefined on most browsers; not undefined on IE, maybe, IDK 
var foo = function foo() {};

@epidemian
Copy link

Choose a reason for hiding this comment

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

Using fresh names as this patch does is the way to go IMO. Some kinds of assignable expressions, like foo.bar = -> ... could be special cased to genSym 'foo$bar' as @Nami-Doc suggested, which i think might be reasonable; but meaningful names for things like foo["do#{lolwut}"] = -> ... are probably impossible to find 😅

@michaelficarra
Copy link
Owner Author

Choose a reason for hiding this comment

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

@epidemian: That is true. I remembered it was exposed, I forgot it was also hoisted. What a terrible series of bugs they introduced... Alright, that's completely off the table. All names must be fresh (as they are in this commit). Still, as @Nami-Doc mentioned, #131 is closed because of the FUD around this bug. I don't want it to negatively influence the merge into jashkenas/coffee-script.

edit: You beat me to it.

@vendethiel
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you van still try.
foo["do#{bar}"] = -> giving something likefoo$do$$bar$$ is a solution but probably too complex. Anyway, having these specialcased (unnamed) doesn't seem that bad to me.

@devinrhode2
Copy link

@devinrhode2 devinrhode2 commented on 45c5d56 Jan 2, 2013 via email

Choose a reason for hiding this comment

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

@vendethiel
Copy link
Contributor

Choose a reason for hiding this comment

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

genSym takes care of conflicts

@kangax
Copy link

@kangax kangax commented on 45c5d56 Jan 2, 2013

Choose a reason for hiding this comment

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

@michaelficarra no, afaik the inspect snippet should return true since foo from variable declaration+assignment is shadowing foo from IE's incorrect function declaration. But because of this incorrect parsing of NFE as function declaration it does get hoisted, like @epidemian said. Dunno if this would be a problem here.

@devinrhode2
Copy link

@devinrhode2 devinrhode2 commented on 45c5d56 Jan 2, 2013 via email

Choose a reason for hiding this comment

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

@Constellation
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of JScript bug, IE6 leaks function expression name definition to upper scope.

console.log(typeof leak);  // => "function"
var i = function leak() {
};

@michaelficarra
Copy link
Owner Author

Choose a reason for hiding this comment

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

@Constellation: Yes, we know. That's why they're fresh names. Then it doesn't matter that it leaks into the containing scope, since it's guaranteed to never be referenced.

@Constellation
Copy link
Contributor

Choose a reason for hiding this comment

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

Then it doesn't matter that it leaks into the containing scope, since it's guaranteed to never be referenced.

I see. Thanks!

Please sign in to comment.