Breaking: stricter rule config validating (fixes eslint#9505)

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

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,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 {
@@ -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(

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]() {

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);
 

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.");
 
 
         });

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 */
     });
 });