Warn about undefined variables

[esamattis]

myfun = ->
  foo = bar

It should warn about the undefined bar variable. This something I miss the most from jshint/jslint.

[clutchski]

Yeah, this is a nice feature. I'll put it on the list.

[juanique]

While this would be an amazing feature... is there a good way to detect this in coffeescript?.

If the sample code was from a website, bar could be defined by another script just by assigning it to a property of the window object: window.bar = 1

[esamattis]

@juanique that kind of global usage is also something coffeelint should warn about ;)

In jshint/jslint you have to manually configure what globals you are going to use, such as the jQuery dollar sign.

[jonahkagan]

You can use https://github.com/Clever/coffee-jshint to detect undefined variables in the meantime.

[thom-nic]

Adding my :+1: so I can get notified on updates of this issue...

[janraasch]

+1

[thom-nic]

I think (but I'm not sure) that this plugin may meet this use case? https://github.com/fragphace/coffeelint-variable-scope

Eager to try it myself but thought I'd let other folks know in the meantime.

[AsaAyers]

That plugin does something different. It's for warning you if you create a variable in an outer scope, for example:

path = require 'path'

and then you reassign it in a different scope

for path in paths
    doSomethingWith path

For a more complete example see https://github.com/clutchski/coffeelint/commit/992d4c84eee4a7cd34cda1ddb26faa50c5f2831a

[AsaAyers]

I started a branch a while back implement a rule to warn about undefined and unused variables. If someone would like to help out what I have is available at https://github.com/clutchski/coffeelint/commits/undefined-rule

It's been a couple months since I had time to work on it. It was close to working, just some edge cases where it would fail, I think around destructuring assignments.

[thom-nic]

Ah right I think I confused myself because they both happen to be cases that interest me.

Would the "undefined rule" have a better chance of success as a plugin rather than a rule in coffeelint core?

[AsaAyers]

The internal structure uses the same plugin system, so the only difference is whether or not it ships with CoffeeLint. I think there's enough interest and it's useful to everyone, so it should go in the core. I can't think of any project where you'd rather find out about undefined variables at run time. I probably won't turn it on by default though, just because when you do have global variables you have to declare them.

###
# global someGlobalVariable
###

They have to be declared with a block comment because the other type doesn't show up in the AST.

[thom-nic]

Sure, either way I'd love to see this supported because of its potential to catch runtime errors.

[fahad19]

would really love this to be supported.

[AsaAyers]

Would anyone like to weigh in on whether or not this should be on by default? I just checked and it's on by default for JSHint. My thought is that it catches run-time errors. The only reason i can think of to turn this off is if you aren't willing to document your global variables like this:

###
# global MyGlobalVariable
###

[thom-nic]

Because of its high potential to catch runtime errors, I'd say on by default, it can easily be turned off. There could also be a "browser mode" and "node mode" where you have a list of predefined globals to automatically ignore things like window or module, process etc.

[clutchski]

@AsaAyers Great feature and great you're working on it. One thought is to maybe keep a list of global variables in the undefined-variable section of the config file, rather than the source file itself. The reasons is that a web project could be split across 100 files all using jquery and a user could add $ in one place, rather than 100.

[AsaAyers]

:thumbsup: I'll make sure to add that too. I'm also using one of jshint's files for declaring environments:

{
    "undefined_variables": {
        "environments": {
            "browser": true,
            "jQuery": true
        }
    }
}

I need to add something for custom globals though.

The biggest challenge in working on this is inspecting the AST, so yesterday I decided to build a tool for that (http://asaayers.github.io/clfiddle/). It also shows the token stream and has a configuration page for CoffeeLint. I think this'll be useful for anyone who wants to write or edit rules.

[eventualbuddha]

@AsaAyers Any update? Is this on a branch you have publicly available somewhere? I really want this for our apps. I'd love to help push this over the finish line.

cc @randometc

[AsaAyers]

I just (force) pushed what I have up to https://github.com/clutchski/coffeelint/tree/undefined-rule. I was using it for a while, but there are too many false positives.

• Returning a variable is not counted toward "using" it. My modules often define a class and it is implicitly returned (RequireJS). These are all considered unused variables because they have a name.
• Variables defined through a destructuring assignment are often missed, and will be flagged as undefined when they are first used.
• I'm also not happy with my attempt at a test suite for this. I had a couple big topics and last time I worked on this started breaking them into smaller pieces.
• Using variables to construct an array or object often doesn't increment it's use counter.

v1 = "some value" # Unused v1
v2 = "some value" # Unused v2

returnValue = [ v1, v2 ]  # Unused returnValue

return returnValue

Thank you for any help you're able to provide.

[eventualbuddha]

@AsaAyers I pushed up some changes to that branch. Feel free to edit. It's still definitely a work in progress. So far it seems pretty good. What do you think of the approach of just looking at Value and Assign nodes? Had you already tried that and found a problem with it?

[AsaAyers]

I have a lintAssign, but I think that isn't used in destructuring assignments or function parameters. There might be a good way to generically handle Values, but I know they also hold literal values. Thanks for the help. I hadn't noticed the Existence node.

[eventualbuddha]

I dug into it a little more. It definitely is more tricky than simply looking at Assign and Value nodes, since Value nodes are used for object literal keys and some other places I didn't initially expect. I'll keep plugging away at it.

[AsaAyers]

I think I have destructuring worked out. I think it's also a much cleaner approach than my lintAssign mess.

[AsaAyers]

I pushed several more commits. I started running it against a large private project I work on. The only false positive I found was around catch e, I don't see how e gets defined as a variable.

I think I also need to figure out something with function parameters. There are some times where I want to keep the unused parameters as documentation. For Javascript one work around is to comment out the unused parameters, but this can't be done with CoffeeScript.

function (jqXHR /* , textStatus, errorThrown */) {
}

 (jqXHR, textStatus, errorThrown) ->
    # CoffeeScript forbids void, so this isn't an option
    #void errorThrown

    # You can simply place it on it's own line, it becomes a useless statement. 
    # Probably not ideal.
    errorThrown
    undefined

[eventualbuddha]

Yeah, I ran into a few of those in my project as well. I also had the same issue with list comprehensions where I only wanted the value:

value for own key, value of obj

It will flag key as unused. I've seen in some languages/tools that if you use _ as a variable name then it effectively means "I know I need an identifier here but I don't care about the result". Unfortunately javascript also has the underscore library which does not share that meaning, obviously.

[AsaAyers]

When creating a new variable we can pass in dependsOn. I'm using that in lintCode(). In my example above jqXHR dependsOn textStatus which dependsOn errorThrown. If errorThrown is used, then it will consider textStatus and jqXHR to have been used too. I bet the for own is throwing it off because I see a dependsOn in lintFor()

[eventualbuddha]

I noticed another issue. The lintComment method is supposed to allow you to say # global jQuery or something like that, but CoffeeScript's AST does not contain comments. Do you have a proposal for fixing that, @AsaAyers?

[AsaAyers]

The AST only keeps things that matter for generating Javascript, so you have to use block comments. This also means your generated Javascript will have that comment in place. I already know people will complain about that, and I just don't think it matters.

###
# global jQuery
###

I did wonder about a work around, but I think it sounds like a mess to regex over the original text collecting line numbers and attempting to patch them into the AST processing.

[eventualbuddha]

I have a fork of coffee-script that adds comments to the AST as a top-level comments array. You'd still have to figure out where they were when you're processing the AST nodes, but it's a more tractable problem.

[AsaAyers]

CoffeeLint is also setup to run in the browser, so I'm not ok with running a fork of CoffeeScript. It's hard enough already to keep up, the latest 1.7 update came out of nowhere. I read that it was getting close to release.... a few hours after it released.

I know I said it sounds like a mess, but it might be worth coming up with a way to scan the file once for comments and provide them in the ASTApi and maybe the TokenAPI. This same issue came up in a 3rd party plugin https://github.com/fragphace/coffeelint-variable-scope/issues/2

Eventually I need to start doing research on making the jump to redux. I don't even know how that's going to work yet.

[eventualbuddha]

Yeah, I'm not proposing running a fork. Just wanted to put that out there in case it set off any ideas.

[eventualbuddha]

@AsaAyers I updated the branch to work with variables declared by try/catch statements. I had forgotten that in CoffeeScript catch variables are not block scope like they are in JS:

try
catch ex

Becomes this:

var ex;

try {

} catch (_error) {
  ex = _error;
}

Which is convenient because it means we don't have to try to support block scoping.

[eventualbuddha]

Oh, also added destructuring support with for statements i.e. for { a, b } in list.

[AsaAyers]

Am I missing something or did you forget to push your changes? I'm going to start running this branch again to see if my every day work reveals any more issues.

[AsaAyers]

@eventualbuddha ping?

[ismell]

I want to try it out as well :)

[AsaAyers]

npm install "git://github.com/clutchski/coffeelint.git#undefined-rule"

@eventualbuddha Do you still have unpushed changes for the catch variable?

[swang]

This is now the most commented on and oldest issue (2+ years old) for CoffeeLint. We should just figure out how far along this is and get it pushed out.

[AsaAyers]

I agree, I've been considering just pushing what I have into 1.6. The only thing I can think of that I know wasn't working was catch error. There was something strange there and I had trouble figuring out how to capture it for some reason.

[Zolmeister]

:+1:

[tomhicks-bsf]

:+1: to get updates on this.

[eventualbuddha]

Here's my branch with my changes. It's probably not working in the current state. I went back and looked at my repo, realized I had some changes that had not yet been committed, and just committed them as a WIP. They may be garbage. Feel free to take from that branch whatever is useful, if anything.

[hyperfocusaurus]

:+1:

[bilalq]

Hey, would you consider breaking this out into a standalone plugin (even if it's still in an unstable/WIP state) or perhaps just rebasing against the current master HEAD? I'd love to include this, but I do want to stay on the latest coffeelint release.

Also, thanks for the work on this. This would definitely be an amazing feature to have.

[Zolmeister]

Any updates on this?

[AsaAyers]

@bilalq is right. The right solution here is for me to publish it as a 3rd party rule. This also means it's more available for people to issue pull requests if you're interested. I know it's incomplete, but it's been long enough since I really sat down to work on it that I don't remember the details. I took what I have and dumped it into a new rule here:

https://www.npmjs.com/package/coffeelint-undefined-variables

[ethanmick]

:+1:

[austin1237]

Is this issue getting worked on at this time?

[AsaAyers]

Yes, this very minute in fact :) It's part of a larger project taking place at #415

[sontek]

Looks like #415 got closed, does that mean this is resolved?