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

inner scopes don't shadow variables; large potential for bugs #238

Closed
onnlucky opened this Issue Mar 8, 2010 · 12 comments

Comments

Projects
None yet
6 participants
@onnlucky
Copy link

onnlucky commented Mar 8, 2010

There is a slight conflict when there is no explicit variable introduction and variables are mutable (in any language). You might accidently re-use a more outer variable in a inner function, while you wanted to shadow it instead.

trivial example:

foo: "hello"

testing: ->
  foo: "unrelated"
  alert foo

testing()
alert foo

This example is trivial; but in the real world this happens easily with only local meaningful variables like an n, or prev or what have you ...

The trouble is: foo: "hello" stands for assign "hello" to foo variable and introduce foo as a variable (in the current scope) if it doesn't exist already.

But there are two distict uses: (1) you need a new (local) variable; (2) you want to assign a new value to a (more) global variable. Trouble is with (1): you need to keep in your head a list of all more global variables.


Using var x: "value" prevents this. And you could make the var optional or use something like local to make this explicit. Or you can use x: "value" as introduction and assignment, and use x = "newvalue" as variable change (but not introduction). Or just document the behavior and its shortcomings clearly and learn to live with them. (Or you can do what python does, if the first reference is a read, it is a more global variable, otherwise it is a local variable; but please don't.)

I'm willing to bet this will cause someone to waste a day of debugging, which turns out to be a unexpected sharing of a more outer variable.

@fizx

This comment has been minimized.

Copy link

fizx commented Mar 8, 2010

+1 for local.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Mar 8, 2010

Thanks for the excellent ticket -- hopefully we'll get some discussion. The current behavior of CoffeeScript is by design, it's working the same way as it does in Ruby (and it seems, according to your explanation, in Python as well) -- with a slight JavaScript twist in that declaration always happens before evaluation. Programmers in those languages get by just fine without var.

It's certainly possible to accidentally blow away an outer variable within an inner scope, but (in Ruby and Python), this turns out not to be a very common problem. If you don't nest functions too deeply, don't introduce a lot of globals, and give things proper names, it never arises.

In JavaScript, having var is even more bug-prone. You need to use it once, and only once at the desired level. If you forget it you get an accidental global variable, and it can't be used as part of a larger expression.

So, I'd like to make the argument that having named variables observe consistent lexical scope -- you can be sure that you're dealing with the same variable in inner functions, and not something unrelated, is a more pleasant experience. Allowing var makes all variable names like chameleons, where you have to backtrack and look around for the declaration to be sure which value you're dealing with (and which you would be dealing with if the var was omitted). Better to use a different name, if it's really a different variable.

@onnlucky

This comment has been minimized.

Copy link
Author

onnlucky commented Mar 8, 2010

does in Ruby (and it seems, according to your explanation, in Python as well)

no and no. ruby has:

  • @@static (class) variable
  • @instance variable
  • afaik the rest is same as in coffee-script but I could be mistaken

Ruby advantage is that you don't need that many local variables; due to class and instance variables.

python is even different:

def myfun():
  variable = "foo" # always local

def myfun():
  print(variable) # always global
  variable = "foo" # changed the global variable

Which is weird, but then python doesn't have lexical scoping at all. So don't look at python.

In JavaScript, having var is even more bug-prone.

I disagree. Doing it once is quite natural, fresh variable? use var. Accidentally forgetting about it is a big deal and a major design flaw in javascript; if the global scope were sealed (you can do that in js5!) that would fix everything, except when you wanted to shadow a outer variable, but forgot the var ... silly mistake.

(As appossed to the coffee-script flowchart: fresh variable? ensure your name is unique in all enclosing scopes.)


Your final argument is a matter of taste. But inside inner scopes, the more-local variables are, the more relevant to the code they are. I would argue it is easier to keep track of the more-local variables, as apposed to all the more-global variables. And any meaningful global variables you would give meaning full names.

E.g.

old: currentNode
currentNode = newNode

... complex code ... that might call helperFunction() ...

if old ->
  old.parent = null
  if focused == old -> focused = getnewfocus()

helperFunction: (...) ->
   helperCallbackFunction: ->
     old: getCurrentCallback()
     setNewCallback(...)
     if old -> old.call()
     ...

So how do I know not to use 'old' as tmp variable name?


However; it looks much better (much more declarative) if you don't have to write var everytime.

@grayrest

This comment has been minimized.

Copy link
Contributor

grayrest commented Mar 8, 2010

Python also has the global keyword which allows you to override the default local scope. It's just not used very often. Doing something similar in coffeescript wouldn't be that great because closures are such a common pattern in javascript.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Mar 8, 2010

grayrest -- To create globals in CoffeeScript on purpose, simply attach your variable to the global object (window in the browser).

As for Ruby's variable options -- @@static variables are rarely used, you usually want an instance variable on the class instead. Instance variables are great -- but they in no way diminish the use of local variables. CoffeeScript has instance variables (properties on the current object) that can be used anywhere you would use them in Ruby. They even have the same @variable syntax, if you like. So the behavior and patterns of use can be largely the same between the languages.

Yes, you need to ensure that your name is unique to the enclosing scope, but you should be doing that in any case. Scopes should be short and sweet, and not nested deeply. CoffeeScript is making it easier to write well structured code (by omitting var), not trying to make it easier to make a tangled mess of things, with multiple shadowed variables with the same name in nested inner functions. You say that global variables should have meaningful names, and that's fine -- If you follow that advice, you'll never have this issue in the first place.

This is really the same issue as the nested for loop. In JavaScript:

for (var i = 0; i < list.length; i++) {
  .... more code here ....
    for (var i = 0; i < array.length; i++ {
       ... boom, sharing a variable ...
    }
}

var is not going to help you here. The solution is not to say that loops should behave as functions and create a new scope, but to use a different variable name, if that's what you mean.

@StanAngeloff

This comment has been minimized.

Copy link
Contributor

StanAngeloff commented Mar 8, 2010

The way Coffee scopes variables now is working out just great. I have not had any issues with it so far. Being able to access outer variables from inner scopes is useful at times:

JOB_ID: 1

nextId: ->
    JOB_ID: JOB_ID + 1
    JOB_ID

In this case, if Coffee defines a local variable to nextId this would break the script. I agreed with Jeremy:

Better to use a different name, if it's really a different variable.

@onnlucky

This comment has been minimized.

Copy link
Author

onnlucky commented Mar 8, 2010

You say that global variables should have meaningful names, and that's fine -- If you follow that advice, you'll never have this issue in the first place.

Its not global variables that are the problem; its the local variable that is accidentally used in an outer scope too, creating havoc.

As said, the way it currently works is a choice, and it is not even a bad choice per-se. It would not be my choice, but that is besides the point. But it is a choice that you would want to explicitly document as a potential pitfall.


This is really the same issue as the nested for loop. In JavaScript
... snip ...
The solution is not to say that loops should behave as functions and create a new scope, but to use a different variable name, if that's what you mean.

Probably; unfortunately any trivial example will resort to bad-style/good-style debate. Which is not the debate at all. Quite sure there can be code that must be considered very good style, but still runs into this accidental variable sharing problem.

To create globals in CoffeeScript on purpose, simply attach your variable to the global object

Also for completeness; indeed that would create a true global. But for static scopes, any variable defined in a more outer scope is 'global' (also called free). What we used to call global variables come from languages that cannot do closures (like c or java). But talking about those does not really contribute to the issue at hand.

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Mar 8, 2010

Great. We're agreed then. I'll update the documentation section that talks about Assignment & Scope to mention this potential pitfall this evening. Thanks for bringing it up.

@onnlucky

This comment has been minimized.

Copy link
Author

onnlucky commented Mar 8, 2010

@StanAngeloff

Nothing would change in how your example works.

I agreed with Jeremy:

Better to use a different name, if it's really a different variable

That is the problem, what if you think you are using a different variable, but you accidentally did not and change state somewhere where you didn't intend to do so?

If you want a new variable; you can mark that using var or local; then you are sure.

(And if coffee-script would add this, any coffee-script-lint would immediately warn on any variable introduction that skips the var.)

@onnlucky

This comment has been minimized.

Copy link
Author

onnlucky commented Mar 8, 2010

Great. We're agreed then. I'll update the documentation section

Cool :)

I think coffee-script is an awesome project, very pragmatic syntax choices. Great inspiration. Keep up the good work!

@jashkenas

This comment has been minimized.

Copy link
Owner

jashkenas commented Mar 9, 2010

Docs added, closing the ticket -- and it's good to have this on record, so it can be referred to in the future.

@zaach

This comment has been minimized.

Copy link
Contributor

zaach commented Mar 13, 2010

FWIW, JavaScript 1.7 (Mozilla extensions) introduced let variable definitions for this purpose. let is also a ECMAScript Harmony proposal.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.