coffeelint
coffeelint copied to clipboard
clutchski
Warn about undefined variables
myfun = ->
foo = bar
It should warn about the undefined bar variable. This something I miss the most from jshint/jslint.
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
@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.
You can use https://github.com/Clever/coffee-jshint to detect undefined variables in the meantime.
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.
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
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.
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?
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.
Sure, either way I'd love to see this supported because of its potential to catch runtime errors.
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
###
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.
@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.
: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.
@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
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.
@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?
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.
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.
I think I have destructuring worked out. I think it's also a much cleaner approach than my lintAssign mess.
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
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.
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()
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?
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.
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.
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.
Yeah, I'm not proposing running a fork. Just wanted to put that out there in case it set off any ideas.
@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.
Oh, also added destructuring support with for statements i.e. for { a, b } in list.
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.
npm install "git://github.com/clutchski/coffeelint.git#undefined-rule"
@eventualbuddha Do you still have unpushed changes for the catch variable?
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.
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.
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.
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.
@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
Yes, this very minute in fact :) It's part of a larger project taking place at #415
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.
@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)
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.
When I compare ES5 and CoffeeScript, CoffeeScript is a clear winner on number of useful features. When I compare ES6 I think I'm losing 2-3 features I use and gaining far superior tooling.
I should clarify. CoffeeScript's internals are good at churning out ES5 code, but the intermediate representations we need to use are too full of hacks to provide proper tooling around the language. On Jul 25, 2015 4:26 PM, "Zoli Kahan" [email protected] wrote:
@AsaAyers https://github.com/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)
— Reply to this email directly or view it on GitHub https://github.com/clutchski/coffeelint/issues/20#issuecomment-124886426 .
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.
@AsaAyers so the recommended way is to switch to ES6/2015 or use the workflow with coffee-jshint that @jonahkagan described, right?
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.
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.
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.
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
FYI @za-creature's coffeescope2 plugin has been working well for me…
@clutchski @swang @za-creature Would there be any interest in merging it into this project?
