Warn about undefined variables #20

Open
epeli opened this Issue Apr 6, 2012 · 62 comments

Projects

None yet
@epeli
myfun = ->
  foo = bar

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

@clutchski
Owner

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

@epeli

@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 👍 so I can get notified on updates of this issue...

@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
Collaborator

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 992d4c8

@AsaAyers
Collaborator

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
Collaborator

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
Collaborator

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
Owner

@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
Collaborator

👍 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
Collaborator

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
Collaborator

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
Collaborator

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

@AsaAyers
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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 pwlmaciejewski/coffeelint-variable-scope#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
Collaborator

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
Collaborator
@ismell

I want to try it out as well :)

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

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

@swang
Collaborator

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
Collaborator

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

👍

@tomhicks-bsf

👍 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.

@cjbrowne

👍

@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
Collaborator

@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

👍

@austin1237

Is this issue getting worked on at this time?

@AsaAyers
Collaborator

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?

@AsaAyers
Collaborator

No, it means I've abandoned the CoffeeScope project and without that we're never going to correctly resolve undefined/unused variables.

I recommend moving on from CoffeeScript and use Babel/ES6 if you have a choice. There are no meaningful advantages to CoffeeScript any more and I won't start new projects with it.

@Zolmeister

@AsaAyers On that we disagree. I believe code readability is one of the most valuable features of a language, and find whitespace sensitivity and other CoffeeScript syntax features (english comparators, the '?', object notation shorthand, etc.) to add significant value. (admittedly I'm also a fan of python)

@AsaAyers
Collaborator

The fact is the internals of CoffeeScript are garbage and have prevented me from completing this issue. I see no reason to continue using it and no one has stepped up to take over maintenance of CoffeeLint. I still stick around to manage PRs, but I generally don't work on this project any more.

This is my favorite test to show people. Which of these are valid?

if foo and
 bar # 1 space
  undefined # 2 spaces

if foo and
  bar # 1 space
    undefined # 4 spaces

if foo and
   bar # 3 space
  undefined # 2 spaces

if foo and
    bar # 4 space
  undefined # 2 spaces

It's a trick question because they all compile. with CoffeeScript, although none compile with Redux. Whitespace for indentation is only good if there is such a thing as being strictly "correct" as I'm sure it is in python. Jeremy Ashkenas seems very proud of his hacks to get CoffeeScript to compile, but the fact is those shortcuts are what prevents us from ever becoming as good as eslint. I'd rather use a language that has all the tools I need instead of a hacked together language whose internals are such garbage we can't build sufficient tools.

@AsaAyers
Collaborator
@nickdima

For those of you using Atom you can try this package: atom-linter-coffee-variables.
It uses ESLint in the background so it's kind of a hack but could be better then nothing.

@wwwdata

@AsaAyers so the recommended way is to switch to ES6/2015 or use the workflow with coffee-jshint that @jonahkagan described, right?

@AsaAyers
Collaborator

I really can't recommend coffee-jshint. At that point you're not actually linting your code, your'e linting the code generated by CoffeeScript that you have very little control over. It will spot undefined or unused variables, but I can't see how it would be useful for anything else.

CoffeeScript just doesn't have a future, so IMO it's better to plan your exit sooner rather than later. A year ago my company decided to switch from Backbone to React. Because JSX support in CoffeeScript is terrible we've also migrated to ES6. Our last non-React release was 15,980 lines of CoffeeScript over 175 files, now we have 1,826 lines left in 11 files. (side note: Switching to React has brought the same codebase down to 3104 lines over 49 files)

I found the best way to convert is to just copy your code into a .js file and begin fixing syntax errors found with eslint. CoffeeScript is so close to ES6 that it's sometimes hard to tell the difference between the two in small sections of code.

@wwwdata

big thanks @AsaAyers for your quick reply and the stats. I really like the way you organise your components with React too. Currently we have a large Angular/CoffeeScript Codebase, but for the future we will have to migrate this.

@eventualbuddha eventualbuddha referenced this issue in decaffeinate/decaffeinate May 3, 2016
Closed

Update README.md #188

@za-creature

If anyone's still interested, my plugin handles undefined vars and more

Ref @wwwdata

@davibe

amazing. Thank you @za-creature

@astorije

Is this still considered, by any chance?
When using coffeelint in a text editor package (such as https://atom.io/packages/linter-coffeelint), adding plugins is not trivial.

@AsaAyers
Collaborator

You just have to make your plugin a local devDependency of the project. linter-coffeelint doesn't run in Node, so it doesn't have access to globally installed node modules. CoffeeLint should be a devDependency too. If it is, then linter-coffeelint and the coffeelint command line with use your local version

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