Repository: eslint/eslint (js)

Issue:
Validate against invalid rule schema defaults in RuleTester
**The version of ESLint you are using.**

v5.15.0

**The problem you want to solve.**

As of v5.14.0, we allow rule schemas to include the `default` keyword to specify default options. However, it's easy for a rule author to inadvertently use the `default` keyword in a place where the keyword is ignored, resulting in a buggy rule. (Also see: https://github.com/eslint/eslint/pull/11427)

**Your take on the correct solution to problem.**

When we validate rule schemas in `RuleTester`, we should use the new `strictDefaults` option in Ajv to raise an error if a rule schema has an invalid `default` keyword.

This is a breaking change for users who use `RuleTester`, because `RuleTester` would sometimes start reporting additional validation errors. However, rules themselves would still work as before, even if they contain invalid defaults. Additionally, the impact is expected to be very low since we only added support for the `default` keyword recently.

**Are you willing to submit a pull request to implement this change?**

Yes

Pull request description:
Breaking: disallow invalid rule defaults in RuleTester (fixes #11473)
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

This implements the proposal from https://github.com/eslint/eslint/issues/11473 to make `RuleTester` throw an error when a rule uses an invalid `default` keyword in its schema. It also fixes a few remaining cases where core rules were doing that.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


Patch:
diff --git a/lib/config/config-validator.js b/lib/config/config-validator.js
index 4db7cd563..c6d106e60 100644
--- a/lib/config/config-validator.js
+++ b/lib/config/config-validator.js
@@ -10,12 +10,12 @@
 //------------------------------------------------------------------------------
 
 const path = require("path"),
-    ajv = require("../util/ajv"),
     lodash = require("lodash"),
     configSchema = require("../../conf/config-schema.js"),
     util = require("util"),
     ConfigOps = require("./config-ops");
 
+const ajv = require("../util/ajv")();
 const ruleValidators = new WeakMap();
 
 //------------------------------------------------------------------------------
diff --git a/lib/rules/line-comment-position.js b/lib/rules/line-comment-position.js
index 9a91d4302..d2af56e8a 100644
--- a/lib/rules/line-comment-position.js
+++ b/lib/rules/line-comment-position.js
@@ -31,8 +31,7 @@ module.exports = {
                         type: "object",
                         properties: {
                             position: {
-                                enum: ["above", "beside"],
-                                default: "above"
+                                enum: ["above", "beside"]
                             },
                             ignorePattern: {
                                 type: "string"
diff --git a/lib/rules/max-len.js b/lib/rules/max-len.js
index 438e38b0a..24ceeffb3 100644
--- a/lib/rules/max-len.js
+++ b/lib/rules/max-len.js
@@ -14,8 +14,7 @@ const OPTIONS_SCHEMA = {
     properties: {
         code: {
             type: "integer",
-            minimum: 0,
-            default: 80
+            minimum: 0
         },
         comments: {
             type: "integer",
@@ -23,35 +22,28 @@ const OPTIONS_SCHEMA = {
         },
         tabWidth: {
             type: "integer",
-            minimum: 0,
-            default: 4
+            minimum: 0
         },
         ignorePattern: {
             type: "string"
         },
         ignoreComments: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         ignoreStrings: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         ignoreUrls: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         ignoreTemplateLiterals: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         ignoreRegExpLiterals: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         ignoreTrailingComments: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         }
     },
     additionalProperties: false
@@ -141,14 +133,14 @@ module.exports = {
             options.tabWidth = context.options[1];
         }
 
-        const maxLength = options.code || 80,
-            tabWidth = options.tabWidth || 4,
-            ignoreComments = options.ignoreComments || false,
-            ignoreStrings = options.ignoreStrings || false,
-            ignoreTemplateLiterals = options.ignoreTemplateLiterals || false,
-            ignoreRegExpLiterals = options.ignoreRegExpLiterals || false,
-            ignoreTrailingComments = options.ignoreTrailingComments || options.ignoreComments || false,
-            ignoreUrls = options.ignoreUrls || false,
+        const maxLength = typeof options.code === "number" ? options.code : 80,
+            tabWidth = typeof options.tabWidth === "number" ? options.tabWidth : 4,
+            ignoreComments = !!options.ignoreComments,
+            ignoreStrings = !!options.ignoreStrings,
+            ignoreTemplateLiterals = !!options.ignoreTemplateLiterals,
+            ignoreRegExpLiterals = !!options.ignoreRegExpLiterals,
+            ignoreTrailingComments = !!options.ignoreTrailingComments || !!options.ignoreComments,
+            ignoreUrls = !!options.ignoreUrls,
             maxCommentLength = options.comments;
         let ignorePattern = options.ignorePattern || null;
 
diff --git a/lib/rules/max-lines-per-function.js b/lib/rules/max-lines-per-function.js
index 3b2f656fb..5abb3f0b8 100644
--- a/lib/rules/max-lines-per-function.js
+++ b/lib/rules/max-lines-per-function.js
@@ -19,20 +19,16 @@ const OPTIONS_SCHEMA = {
     properties: {
         max: {
             type: "integer",
-            minimum: 0,
-            default: 50
+            minimum: 0
         },
         skipComments: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         skipBlankLines: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         },
         IIFEs: {
-            type: "boolean",
-            default: false
+            type: "boolean"
         }
     },
     additionalProperties: false
@@ -101,10 +97,10 @@ module.exports = {
         let IIFEs = false;
 
         if (typeof option === "object") {
-            maxLines = option.max;
-            skipComments = option.skipComments;
-            skipBlankLines = option.skipBlankLines;
-            IIFEs = option.IIFEs;
+            maxLines = typeof option.max === "number" ? option.max : 50;
+            skipComments = !!option.skipComments;
+            skipBlankLines = !!option.skipBlankLines;
+            IIFEs = !!option.IIFEs;
         } else if (typeof option === "number") {
             maxLines = option;
         }
diff --git a/lib/rules/one-var.js b/lib/rules/one-var.js
index 0571cd338..e79461ce4 100644
--- a/lib/rules/one-var.js
+++ b/lib/rules/one-var.js
@@ -32,8 +32,7 @@ module.exports = {
                         type: "object",
                         properties: {
                             separateRequires: {
-                                type: "boolean",
-                                default: false
+                                type: "boolean"
                             },
                             var: {
                                 enum: ["always", "never", "consecutive"]
@@ -77,7 +76,7 @@ module.exports = {
             options.let = { uninitialized: mode, initialized: mode };
             options.const = { uninitialized: mode, initialized: mode };
         } else if (typeof mode === "object") { // options configuration is an object
-            options.separateRequires = mode.separateRequires;
+            options.separateRequires = !!mode.separateRequires;
             options.var = { uninitialized: mode.var, initialized: mode.var };
             options.let = { uninitialized: mode.let, initialized: mode.let };
             options.const = { uninitialized: mode.const, initialized: mode.const };
diff --git a/lib/util/ajv.js b/lib/util/ajv.js
index ecf0ebd17..3fb0fbdd3 100644
--- a/lib/util/ajv.js
+++ b/lib/util/ajv.js
@@ -15,17 +15,20 @@ const Ajv = require("ajv"),
 // Public Interface
 //------------------------------------------------------------------------------
 
-const ajv = new Ajv({
-    meta: false,
-    useDefaults: true,
-    validateSchema: false,
-    missingRefs: "ignore",
-    verbose: true,
-    schemaId: "auto"
-});
+module.exports = (additionalOptions = {}) => {
+    const ajv = new Ajv({
+        meta: false,
+        useDefaults: true,
+        validateSchema: false,
+        missingRefs: "ignore",
+        verbose: true,
+        schemaId: "auto",
+        ...additionalOptions
+    });
 
-ajv.addMetaSchema(metaSchema);
-// eslint-disable-next-line no-underscore-dangle
-ajv._opts.defaultMeta = metaSchema.id;
+    ajv.addMetaSchema(metaSchema);
+    // eslint-disable-next-line no-underscore-dangle
+    ajv._opts.defaultMeta = metaSchema.id;
 
-module.exports = ajv;
+    return ajv;
+};
diff --git a/package.json b/package.json
index 9bf5ff6d9..d15aa4d1f 100644
--- a/package.json
+++ b/package.json
@@ -36,7 +36,7 @@
   "bugs": "https://github.com/eslint/eslint/issues/",
   "dependencies": {
     "@babel/code-frame": "^7.0.0",
-    "ajv": "^6.9.1",
+    "ajv": "^6.10.0",
     "chalk": "^2.1.0",
     "cross-spawn": "^6.0.5",
     "debug": "^4.0.1",


Test patch:
diff --git a/lib/testers/rule-tester.js b/lib/testers/rule-tester.js
index b0bfeac97..2cad541ce 100644
--- a/lib/testers/rule-tester.js
+++ b/lib/testers/rule-tester.js
@@ -45,12 +45,13 @@ const lodash = require("lodash"),
     path = require("path"),
     util = require("util"),
     validator = require("../config/config-validator"),
-    ajv = require("../util/ajv"),
     Linter = require("../linter"),
     Environments = require("../config/environments"),
     SourceCodeFixer = require("../util/source-code-fixer"),
     interpolate = require("../util/interpolate");
 
+const ajv = require("../util/ajv")({ strictDefaults: true });
+
 //------------------------------------------------------------------------------
 // Private Members
 //------------------------------------------------------------------------------
@@ -383,6 +384,18 @@ class RuleTester {
 
                     throw new Error([`Schema for rule ${ruleName} is invalid:`, errors]);
                 }
+
+                /*
+                 * `ajv.validateSchema` checks for errors in the structure of the schema (by comparing the schema against a "meta-schema"),
+                 * and it reports those errors individually. However, there are other types of schema errors that only occur when compiling
+                 * the schema (e.g. using invalid defaults in a schema), and only one of these errors can be reported at a time. As a result,
+                 * the schema is compiled here separately from checking for `validateSchema` errors.
+                 */
+                try {
+                    ajv.compile(schema);
+                } catch (err) {
+                    throw new Error(`Schema for rule ${ruleName} is invalid: ${err.message}`);
+                }
             }
 
             validator.validate(config, ruleMap.get.bind(ruleMap), new Environments(), "rule-tester");
diff --git a/tests/conf/config-schema.js b/tests/conf/config-schema.js
index 3fdb4bb03..09fa86061 100644
--- a/tests/conf/config-schema.js
+++ b/tests/conf/config-schema.js
@@ -9,10 +9,11 @@
 // Requirements
 //------------------------------------------------------------------------------
 
-const ajv = require("../../lib/util/ajv"),
-    configSchema = require("../../conf/config-schema.js"),
+const configSchema = require("../../conf/config-schema.js"),
     assert = require("assert");
 
+const ajv = require("../../lib/util/ajv")();
+
 //------------------------------------------------------------------------------
 // Tests
 //------------------------------------------------------------------------------
diff --git a/tests/lib/rules/max-len.js b/tests/lib/rules/max-len.js
index 41b0efe9b..0af7965b6 100644
--- a/tests/lib/rules/max-len.js
+++ b/tests/lib/rules/max-len.js
@@ -184,6 +184,11 @@ ruleTester.run("max-len", rule, {
         {
             code: "var longNameLongName = '𝌆𝌆'",
             options: [5, { ignorePattern: "𝌆{2}" }]
+        },
+
+        {
+            code: "\tfoo",
+            options: [4, { tabWidth: 0 }]
         }
     ],
 
@@ -625,6 +630,20 @@ ruleTester.run("max-len", rule, {
                     column: 1
                 }
             ]
+        },
+
+        {
+            code: "a",
+            options: [0],
+            errors: [
+                {
+                    messageId: "max",
+                    data: { lineNumber: 1, maxLength: 0 },
+                    type: "Program",
+                    line: 1,
+                    column: 1
+                }
+            ]
         }
     ]
 });
diff --git a/tests/lib/rules/max-lines-per-function.js b/tests/lib/rules/max-lines-per-function.js
index 9046a1406..2879c2b62 100644
--- a/tests/lib/rules/max-lines-per-function.js
+++ b/tests/lib/rules/max-lines-per-function.js
@@ -205,6 +205,15 @@ if ( x === y ) {
             ]
         },
 
+        // Test that option defaults work as expected
+        {
+            code: `() => {${"foo\n".repeat(60)}}`,
+            options: [{}],
+            errors: [
+                { messageId: "exceed", data: { name: "arrow function", lineCount: 61, maxLines: 50 } }
+            ]
+        },
+
         // Test skipBlankLines: false
         {
             code: "function name() {\nvar x = 5;\n\t\n \n\nvar x = 2;\n}",
diff --git a/tests/lib/testers/rule-tester.js b/tests/lib/testers/rule-tester.js
index 875925ccc..2ff61f1a5 100644
--- a/tests/lib/testers/rule-tester.js
+++ b/tests/lib/testers/rule-tester.js
@@ -659,6 +659,43 @@ describe("RuleTester", () => {
 
     });
 
+    it("should disallow invalid defaults in rules", () => {
+        const ruleWithInvalidDefaults = {
+            meta: {
+                schema: [
+                    {
+                        oneOf: [
+                            { enum: ["foo"] },
+                            {
+                                type: "object",
+                                properties: {
+                                    foo: {
+                                        enum: ["foo", "bar"],
+                                        default: "foo"
+                                    }
+                                },
+                                additionalProperties: false
+                            }
+                        ]
+                    }
+                ]
+            },
+            create: () => ({})
+        };
+
+        assert.throws(() => {
+            ruleTester.run("invalid-defaults", ruleWithInvalidDefaults, {
+                valid: [
+                    {
+                        code: "foo",
+                        options: [{}]
+                    }
+                ],
+                invalid: []
+            });
+        }, /Schema for rule invalid-defaults is invalid: default is ignored for: data1\.foo/u);
+    });
+
     it("throw an error when an unknown config option is included", () => {
         assert.throws(() => {
             ruleTester.run("no-eval", require("../../fixtures/testers/rule-tester/no-eval"), {