diff --git a/lib/rules/require-var-decl-first.js b/lib/rules/require-var-decl-first.js index ad8ff86d4..c2e542cf3 100644 --- a/lib/rules/require-var-decl-first.js +++ b/lib/rules/require-var-decl-first.js @@ -108,111 +108,44 @@ var assert = require('assert'); -function getVariableScope(node) { - while (node.type !== 'Program' && - node.type !== 'FunctionDeclaration' && - node.type !== 'FunctionExpression') { - node = node.parentElement; - } - - return node; -} - -function getOffsetForBlockStatement(enclosingScope, varDecl, commentTokens, file) { - var offset = 0; - var parentElement = varDecl.parentElement; - if (enclosingScope.type !== 'Program' && parentElement.type === 'BlockStatement') { - offset += 1; - offset += getCommentOffsetBetweenNodes(parentElement, varDecl, commentTokens, file); - } - return offset; -} - -function getUseStrictDeclFirst(enclosingScope) { - var firstNode; - if (enclosingScope.type === 'Program') { - firstNode = enclosingScope.body[0]; - } else { - firstNode = enclosingScope.body.body[0]; - } - - if (firstNode.type === 'ExpressionStatement' && - firstNode.hasOwnProperty('expression') === true && - firstNode.expression.hasOwnProperty('value') === true && - firstNode.expression.value === 'use strict') { - return firstNode; - } - - return null; -} - -function isFirstVarDeclInScope(enclosingScope, varDecl, whitespaceOffsetBeforeVarDecl, commentTokens, file) { - var adjustedVarDeclStart = varDecl.range[0]; - var adjustedScopeStart = enclosingScope.range[0]; - - if (enclosingScope.type !== 'Program') { - // For function declaration and function expression scope use the top block statement as start - // This removes the requirement to offset the function declaration or expression related tokens - adjustedScopeStart = enclosingScope.body.range[0]; - // If enclosing scope node type is Program the range start ignores all comments and whitespace before the - // variable declaration - adjustedVarDeclStart -= whitespaceOffsetBeforeVarDecl; - } - - adjustedVarDeclStart -= getOffsetForBlockStatement(enclosingScope, varDecl, commentTokens, file); - - if (adjustedVarDeclStart === adjustedScopeStart) { - return true; - } - - return false; +function isScopeElement(elem) { + return elem.type === 'Program' || + elem.type === 'BlockStatement' && ( + elem.parentElement.type === 'FunctionExpression' || + elem.parentElement.type === 'FunctionDeclaration' || + elem.parentElement.type === 'ArrowFunctionExpression' + ); } -function getCommentOffsetBetweenNodes(previousNode, currentNode, commentTokens, file) { - var count; - var comment; - var commentLength = 0; - - for (count = 0; count < commentTokens.length; count++) { - comment = commentTokens[count]; - if (comment.range[0] >= currentNode.range[1]) { - // Stop processing comments that are occurred after current node - break; - } - - if (previousNode.range[1] < currentNode.range[0] && - comment.range[0] > previousNode.range[0] && - comment.range[1] < previousNode.range[1]) { - // Stop processing comments that are within multiple declarators in a single variable declaration - // of the previous node and the previousNode is not the parent of currentNode - break; - } +/** + * Checks for allowed elements before variable declaration + * @param {Object} elem + * @returns {Boolean} + */ +function isRelevantElement(elem) { + // Allow comments and whitespaces. + var itIs = elem.isComment || elem.isWhitespace; - if (comment.range[0] > currentNode.range[0] && - comment.range[1] < currentNode.range[1]) { - // Stop processing comments that are within multiple declarators in a single variable declaration - break; - } + // Variable declaration itself. + itIs = itIs || elem.type === 'VariableDeclaration'; - if (previousNode.range[0] >= comment.range[1]) { - // Skip comments that occurred before the previous node - continue; - } + // For '{' in `function() { var a; }`. + itIs = itIs || elem.type === 'Punctuator'; - commentLength += comment.range[1] - comment.range[0] + file.getWhitespaceBefore(comment).length; - } + // Skip 'use strict' statements at the beginning. + itIs = itIs || elem.type === 'ExpressionStatement' && elem.expression.value === 'use strict'; - return commentLength; + return itIs; } -function isPreviousNodeAVarDecl(previousNode, varDecl, whitespaceOffsetBeforeVarDecl, commentTokens, file) { - var offsetForComments; - if (varDecl.range[0] === previousNode.range[1]) { - return true; +function isVarDeclFirst(varDecl) { + var elem = varDecl; + // All elements should be relevant. + while (elem && (elem.isComment || elem.isWhitespace || isRelevantElement(elem))) { + elem = elem.previousSibling; } - offsetForComments = getCommentOffsetBetweenNodes(previousNode, varDecl, commentTokens, file); - if (varDecl.range[0] - whitespaceOffsetBeforeVarDecl - offsetForComments === previousNode.range[1]) { + if (elem === null) { return true; } @@ -234,62 +167,19 @@ module.exports.prototype = { }, check: function(file, errors) { - var scopesFoundInFile = {}; - var commentTokens = []; - - file.iterateTokensByType(['CommentLine', 'CommentBlock'], function(commentToken) { - commentTokens.push(commentToken); - }); - file.iterateNodesByType(['VariableDeclaration'], function(varDecl) { // Ignore let and const for now #1783 if (varDecl.kind !== 'var') { return; } - var enclosingScope; - var scopeContents; - var previousNode; - var useStrictDirective; - var isVarDeclFirst = false; - - var whitespaceOffsetBeforeVarDecl = file.getWhitespaceBefore(file.getFirstNodeToken(varDecl)).length; - - enclosingScope = getVariableScope(varDecl.parentElement); - if (!scopesFoundInFile.hasOwnProperty(enclosingScope.range[0])) { - scopesFoundInFile[enclosingScope.range[0]] = { hasNonVarDecl: false, varDecl: [] }; - // placing the handling 'use strict' declared as the first statement of scope here to improve - // performance to run once per scope discovered in file - useStrictDirective = getUseStrictDeclFirst(enclosingScope); - if (useStrictDirective !== null) { - // Special case to make varDecl stack contain the use strict as first node - // this reduces the complexity of the isFirstVarDecInScope and reuses - // isPreviousNodeAVarDecl to handle this special scenario - scopesFoundInFile[enclosingScope.range[0]].varDecl.push(useStrictDirective); - } - } - - scopeContents = scopesFoundInFile[enclosingScope.range[0]]; - - if (scopeContents.varDecl.length === 0) { - isVarDeclFirst = isFirstVarDeclInScope( - enclosingScope, varDecl, whitespaceOffsetBeforeVarDecl, commentTokens, file - ); - } else { - previousNode = scopeContents.varDecl[scopeContents.varDecl.length - 1]; - if (!scopeContents.hasNonVarDecl) { - isVarDeclFirst = isPreviousNodeAVarDecl( - previousNode, varDecl, whitespaceOffsetBeforeVarDecl, commentTokens, file - ); - } + // Checking scope to not allow vars inside block statements. + if (isScopeElement(varDecl.parentElement) && isVarDeclFirst(varDecl)) { + return; } - scopeContents.varDecl.push(varDecl); - if (!isVarDeclFirst) { - scopeContents.hasNonVarDecl = true; - errors.add('Variable declarations must be the first statements of a function scope.', - varDecl); - } + errors.add('Variable declarations must be the first statements of a function scope.', + varDecl); }); } }; diff --git a/test/specs/rules/require-var-decl-first.js b/test/specs/rules/require-var-decl-first.js index d3604183f..af38087a8 100644 --- a/test/specs/rules/require-var-decl-first.js +++ b/test/specs/rules/require-var-decl-first.js @@ -1,7 +1,7 @@ var Checker = require('../../../lib/checker'); var expect = require('chai').expect; -describe.skip('rules/require-var-decl-first', function() { +describe('rules/require-var-decl-first', function() { describe('boolean', function() { var checker; beforeEach(function() { @@ -32,7 +32,8 @@ describe.skip('rules/require-var-decl-first', function() { expect(resultCount).to.equal(expectedErrorCount); for (count = 0; count < resultCount; count++) { expect(errors[count].message) - .to.equal('Variable declarations must be the first statements of a function scope.'); + .to.equal('requireVarDeclFirst: Variable declarations ' + + 'must be the first statements of a function scope.'); } }; @@ -102,17 +103,17 @@ describe.skip('rules/require-var-decl-first', function() { }); it('should not return errors for multiple var declaration at top of program scope', function() { - testDeclStatements(checker, 'var a;var b;', 0); + testDeclStatements(checker, 'var a, c;var b;', 0); }); it('should not return errors for multiple var declaration sandwiching a single comment ' + 'at top of program scope', function() { - testDeclStatements(checker, 'var a;/*block comment*/var b;', 0); + testDeclStatements(checker, 'var a, c;/*block comment*/var b;', 0); }); it('should not return errors for multiple var declaration with a single comment before the ' + 'first var decl at top of program scope', function() { - testDeclStatements(checker, 'var a;/*block comment*/var b;/*block comment 2*/var c;', 0); + testDeclStatements(checker, 'var a, d;/*block comment*/var b;/*block comment 2*/var c;', 0); }); it('should not return errors for a single var declaration with comments sandwiched ' + @@ -180,7 +181,7 @@ describe.skip('rules/require-var-decl-first', function() { }); it('should not return errors for multiple var declaration at top of program scope', function() { - testDeclStatements(checker, 'var a; var b;', 0); + testDeclStatements(checker, 'var a, A; var b;', 0); }); it('should not return errors for a single var declaration with comments sandwiched ' + @@ -200,10 +201,6 @@ describe.skip('rules/require-var-decl-first', function() { testDeclStatements(checker, '"use strict";\nvar a;', 0); }); - it('should not return errors for single const var declaration at top of program scope', function() { - testDeclStatements(checker, '\nconst a = 1;', 0); - }); - it('should not return errors for single var declaration after single comment at top of program scope', function() { testDeclStatements(checker, '/*block comment*/\nvar a;', 0); @@ -225,7 +222,7 @@ describe.skip('rules/require-var-decl-first', function() { }); it('should not return errors for multiple var declaration at top of program scope', function() { - testDeclStatements(checker, 'var a;\nvar b;', 0); + testDeclStatements(checker, 'var a, A;\nvar b;', 0); }); it('should not return errors for a single var declaration with comments sandwiched ' + @@ -236,7 +233,28 @@ describe.skip('rules/require-var-decl-first', function() { it('should not return errors for multiple var declaration with a comment inside the scope of' + ' first variable declaration', function() { - testDeclStatements(checker, 'var a = function() { var b;\n/*block comment*/\n};\nvar c;', 0); + testDeclStatements(checker, 'var a = function() { var b;\n/*block comment*/\n}, A;\nvar c;', 0); + }); + }); + + describe('statements inside blocks', function() { + it('should report for single var declaration inside block', function() { + testDeclStatements(checker, '{var a;}', 1); + }); + + it('should report for single var declaration at top of program scope after prologue', + function() { + testDeclStatements(checker, '{"use strict";\nvar a;}', 1); + }); + + it('should report for single var declaration after single comment inside a block', + function() { + testDeclStatements(checker, '{/*block comment*/\nvar a;}', 1); + }); + + it('should report for single var decl inside a block after assignment', + function() { + testDeclStatements(checker, 'x=1;{var a;}', 1); }); }); });