Skip to content

Commit

Permalink
Breaking: stricter rule config validating (fixes eslint#9505)
Browse files Browse the repository at this point in the history 
to fix issue eslint#9505, it made a refactor and introduced a few changes to make validating more consistent:
* will report a linting error when enable/disable a non-existent in inline comment.
* will throw an error, if config a non-existent rule to non-zero value.
* fixes problem loc in some cases
  • Loading branch information
@aladdin-add
aladdin-add committed May 24, 2019
1 parent ec105b2 commit 97c2808
Show file tree
Hide file tree
Showing 8 changed files with 179 additions and 95 deletions.
144 changes: 105 additions & 39 deletions lib/linter/linter.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* @fileoverview Main Linter Class
* @author Gyandeep Singh
* @author aladdin-add
*/

"use strict";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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]) {
Expand All @@ -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;
}

Expand All @@ -245,34 +291,39 @@ 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": {
const parseResult = commentParser.parseJsonConfig(directiveValue, comment.loc);

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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down
30 changes: 2 additions & 28 deletions lib/linter/rules.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/**
* @fileoverview Defines a storage for rules.
* @author Nicholas C. Zakas
* @author aladdin-add
*/

"use strict";
Expand All @@ -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
Expand Down Expand Up @@ -88,7 +62,7 @@ class Rules {
return builtInRules.get(ruleId);
}

return createMissingRule(ruleId);
return null;
}

*[Symbol.iterator]() {
Expand Down
7 changes: 4 additions & 3 deletions lib/shared/config-validator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -121,9 +125,6 @@ function validateRuleSchema(rule, localOptions) {
* @returns {void}
*/
function validateRuleOptions(rule, ruleId, options, source = null) {
if (!rule) {
return;
}
try {
const severity = validateRuleSeverity(options);

Expand Down
4 changes: 2 additions & 2 deletions tests/lib/cli-engine/cli-engine.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");


});
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/linter/code-path-analysis/code-path.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,6 @@ describe("CodePathAnalyzer", () => {
*/
});

/* eslint-enable rulesdir/multiline-comment-style */
/* eslint-enable internal-rules/multiline-comment-style */
});
});
Loading

0 comments on commit 97c2808

Please sign in to comment.