-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix implicit loop closure, support do (x) ->
#959
Comments
Nice writeup, Trevor. I was having trouble teasing apart the issue before this. I'm in favor of the alternative approach you outlined. I'm comfortable with my control structures not closing over my variables, as I'm sure many other JS devs are, and I'd be more confused by the existing behavior. There being a syntactic way to semi-simulate block level scope seems like the best of both worlds. I'm curious, though: why overload the |
@geraldalewis I don't want to get too deep into the issue 788 discussion, but in my opinion, The full form is |
@TrevorBurnham - ahh -- I'd missed that |
This isn't a part of the proposal (nor the current implementation in Coco). |
The purpose of |
@satyr Thanks, I've clarified the use of |
I don't have any philosophical problems whatsoever with introducing block scope. In my eyes, it's less of a deviation from Javascript then the compulsive list comprehensions. But since obviously it's really hard to get this right, I agree that it would be better to drop the feature in favour of simplicity. I think I like the idea of the I'm wondering about the best syntax, though. Should the arrow be mandatory? What about the argument list. For example, one could imagine Another question I'm asking myself is whether to use |
@odf Good points. I think that the proposed As to |
@TrevorBurnham Yes, I think I agree that the I didn't realise that about |
Yes, I suppose It's true that |
Thanks for detailing this issue, Trevor. On master, the current situation is this:
|
I'm fine with the scoped The |
I like the Like TrevorBurnham, I'm still very much in favour of the proposed |
I've talked about this a bit with Trevor out-of-band, but it's currently my opinion that loops "that need to close over certain values but not others" are an anti-pattern. Those loops can always be written in a clearer, more structured way, and it's pretty nasty to have to check a declaration to determine why some variables are being shared across iterations, and some variables aren't. If we can keep you from having to bother dealing with that whole can of worms, in CoffeeScript, I think we'd be better off in the end. |
The Anyway, here's what we can do right now, without implicit loop scoping or the new
Note that I introduce a new scope only where I want it. I can still do anything I want in the rest of the loop, including calling pure statements and accessing (Edit: Moved the |
So, running with the wrapped return value proposal, I present a more formal proposal, one that I believe is actually the "perfect" solution Trevor mentions above. The solution currently on master makes the user think about when they need to closure-wrap their loops, and we shouldn't make them have to do that. Take the following contrived example showing all proposed features: a function definition, a
This example should compile to
Remember that this is the most complex example. Simpler function definitions will produce simpler JS output. See the next examples below.
This example should compile to
Features/optimizations that you may have already noticed:
This proposed transformation process is completely transparent to the user, and will provide them with consistent, expected values in the closure-wrapped variables. Some reasoning as to why this is the best choice for semantics, taken from #coffeescript:
PS: For the inconsistency with mutability of the loop variables, see my still-valid solution on #950. |
Render me impressed. Just one question, though: if this is implemented, would not users expect the bodies of I think the consistent way to go about this would be to either make the language fully block-scoped or else leave it function-scoped the way JavaScript is, and introduce a convenient, readable way of introducing a new scope. The latter approach is obviously much easier to implement, and stays closer to the target language. If the generic |
@odf: Can you give me an example of how a user would see different scope-related results when using an |
That's a fair question. Here is some silly example code:
Because of the default function-scoping, the first |
Michael, I'd be OK with an implementation of Going back to Suppose that every loop iteration I get a new
There are three problems here: Non-linearity (defining a function right before the only time it's called), repetition, and having to name a pretty self-explanatory 2-line function. With
Surely this is both more readable and more writeable? |
@TrevorBurnham: A few things:
|
Michael, if you agree that people might have reason to write
then surely you can see why allowing
would be an improvement? Imagine if you had two or three such nested structures! |
Trevor, it's an alright feature, and I'm not vehemently opposed to it, but I do think it's a little unnecessary. I'd lean slightly toward not including it if given the option. But only slightly. I'd like to see some cases where it really cleans up some real-world code. I think it'll be hard to find such an instance. |
The |
I think that Trevor's example is a great place that demonstrates how using more structured code helps avoid having to worry about variable scoping bugs:
|
@jashkenas I think this is a great demonstration of how more 'structured' code can actually be harder to read. The name |
Jeremy, while I appreciate your efforts to promote a clear, consistent coding style, I think that the form you propose—even if better in this case (and me and odf would disagree)—becomes untenable in heavily asynchronous code where there are several layers of nested callbacks. Let me extend my example slightly, by saying that we need to save
By organizing the code in this way, it's not only nicely succinct (which is a great benefit to writeability—I always feel slowed down by having to name functions whose code is self-explanatory), but also makes the scope of everything quite obvious. From the Now here's the "proper" way to write this without
We've doubled the line count, and in my view, we've made the code much more daunting. We've defined things in the opposite order of when they happen, which makes sense when a function is called from several places, but not when it's particular to a single loop. Also, with each level of nesting, we get more and more arguments to each function. I've had functions with 5-6 levels of callback nesting, each adding more variables, which makes for some very messy function declarations. Granted, all the structure added by defining functions outside of the loop makes scope easy to keep track of, but at a very heavy price in all the other virtues that make CoffeeScript such a grand language. In closing, Jeremy, all I want for Christmas is |
Nice example, Trevor! I was talking more in terms of generalities, by the way. How I'd write a piece of particular code depends a lot on the context, so I wouldn't dare to decide what's more readable just based on a tiny snipped. The point was simply that with a small piece of code like that, it is often not the best option to tear it apart artificially. But I think your new example demonstrates that much better. By the way, I've noticed that there appears to be a |
Yes, the proposed |
No, I wasn't proposing to wait for that. More to suggest it as evidence that this construct could be useful, after all. |
Trevor, looks like you get your wish. SHA: 094b876 I'm not entirely happy with the verbosity
And:
... the reason for removing scoped loop literals is that they didn't really cover all of the possible cases, and |
Thanks, Jeremy, I'm glad to hear it. One small tweak suggestion: How about making a special case in the parser to allow
rather than requiring Also, for the record, what cases did the auto-scoped
would continue to work, but it would fail if the loop contained a closure, since |
The cases that the auto-scoped for failed to cover were We may make a special case in the parser later, but for now, let's stick with |
At last I'm starting to use coffeescript (and really like it). As a Rubyist, I was naively surprised that coffeescript does not create a closure on Just add my "+1" for the simpler notation "for... ->". I read the whole thread and understand why the "do" was introduced (good idea), but I would really like an even shorter notation, in particular one that doesn't require me to repeat my parameters... Personally, I would close all my Thanks! Thanks |
…oped magic for once and for all.
When I noticed that the current version of coffeescript doesn't make a new scope for the loop variable, I wanted to make an issue about it, but it looks like this has been discussed to death already. It didn't occur to me that this would alter the behavior of return/break/continue, and that alone is a good reason my intuition is wrong here. I think the real solution, if you want a closure for your loop variables, is to use map or each, which are provided by underscore.js in case your environment doesn't already have them. If you need to exit early (break) you may be better off using detect, or you could use an exception. Also, I was shocked when I found that Python has the same problem here. I thought Python was usually a step above Ruby in terms of technical correctness, but Ruby 1.9 actually does allow loop variables to shadow outer scopes. Then again, Ruby loop bodies really are functions, so perhaps it's not a fair comparison. |
This is an attempt to tie together a few recent issues and come to a consensus on the current feature of closing-over variables in a loop.
The purpose of the feature
Originally proposed in issue 423, the feature is nicely illustrated by this test case:
If converted straightforwardly to JavaScript, this will generate the output
Why? Because the closure doesn't get invoked until after the loop has finished executing, by which point
i
is 2. The closures always references the samei
.But because of this feature, you get the output
Now the closure references the loop's indices (and other variables with "loop scope"—see issue 948) as they are when the closure is defined. This is implemented using a function called from each loop step:
Inconsistencies resulting from this feature
Unfortunately, there are several problems with this implementation, most notably issue 954: The transformation doesn't work if there's a
break
,continue
orreturn
anywhere in the loop. So the codegives you
This is highly unintuitive: Why would the addition of a control statement affect the code's output?
Another inconsistency, issue 950, is that loop indices are immutable when this transformation occurs (that is, when there's a closure and no control statements in the loop), but are mutable otherwise.
Yet another inconsistency, pointed out by satyr at issue 728, is that
arguments
won't work within thefor
loop if the transformation occurs: It will reference thearguments
of_fn
rather than the arguments of whatever function thefor
loop exists in.A more sophisticated implementation could correct these problems, but it would not be simple or elegant, and it's certainly not going to happen in the next few days.
Philosophical problems with this feature
Even if it weren't for the inconsistencies pointed out above, would we want this feature? It's a pretty dramatic transformation on the CoffeeScript compiler's part, and makes an unintuitive leap from JavaScript's semantics.
While it may be the case that you usually want a closure in a
for
loop to reference surrounding variables as they were at the time the closure was declined, this isn't the way that closures behave anywhere else in the language.One of my favorite things about JavaScript is that only functions have scope. It's a nice, easy, consistent rule, one that lets you easily determine scope in CoffeeScript by looking for
->
and=>
(andclass
). There have been several attempts at giving loops special scope in CoffeeScript, but all have proven problematic. I think loops should be kept simple.Alternatives
Let's look at that original example again. How do we say "let's preserve the value of
i
when the closure is invoked"? The simplest approach is to write our own explicit closure,Of course, the syntax here is terrible. There was a better syntax that was proposed at issue 788 and had fairly wide support, not to mention a working implementation courtesy of satyr. If a proposed variation on this syntax were added to master, then you could write
where
do (i) -> ...
is shorthand fordo (i = i) -> ...
, which in turn is shorthand for the current((i) -> ...)(i)
.This, I believe, is the perfect solution: A simple, succinct way of explicitly stating which variables you want to capture. This is a much better fit with CoffeeScript's philosophy of letting you write syntactically elegant code with JavaScript's semantics.
Conclusion
The implicit loop closure that's been in CoffeeScript since issue 423 should be removed due to the unpleasant surprises listed above. Even a perfect implementation would not, in my view, be worth the increased distance between the CoffeeScript and the underlying JavaScript. And the
do
keyword, rejected as insufficiently practical, should be reconsidered as an elegant way of capturing variables.The text was updated successfully, but these errors were encountered: