Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Add "no_implicit_return" option #30

Closed
wants to merge 2 commits into from

9 participants

@omarkhan

I don't like coffeescript's implicit returns and try not to use them. This is a patch that allows coffeelint to detect implicit returns.

@brysgo

Can we have an option to allow implicit returns for one line functions?

@brysgo brysgo commented on the diff
test/test_no_implicit_return.coffee
((30 lines not shown))
+ jump: => @howhigh
+ '''
+
+ 'are allowed by default' : (source) ->
+ errors = coffeelint.lint(source)
+ assert.isArray(errors)
+ assert.isEmpty(errors)
+
+ 'can be forbidden' : (source) ->
+ config = {no_implicit_return : {level:'error'}}
+ errors = coffeelint.lint(source, config)
+ assert.isArray(errors)
+ assert.lengthOf(errors, 3)
+ for error in errors
+ assert.equal(error.message, 'Implicit returns are forbidden')
+ assert.equal(error.rule, 'no_implicit_return')
@brysgo
brysgo added a note

Consider adding this line to your test:

assert.notEqual(error.lineNumber, undefined)

If you add line numbers to your errors it will pass.

@omarkhan
omarkhan added a note

I don't think it's possible to get line numbers from the AST. I would need to cross-reference with the source file. Could be done I suppose, I'll look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/coffeelint.coffee
((7 lines not shown))
# Return the complexity for the benefit of parent nodes.
return complexity
+ # Check for an implicit return, but only if it's not a constructor.
+ lintImplicitReturn : (node) ->
@brysgo
brysgo added a note

The functions here are generally named after the structure they check, rather then the rule they are relevant to.

@omarkhan
omarkhan added a note

I have renamed this to lintFunctionNode in cb9a8ca

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

Sorry it took me so long to get back to you on this. I agree with your comments about line numbers and one-line functions, but the changes are impossible to implement without extending the ASTLinter to look at the source file itself. What do you think the best approach would be?

@brysgo

I don't know enough about the AST but I would say that if you have trouble determining function length have the LexicalLinter record it somehow.

@clutchski
Owner

@omarkhan CoffeeScript is trying to add line number support for source maps, so I think they'll end up on AST nodes. Maybe check in with that effort. That being said, if you want no implicit returns, I'd stay simple and enforce that rule for functions of any length.

@brysgo Thanks for the nice code review in this thread.

@aripalo

So what's the situation with this rule?

I also don't really like the implicit return that coffeescript has... and coffeelint is a perfect tool to force our team's development work to use the same conventions and adding a return statement is easy to forget. CoffeeLint would be a perfect tool for detecting and notifying about implicit returns.

Though I agree it'd be nice to have the possibility for allowing implicit returns for one line functions, but even the plain "boolean rule" of handling all implicit returns would help!

@omarkhan

This patch should work, although you might need to do some merge conflict resolution as it's quite old. There is not yet a straightforward way to detect whether the return is on the same line as the '->', this will be easier when/if coffeescript starts including line numbers in the AST.

@omarkhan

Looks like this might be easier now that we have source locations in the AST: jashkenas/coffeescript@ac9d0e1

I'll give this another look when I get the chance

@aripalo

@omarkhan Very nice of you :)

@AsaAyers
Collaborator

Is there still interest in this issue? It seems like it's probably possible now to identify single line functions. That seems like it would be a critical part of this rule.

@Quazie

I'm very interested in adding this rule to my project if it could exist.

@AsaAyers
Collaborator

I believe everything needed to build this rule is in place, except I have not use for this rule. I'd rather let someone with a use for it build it, I greatly simplified the rule system to help people get started.cyclomatic_complexity.coffee, no_unnecessary_fat_arrows.coffee, and missing_fat_arrows.coffee are our 3 AST based rules that would be most similar to what is needed.

@xixixao xixixao referenced this pull request in jashkenas/coffeescript
Closed

Remove implicit returns? #2477

@JohnTheodore

This rule will definitely be used in our organization. +1

@cspiegl

+1

@AsaAyers
Collaborator

@saifelse has published this as a 3rd party rule. https://github.com/saifelse/coffeelint-no-implicit-returns

I read through the code and it looks good to me :thumbsup:. It doesn't match my style, so I haven't take the time to run it yet.

This is mentioned on coffeelint.org under "Loading Custom Rules", but as a reminder rules can be found at https://www.npmjs.org/search?q=coffeelintrule

@AsaAyers AsaAyers closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 15, 2012
  1. Add no_implicit_return option

    Omar Khan authored
Commits on Aug 7, 2012
  1. Rename lintImplicitReturn -> lintFunctionNode

    Omar Khan authored
This page is out of date. Refresh to see the latest.
View
19 bin/coffeelint
@@ -1,5 +1,5 @@
#!/usr/bin/env node
-// Generated by CoffeeScript 1.3.1
+// Generated by CoffeeScript 1.3.3
/*
CoffeeLint
@@ -13,7 +13,7 @@ CoffeeLint is freely distributable under the MIT license.
var CSVReporter, ErrorReport, Reporter, coffeelint, config, configPath, data, errorReport, findCoffeeScripts, fs, glob, lintFiles, lintSource, optimist, options, path, paths, read, reportAndExit, scripts, stdin, thisdir,
__slice = [].slice,
__hasProp = {}.hasOwnProperty,
- __extends = function(child, parent) { for (var key in parent) { if (__hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor; child.__super__ = parent.prototype; return child; };
+ __extends = function(child, parent) { for (var key in parent) { if (__hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; };
path = require("path");
@@ -49,14 +49,11 @@ CoffeeLint is freely distributable under the MIT license.
ErrorReport = (function() {
- ErrorReport.name = 'ErrorReport';
-
function ErrorReport() {
this.paths = {};
}
ErrorReport.prototype.getExitCode = function() {
- var path;
for (path in this.paths) {
if (this.pathHasError(path)) {
return 1;
@@ -66,7 +63,7 @@ CoffeeLint is freely distributable under the MIT license.
};
ErrorReport.prototype.getSummary = function() {
- var error, errorCount, errors, path, pathCount, warningCount, _i, _len, _ref;
+ var error, errorCount, errors, pathCount, warningCount, _i, _len, _ref;
pathCount = errorCount = warningCount = 0;
_ref = this.paths;
for (path in _ref) {
@@ -119,8 +116,6 @@ CoffeeLint is freely distributable under the MIT license.
Reporter = (function() {
- Reporter.name = 'Reporter';
-
function Reporter(errorReport) {
this.errorReport = errorReport;
this.ok = '';
@@ -143,7 +138,7 @@ CoffeeLint is freely distributable under the MIT license.
};
Reporter.prototype.publish = function() {
- var errors, path, summary, _ref;
+ var errors, summary, _ref;
this.print("");
_ref = this.errorReport.paths;
for (path in _ref) {
@@ -206,14 +201,12 @@ CoffeeLint is freely distributable under the MIT license.
__extends(CSVReporter, _super);
- CSVReporter.name = 'CSVReporter';
-
function CSVReporter() {
return CSVReporter.__super__.constructor.apply(this, arguments);
}
CSVReporter.prototype.publish = function() {
- var e, errors, f, path, _ref, _results;
+ var e, errors, f, _ref, _results;
_ref = this.errorReport.paths;
_results = [];
for (path in _ref) {
@@ -237,7 +230,7 @@ CoffeeLint is freely distributable under the MIT license.
})(Reporter);
lintFiles = function(paths, config) {
- var errorReport, path, source, _i, _len;
+ var errorReport, source, _i, _len;
errorReport = new ErrorReport();
for (_i = 0, _len = paths.length; _i < _len; _i++) {
path = paths[_i];
View
39 lib/coffeelint.js
@@ -1,4 +1,4 @@
-// Generated by CoffeeScript 1.3.1
+// Generated by CoffeeScript 1.3.3
/*
CoffeeLint
@@ -91,6 +91,10 @@ CoffeeLint is freely distributable under the MIT license.
space_operators: {
level: IGNORE,
message: 'Operators must be spaced properly'
+ },
+ no_implicit_return: {
+ level: IGNORE,
+ message: 'Implicit returns are forbidden'
}
};
@@ -137,8 +141,6 @@ CoffeeLint is freely distributable under the MIT license.
LineLinter = (function() {
- LineLinter.name = 'LineLinter';
-
function LineLinter(source, config, tokensByLine) {
this.source = source;
this.config = config;
@@ -264,8 +266,6 @@ CoffeeLint is freely distributable under the MIT license.
LexicalLinter = (function() {
- LexicalLinter.name = 'LexicalLinter';
-
function LexicalLinter(source, config) {
this.source = source;
this.tokens = CoffeeScript.tokens(source);
@@ -294,9 +294,9 @@ CoffeeLint is freely distributable under the MIT license.
};
LexicalLinter.prototype.lintToken = function(token) {
- var lineNumber, type, value, _base;
+ var lineNumber, type, value, _base, _ref;
type = token[0], value = token[1], lineNumber = token[2];
- if ((_base = this.tokensByLine)[lineNumber] == null) {
+ if ((_ref = (_base = this.tokensByLine)[lineNumber]) == null) {
_base[lineNumber] = [];
}
this.tokensByLine[lineNumber].push(token);
@@ -572,8 +572,6 @@ CoffeeLint is freely distributable under the MIT license.
ASTLinter = (function() {
- ASTLinter.name = 'ASTLinter';
-
function ASTLinter(source, config) {
this.source = source;
this.config = config;
@@ -610,9 +608,32 @@ CoffeeLint is freely distributable under the MIT license.
this.errors.push(error);
}
}
+ if (name === 'Assign' && node.value.constructor.name === 'Code') {
+ this.lintImplicitReturn(node);
+ }
return complexity;
};
+ ASTLinter.prototype.lintImplicitReturn = function(node) {
+ var attrs, error, func, funcname, rule, _ref, _ref1;
+ rule = this.config.no_implicit_return;
+ funcname = (_ref = node.variable) != null ? (_ref1 = _ref.base) != null ? _ref1.value : void 0 : void 0;
+ if (node.context === 'object' && funcname === 'constructor') {
+ return;
+ }
+ func = node.value.body.expressions;
+ if (func.length && !(func[func.length - 1].expression != null)) {
+ attrs = {
+ context: funcname,
+ level: rule.level
+ };
+ error = createError('no_implicit_return', attrs);
+ if (error) {
+ return this.errors.push(error);
+ }
+ }
+ };
+
return ASTLinter;
})();
View
22 src/coffeelint.coffee
@@ -95,6 +95,10 @@ RULES =
level : IGNORE
message : 'Operators must be spaced properly'
+ no_implicit_return :
+ level : IGNORE
+ message : 'Implicit returns are forbidden'
+
# Some repeatedly used regular expressions.
regexes =
@@ -528,9 +532,27 @@ class ASTLinter
error = createError 'cyclomatic_complexity', attrs
@errors.push error if error
+ if name == 'Assign' and node.value.constructor.name == 'Code'
+ @lintFunctionNode(node)
+
# Return the complexity for the benefit of parent nodes.
return complexity
+ # Check for an implicit return, but only if it's not a constructor.
+ lintFunctionNode : (node) ->
+ rule = @config.no_implicit_return
+ funcname = node.variable?.base?.value
+ if node.context == 'object' and funcname == 'constructor'
+ return
+ func = node.value.body.expressions
+ if func.length and not func[func.length - 1].expression?
+ attrs = {
+ context: funcname
+ level: rule.level
+ }
+ error = createError 'no_implicit_return', attrs
+ @errors.push error if error
+
# Merge default and user configuration.
mergeDefaultConfig = (userConfig) ->
View
50 test/test_no_implicit_return.coffee
@@ -0,0 +1,50 @@
+path = require 'path'
+vows = require 'vows'
+assert = require 'assert'
+coffeelint = require path.join('..', 'lib', 'coffeelint')
+
+vows.describe('return').addBatch({
+
+ 'Implicit returns' :
+
+ topic : () ->
+ '''
+ double = (x) ->
+ x*2
+
+ triple = (x) ->
+ return x*3
+
+ noop = ->
+
+ complex = (x) ->
+ x += 2
+ x -= 5
+ x = x * 4
+ x / 2
+
+ class Cat
+ constructor: ->
+ @howhigh = 5
+ roar: -> return 'meow'
+ jump: => @howhigh
+ '''
+
+ 'are allowed by default' : (source) ->
+ errors = coffeelint.lint(source)
+ assert.isArray(errors)
+ assert.isEmpty(errors)
+
+ 'can be forbidden' : (source) ->
+ config = {no_implicit_return : {level:'error'}}
+ errors = coffeelint.lint(source, config)
+ assert.isArray(errors)
+ assert.lengthOf(errors, 3)
+ for error in errors
+ assert.equal(error.message, 'Implicit returns are forbidden')
+ assert.equal(error.rule, 'no_implicit_return')
@brysgo
brysgo added a note

Consider adding this line to your test:

assert.notEqual(error.lineNumber, undefined)

If you add line numbers to your errors it will pass.

@omarkhan
omarkhan added a note

I don't think it's possible to get line numbers from the AST. I would need to cross-reference with the source file. Could be done I suppose, I'll look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ assert.deepEqual(errors.map((e) -> e.context),
+ ['double', 'complex', 'jump'])
+
+}).export(module)
Something went wrong with that request. Please try again.