Skip to content

Commit

Permalink
Breaking: throw an error if disables a non-existent rule in a configu…
Browse files Browse the repository at this point in the history
…ration comment (fixes#9505)
  • Loading branch information
aladdin-add committed May 19, 2019
1 parent 5dad0b1 commit e17b336
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 20 deletions.
10 changes: 9 additions & 1 deletion lib/config/config-validator.js
Expand Up @@ -125,6 +125,12 @@ function validateRuleOptions(rule, ruleId, options, source = null) {
if (!rule) {
return;
}

// https://github.com/eslint/eslint/issues/9505
if (rule && rule.meta && rule.meta.type === "internal") {
throw new Error(`Definition for rule '${ruleId}' was not found`);
}

try {
const severity = validateRuleSeverity(options);

Expand Down Expand Up @@ -332,5 +338,7 @@ module.exports = {
validate,
validateConfigArray,
validateConfigSchema,
validateRuleOptions
validateRuleOptions,
validateRules,
validateRuleSeverity
};
50 changes: 43 additions & 7 deletions lib/linter.js
Expand Up @@ -243,9 +243,39 @@ function getDirectiveComments(filename, ast, ruleMapper) {
}
break;

case "eslint-disable":
disableDirectives.push(...createDisableDirectives("disable", comment.loc.start, directiveValue));
case "eslint-disable": {
const directiveRules = createDisableDirectives("disable", comment.loc.start, directiveValue);

// validate disable directives
directiveRules.forEach(directive => {
const ruleId = directive.ruleId;

if (ruleId !== null) {
const options = directive.type === "disable" ? [0] : [2];

try {
validator.validateRuleOptions(ruleMapper(ruleId), ruleId, options);
} catch (err) {
problems.push({
ruleId,
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
});

// do not apply the config, if found invalid options.
return;
}
}

disableDirectives.push(directive);
});
break;
}

case "eslint-enable":
disableDirectives.push(...createDisableDirectives("enable", comment.loc.start, directiveValue));
Expand All @@ -271,8 +301,13 @@ function getDirectiveComments(filename, ast, ruleMapper) {
endColumn: comment.loc.end.column + 1,
nodeType: null
});

// do not apply the config, if found invalid options.
return;
}

configuredRules[name] = ruleValue;

});
} else {
problems.push(parseResult.error);
Expand Down Expand Up @@ -699,12 +734,13 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser

Object.keys(configuredRules).forEach(ruleId => {
const severity = ConfigOps.getRuleSeverity(configuredRules[ruleId]);
const rule = ruleMapper(ruleId);

if (severity === 0) {
if (severity === 0 || !rule) {
return;
}

const rule = ruleMapper(ruleId);

const messageIds = rule.meta && rule.meta.messages;
let reportTranslator = null;
const ruleContext = Object.freeze(
Expand Down Expand Up @@ -946,13 +982,13 @@ class Linter {
configuredGlobals,
{ exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals }
);
let lintingProblems = [];

const configuredRules = Object.assign({}, config.rules, commentDirectives.configuredRules);

let lintingProblems;

try {
lintingProblems = runRules(
lintingProblems = lintingProblems.concat(runRules(
sourceCode,
configuredRules,
ruleId => getRule(slots, ruleId),
Expand All @@ -961,7 +997,7 @@ class Linter {
settings,
options.filename,
options.disableFixes
);
));
} catch (err) {
err.message += `\nOccurred while linting ${options.filename}`;
debug("An error occurred while traversing");
Expand Down
3 changes: 3 additions & 0 deletions lib/rules.js
Expand Up @@ -31,6 +31,9 @@ const createMissingRule = lodash.memoize(ruleId => {
: `Definition for rule '${ruleId}' was not found`;

return {
meta: {
type: "internal" // only used in eslint core.
},
create: context => ({
Program() {
context.report({
Expand Down
12 changes: 0 additions & 12 deletions tests/lib/config/config-validator.js
Expand Up @@ -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");

Expand Down
18 changes: 18 additions & 0 deletions tests/lib/linter.js
Expand Up @@ -1699,6 +1699,24 @@ describe("Linter", () => {
});
});

describe("when evaluating code with comments to disable rules", () => {


it("should report an error when disabling a non-existent rule", () => {
const code = "/*eslint foo:0*/ ;";
const messages = linter.verify(code, {}, filename);

assert.strictEqual(messages.length, 1);
});

it("should report an error when disabling a non-existent rule", () => {
const code = "/*eslint-disable foo*/ ;";
const messages = linter.verify(code, {}, filename);

assert.strictEqual(messages.length, 1);
});
});

describe("when evaluating code with comments to enable multiple rules", () => {
const code = "/*eslint no-alert:1 no-console:1*/ alert('test'); console.log('test');";

Expand Down

0 comments on commit e17b336

Please sign in to comment.