Repository: eslint/eslint
Pull request #11592 (2019-04-05 13:06:51)

Fix: getErrorResults function to not mutate passed parameter
<!--
    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)**

[ ] Documentation update
[X] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] 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.)
-->

**Tell us about your environment**

* **ESLint Version:** 5.9.0
* **Node Version:** 10.9.0
* **npm Version:** 6.2.0

<!--
    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)**
Changed the `getErrorResults` to not mutate passed parameter

**Is there anything you'd like reviewers to focus on?**
`getErrorResults` function

See issue #11589

jsf-clabot: [![CLA assistant check](https://cla.js.foundation/pull/badge/not_signed)](https://cla.js.foundation/eslint/eslint?pullRequest=11592) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla.js.foundation/eslint/eslint?pullRequest=11592) before we can accept your contribution.<br/><hr/>**Daniela Maia** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).
danielamaia: Hi @platinumazure, thanks for your reply. 
Test won't pass without changes because the passed argument was being mutated, sice it was being manipulated directly.
As for the test, what I'm trying to prove is that the `report.results` I'm passing as argument doesn't mutate after going through `getErrorResults`.
platinumazure: @danielamaia Oh, now I see. You're right-- it's comparing the *original* report object's message count. Sorry, I misread it. I'll change my review.
danielamaia: > [![CLA assistant check](https://camo.githubusercontent.com/246c4bf37547ead9332a6afcd321d7f65edb2fe3/68747470733a2f2f636c612e6a732e666f756e646174696f6e2f70756c6c2f62616467652f6e6f745f7369676e6564)](https://cla.js.foundation/eslint/eslint?pullRequest=11592) 
> Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla.js.foundation/eslint/eslint?pullRequest=11592) before we can accept your contribution.
> 
> **Daniela Maia** seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please [add the email address used for this commit to your account](https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/#commits-are-not-linked-to-any-user).

It's already signed

Patch:
diff --git a/lib/cli-engine.js b/lib/cli-engine.js
index 7eaa97946..1f4020338 100644
--- a/lib/cli-engine.js
+++ b/lib/cli-engine.js
@@ -543,13 +543,14 @@ class CLIEngine {
 
             if (filteredMessages.length > 0) {
                 filtered.push(
-                    Object.assign(result, {
+                    {
+                        ...result,
                         messages: filteredMessages,
                         errorCount: filteredMessages.length,
                         warningCount: 0,
                         fixableErrorCount: result.fixableErrorCount,
                         fixableWarningCount: 0
-                    })
+                    }
                 );
             }
         });
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/cli-engine.js b/tests/lib/cli-engine.js
index d871bf662..cff648d49 100644
--- a/tests/lib/cli-engine.js
+++ b/tests/lib/cli-engine.js
@@ -3111,6 +3111,20 @@ describe("CLIEngine", () => {
             assert.strictEqual(errorResults[0].messages[4].severity, 2);
         });
 
+        it("should not mutate passed report.results parameter", () => {
+            process.chdir(originalDir);
+            const engine = new CLIEngine({
+                rules: { quotes: [1, "double"] }
+            });
+
+            const report = engine.executeOnText("var foo = 'bar';");
+            const reportResultsLength = report.results[0].messages.length;
+
+            CLIEngine.getErrorResults(report.results);
+
+            assert.lengthOf(report.results[0].messages, reportResultsLength);
+        });
+
         it("should report a warningCount of 0 when looking for errors only", () => {
 
             process.chdir(originalDir);
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"), {