diff --git a/lib/linter/linter.js b/lib/linter/linter.js index e83471fa200e..b48dabc7bd94 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -1,6 +1,7 @@ /** * @fileoverview Main Linter Class * @author Gyandeep Singh + * @author aladdin-add */ "use strict"; @@ -30,12 +31,14 @@ const Rules = require("./rules"), createEmitter = require("./safe-emitter"), SourceCodeFixer = require("./source-code-fixer"), - timing = require("./timing"); + timing = require("./timing"), + ruleReplacements = require("../../conf/replacements.json"); const debug = require("debug")("eslint:linter"); const MAX_AUTOFIX_PASSES = 10; const DEFAULT_PARSER_NAME = "espree"; const commentParser = new ConfigCommentParser(); +const DEFAULT_ERROR_LOC = { start: { line: 1, column: 0 }, end: { line: 1, column: 1 } }; //------------------------------------------------------------------------------ // Typedefs @@ -146,19 +149,71 @@ function addDeclaredGlobals(globalScope, configGlobals, { exportedVariables, ena }); } +/** + * creates a linting problem for a undefined rule. + * @param {string} ruleId the ruleId to create + * @returns {string} created error message + * @private + */ +function createMissingRuleMessage(ruleId) { + return Object.prototype.hasOwnProperty.call(ruleReplacements.rules, ruleId) + ? `Rule '${ruleId}' was removed and replaced by: ${ruleReplacements.rules[ruleId].join(", ")}` + : `Definition for rule '${ruleId}' was not found.`; +} + +/** + * creates a linting problem + * @param {Object} options to create linting error + * @param {string} options.ruleId the ruleId to report + * @param {Object} options.loc the loc to report + * @param {string} options.message the error message to report + * @returns {Problem} created problem, returns a missing-rule problem if only provided ruleId. + * @private + */ +function createLintingProblem(options) { + const { ruleId, loc = DEFAULT_ERROR_LOC, message = createMissingRuleMessage(options.ruleId) } = options; + + return { + ruleId, + message, + line: loc.start.line, + column: loc.start.column + 1, + endLine: loc.end.line, + endColumn: loc.end.column + 1, + severity: 2, + nodeType: null + }; +} + /** * Creates a collection of disable directives from a comment - * @param {("disable"|"enable"|"disable-line"|"disable-next-line")} type The type of directive comment - * @param {{line: number, column: number}} loc The 0-based location of the comment token - * @param {string} value The value after the directive in the comment + * @param {Object} options to create disable directives + * @param {("disable"|"enable"|"disable-line"|"disable-next-line")} options.type The type of directive comment + * @param {{line: number, column: number}} options.loc The 0-based location of the comment token + * @param {string} options.value The value after the directive in the comment * comment specified no specific rules, so it applies to all rules (e.g. `eslint-disable`) - * @returns {DisableDirective[]} Directives from the comment + * @param {function(string): {create: Function}} options.ruleMapper A map from rule IDs to defined rules + * @returns {Object} Directives and problems from the comment */ -function createDisableDirectives(type, loc, value) { +function createDisableDirectives(options) { + const { type, loc, value, ruleMapper } = options; const ruleIds = Object.keys(commentParser.parseListConfig(value)); const directiveRules = ruleIds.length ? ruleIds : [null]; + const result = { + directives: [], // valid disable directives + directiveProblems: [] // problems in directives + }; + + for (const ruleId of directiveRules) { - return directiveRules.map(ruleId => ({ type, line: loc.line, column: loc.column + 1, ruleId })); + // push to directives, if the rule is defined(including null, e.g. /*eslint enable*/) + if (ruleId === null || ruleMapper(ruleId)) { + result.directives.push({ type, line: loc.start.line, column: loc.start.column + 1, ruleId }); + } else { + result.directiveProblems.push(createLintingProblem({ ruleId, loc })); + } + } + return result; } /** @@ -187,23 +242,19 @@ function getDirectiveComments(filename, ast, ruleMapper) { } const directiveValue = trimmedCommentText.slice(match.index + match[1].length); + let directiveType = ""; if (/^eslint-disable-(next-)?line$/u.test(match[1])) { if (comment.loc.start.line === comment.loc.end.line) { - const directiveType = match[1].slice("eslint-".length); - - disableDirectives.push(...createDisableDirectives(directiveType, comment.loc.start, directiveValue)); + directiveType = match[1].slice("eslint-".length); } else { - problems.push({ + const message = `${match[1]} comment should not span multiple lines.`; + + problems.push(createLintingProblem({ ruleId: null, - severity: 2, - message: `${match[1]} comment should not span multiple lines.`, - line: comment.loc.start.line, - column: comment.loc.start.column + 1, - endLine: comment.loc.end.line, - endColumn: comment.loc.end.column + 1, - nodeType: null - }); + message, + loc: comment.loc + })); } } else if (comment.type === "Block") { switch (match[1]) { @@ -219,16 +270,11 @@ function getDirectiveComments(filename, ast, ruleMapper) { try { normalizedValue = ConfigOps.normalizeConfigGlobal(value); } catch (err) { - problems.push({ + problems.push(createLintingProblem({ ruleId: null, - severity: 2, - message: err.message, - line: comment.loc.start.line, - column: comment.loc.start.column + 1, - endLine: comment.loc.end.line, - endColumn: comment.loc.end.column + 1, - nodeType: null - }); + loc: comment.loc, + message: err.message + })); continue; } @@ -245,11 +291,11 @@ function getDirectiveComments(filename, ast, ruleMapper) { break; case "eslint-disable": - disableDirectives.push(...createDisableDirectives("disable", comment.loc.start, directiveValue)); + directiveType = "disable"; break; case "eslint-enable": - disableDirectives.push(...createDisableDirectives("enable", comment.loc.start, directiveValue)); + directiveType = "enable"; break; case "eslint": { @@ -257,22 +303,27 @@ function getDirectiveComments(filename, ast, ruleMapper) { if (parseResult.success) { Object.keys(parseResult.config).forEach(name => { + const rule = ruleMapper(name); const ruleValue = parseResult.config[name]; + if (rule === null) { + problems.push(createLintingProblem({ ruleId: name, loc: comment.loc })); + return; + } + try { - validator.validateRuleOptions(ruleMapper(name), name, ruleValue); + validator.validateRuleOptions(rule, name, ruleValue); } catch (err) { - problems.push({ + problems.push(createLintingProblem({ ruleId: name, - severity: 2, message: err.message, - line: comment.loc.start.line, - column: comment.loc.start.column + 1, - endLine: comment.loc.end.line, - endColumn: comment.loc.end.column + 1, - nodeType: null - }); + loc: comment.loc + })); + + // do not apply the config, if found invalid options. + return; } + configuredRules[name] = ruleValue; }); } else { @@ -285,6 +336,14 @@ function getDirectiveComments(filename, ast, ruleMapper) { // no default } } + + if (directiveType !== "") { + const options = { type: directiveType, loc: comment.loc, value: directiveValue, ruleMapper }; + const { directives, directiveProblems } = createDisableDirectives(options); + + disableDirectives.push(...directives); + problems.push(...directiveProblems); + } }); return { @@ -701,11 +760,18 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser Object.keys(configuredRules).forEach(ruleId => { const severity = ConfigOps.getRuleSeverity(configuredRules[ruleId]); + // not load disabled rules if (severity === 0) { return; } const rule = ruleMapper(ruleId); + + if (rule === null) { + lintingProblems.push(createLintingProblem({ ruleId })); + return; + } + const messageIds = rule.meta && rule.meta.messages; let reportTranslator = null; const ruleContext = Object.freeze( diff --git a/lib/linter/rules.js b/lib/linter/rules.js index 1cd68bc7f38e..a153266efb3f 100644 --- a/lib/linter/rules.js +++ b/lib/linter/rules.js @@ -1,6 +1,7 @@ /** * @fileoverview Defines a storage for rules. * @author Nicholas C. Zakas + * @author aladdin-add */ "use strict"; @@ -9,39 +10,12 @@ // Requirements //------------------------------------------------------------------------------ -const lodash = require("lodash"); -const ruleReplacements = require("../../conf/replacements").rules; const builtInRules = require("../rules"); //------------------------------------------------------------------------------ // Helpers //------------------------------------------------------------------------------ -/** - * Creates a stub rule that gets used when a rule with a given ID is not found. - * @param {string} ruleId The ID of the missing rule - * @returns {{create: function(RuleContext): Object}} A rule that reports an error at the first location - * in the program. The report has the message `Definition for rule '${ruleId}' was not found` if the rule is unknown, - * or `Rule '${ruleId}' was removed and replaced by: ${replacements.join(", ")}` if the rule is known to have been - * replaced. - */ -const createMissingRule = lodash.memoize(ruleId => { - const message = Object.prototype.hasOwnProperty.call(ruleReplacements, ruleId) - ? `Rule '${ruleId}' was removed and replaced by: ${ruleReplacements[ruleId].join(", ")}` - : `Definition for rule '${ruleId}' was not found`; - - return { - create: context => ({ - Program() { - context.report({ - loc: { line: 1, column: 0 }, - message - }); - } - }) - }; -}); - /** * Normalizes a rule module to the new-style API * @param {(Function|{create: Function})} rule A rule object, which can either be a function @@ -88,7 +62,7 @@ class Rules { return builtInRules.get(ruleId); } - return createMissingRule(ruleId); + return null; } *[Symbol.iterator]() { diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index dfdcff350ba0..e8504ac5f07b 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -43,6 +43,10 @@ const severityMap = { * @returns {Object} JSON Schema for the rule's options. */ function getRuleOptionsSchema(rule) { + if (!rule) { + return null; + } + const schema = rule.schema || rule.meta && rule.meta.schema; // Given a tuple of schemas, insert warning level at the beginning @@ -124,6 +128,7 @@ function validateRuleOptions(rule, ruleId, options, source = null) { if (!rule) { return; } + try { const severity = validateRuleSeverity(options); diff --git a/tests/lib/cli-engine/cli-engine.js b/tests/lib/cli-engine/cli-engine.js index 0aa68fceaf65..8abed263d53e 100644 --- a/tests/lib/cli-engine/cli-engine.js +++ b/tests/lib/cli-engine/cli-engine.js @@ -1370,8 +1370,8 @@ describe("CLIEngine", () => { assert.strictEqual(report.results.length, 1); assert.strictEqual(report.results[0].messages.length, 1); assert.strictEqual(report.results[0].messages[0].ruleId, "missing-rule"); - assert.strictEqual(report.results[0].messages[0].severity, 1); - assert.strictEqual(report.results[0].messages[0].message, "Definition for rule 'missing-rule' was not found"); + assert.strictEqual(report.results[0].messages[0].severity, 2); + assert.strictEqual(report.results[0].messages[0].message, "Definition for rule 'missing-rule' was not found."); }); diff --git a/tests/lib/linter/code-path-analysis/code-path.js b/tests/lib/linter/code-path-analysis/code-path.js index cb1b53457d7a..395eca022aea 100644 --- a/tests/lib/linter/code-path-analysis/code-path.js +++ b/tests/lib/linter/code-path-analysis/code-path.js @@ -303,6 +303,6 @@ describe("CodePathAnalyzer", () => { */ }); - /* eslint-enable rulesdir/multiline-comment-style */ + /* eslint-enable internal-rules/multiline-comment-style */ }); }); diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 71087a6ce8f1..6af1ce9a7dd6 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -1699,6 +1699,50 @@ describe("Linter", () => { }); }); + describe("when evaluating code with comments to disable rules", () => { + let code, messages; + + it("should report an error when disabling a non-existent rule in inline comment", () => { + code = "/*eslint foo:0*/ ;"; + messages = linter.verify(code, {}, filename); + assert.strictEqual(messages.length, 1); + assert.strictEqual(messages[0].message, "Definition for rule 'foo' was not found."); + + code = "/*eslint-disable foo*/ ;"; + messages = linter.verify(code, {}, filename); + assert.strictEqual(messages.length, 1); + assert.strictEqual(messages[0].message, "Definition for rule 'foo' was not found."); + + code = "/*eslint-disable-line foo*/ ;"; + messages = linter.verify(code, {}, filename); + assert.strictEqual(messages.length, 1); + assert.strictEqual(messages[0].message, "Definition for rule 'foo' was not found."); + + code = "/*eslint-disable-next-line foo*/ ;"; + messages = linter.verify(code, {}, filename); + assert.strictEqual(messages.length, 1); + assert.strictEqual(messages[0].message, "Definition for rule 'foo' was not found."); + }); + + it("should not report an error, when disabling a non-existent rule in config", () => { + messages = linter.verify("", { rules: { foo: 0 } }, filename); + + assert.strictEqual(messages.length, 0); + }); + + it("should report an error, when disabling a non-existent rule in config", () => { + messages = linter.verify("", { rules: { foo: 1 } }, filename); + assert.strictEqual(messages.length, 1); + assert.strictEqual(messages[0].severity, 2); + assert.strictEqual(messages[0].message, "Definition for rule 'foo' was not found."); + + messages = linter.verify("", { rules: { foo: 2 } }, filename); + assert.strictEqual(messages.length, 1); + assert.strictEqual(messages[0].severity, 2); + assert.strictEqual(messages[0].message, "Definition for rule 'foo' was not found."); + }); + }); + describe("when evaluating code with comments to enable multiple rules", () => { const code = "/*eslint no-alert:1 no-console:1*/ alert('test'); console.log('test');"; @@ -2761,7 +2805,7 @@ describe("Linter", () => { const objectOptionResults = linter.verify(code, { rules: { foobar: [1, { bar: false }] } }); const resultsMultiple = linter.verify(code, { rules: { foobar: 2, barfoo: 1 } }); - it("should add a stub rule", () => { + it("should report a problem", () => { assert.isNotNull(result); assert.isArray(results); assert.isObject(result); @@ -2771,13 +2815,13 @@ describe("Linter", () => { it("should report that the rule does not exist", () => { assert.property(result, "message"); - assert.strictEqual(result.message, "Definition for rule 'foobar' was not found"); + assert.strictEqual(result.message, "Definition for rule 'foobar' was not found."); }); it("should report at the correct severity", () => { assert.property(result, "severity"); assert.strictEqual(result.severity, 2); - assert.strictEqual(warningResult.severity, 1); + assert.strictEqual(warningResult.severity, 2); // this is 2, since the rulename is very likely to be wrong }); it("should accept any valid rule configuration", () => { @@ -2792,10 +2836,12 @@ describe("Linter", () => { resultsMultiple[1], { ruleId: "barfoo", - message: "Definition for rule 'barfoo' was not found", + message: "Definition for rule 'barfoo' was not found.", line: 1, column: 1, - severity: 1, + endLine: 1, + endColumn: 2, + severity: 2, nodeType: null } ); diff --git a/tests/lib/linter/rules.js b/tests/lib/linter/rules.js index 950a24d23019..31d6764dc266 100644 --- a/tests/lib/linter/rules.js +++ b/tests/lib/linter/rules.js @@ -52,30 +52,26 @@ describe("rules", () => { }); }); + describe("when a rule is not found", () => { - it("should return a stub rule that reports an error if the rule is unknown", () => { - const stubRule = rules.get("not-defined"); - const linter = new Linter(); + it("should report a linting error if the rule is unknown", () => { - linter.defineRule("test-rule", stubRule); + const linter = new Linter(); const problems = linter.verify("foo", { rules: { "test-rule": "error" } }); assert.lengthOf(problems, 1); - assert.strictEqual(problems[0].message, "Definition for rule 'not-defined' was not found"); + assert.strictEqual(problems[0].message, "Definition for rule 'test-rule' was not found."); assert.strictEqual(problems[0].line, 1); assert.strictEqual(problems[0].column, 1); - assert.typeOf(problems[0].endLine, "undefined"); - assert.typeOf(problems[0].endColumn, "undefined"); + assert.strictEqual(problems[0].endLine, 1); + assert.strictEqual(problems[0].endColumn, 2); }); - it("should return a stub rule that lists replacements if a rule is known to have been replaced", () => { - const stubRule = rules.get("no-arrow-condition"); - const linter = new Linter(); - - linter.defineRule("test-rule", stubRule); - const problems = linter.verify("foo", { rules: { "test-rule": "error" } }); + it("should report a linting error that lists replacements if a rule is known to have been replaced", () => { + const linter = new Linter(); + const problems = linter.verify("foo", { rules: { "no-arrow-condition": "error" } }); assert.lengthOf(problems, 1); assert.strictEqual( @@ -84,11 +80,12 @@ describe("rules", () => { ); assert.strictEqual(problems[0].line, 1); assert.strictEqual(problems[0].column, 1); - assert.typeOf(problems[0].endLine, "undefined"); - assert.typeOf(problems[0].endColumn, "undefined"); + assert.strictEqual(problems[0].endLine, 1); + assert.strictEqual(problems[0].endColumn, 2); }); }); + describe("when loading all rules", () => { it("should iterate all rules", () => { const allRules = new Map(rules); diff --git a/tests/lib/rules/no-inline-comments.js b/tests/lib/rules/no-inline-comments.js index 653548f27582..916c836572ee 100644 --- a/tests/lib/rules/no-inline-comments.js +++ b/tests/lib/rules/no-inline-comments.js @@ -31,8 +31,8 @@ ruleTester.run("no-inline-comments", rule, { "// A valid comment before code\nvar a = 1;", "var a = 2;\n// A valid comment after code", "// A solitary comment", - "var a = 1; // eslint-disable-line some-rule", - "var a = 1; /* eslint-disable-line some-rule */" + "var a = 1; // eslint-disable-line no-debugger", + "var a = 1; /* eslint-disable-line no-debugger */" ], invalid: [ diff --git a/tests/lib/shared/config-validator.js b/tests/lib/shared/config-validator.js index b89c74cfa562..c4974aaf899d 100644 --- a/tests/lib/shared/config-validator.js +++ b/tests/lib/shared/config-validator.js @@ -443,18 +443,6 @@ describe("Validator", () => { assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '[ \"error\" ]').\n"); }); - it("should only check warning level for nonexistent rules", () => { - const fn = validator.validateRuleOptions.bind(null, ruleMapper("non-existent-rule"), "non-existent-rule", [3, "foobar"], "tests"); - - assert.throws(fn, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); - }); - - it("should only check warning level for plugin rules", () => { - const fn = validator.validateRuleOptions.bind(null, ruleMapper("plugin/rule"), "plugin/rule", 3, "tests"); - - assert.throws(fn, "tests:\n\tConfiguration for rule \"plugin/rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); - }); - it("should throw for incorrect configuration values", () => { const fn = validator.validateRuleOptions.bind(null, ruleMapper("mock-rule"), "mock-rule", [2, "frist"], "tests");