From e724d97076734b1657f565d9ea4edbde7fc7ba80 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 1 May 2023 16:54:50 -0400 Subject: [PATCH 1/5] feat: improve flat config errors for invalid rule options and severities --- lib/config/flat-config-schema.js | 92 +++++++++++++++++---------- messages/invalid-rule-options.js | 17 +++++ messages/invalid-rule-severity.js | 13 ++++ messages/shared.js | 18 ++++++ tests/lib/config/flat-config-array.js | 6 +- tests/lib/linter/linter.js | 2 +- 6 files changed, 109 insertions(+), 39 deletions(-) create mode 100644 messages/invalid-rule-options.js create mode 100644 messages/invalid-rule-severity.js create mode 100644 messages/shared.js diff --git a/lib/config/flat-config-schema.js b/lib/config/flat-config-schema.js index bb6e9f899ac..e0a3f9048fa 100644 --- a/lib/config/flat-config-schema.js +++ b/lib/config/flat-config-schema.js @@ -126,32 +126,65 @@ function normalizeRuleOptions(ruleOptions) { // Assertions //----------------------------------------------------------------------------- +/** + * The error type when a rule's options are configured with an invalid type. + */ +class InvalidRuleOptionsError extends Error { + + /** + * @param {string} ruleId Rule name being configured. + * @param {any} value The invalid value. + */ + constructor(ruleId, value) { + super(`Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2.`); + this.messageTemplate = "invalid-rule-options"; + this.messageData = { ruleId, value }; + } +} + /** * Validates that a value is a valid rule options entry. + * @param {string} ruleId Rule name being configured. * @param {any} value The value to check. * @returns {void} - * @throws {TypeError} If the value isn't a valid rule options. + * @throws {InvalidRuleOptionsError} If the value isn't a valid rule options. */ -function assertIsRuleOptions(value) { - +function assertIsRuleOptions(ruleId, value) { if (typeof value !== "string" && typeof value !== "number" && !Array.isArray(value)) { - throw new TypeError("Expected a string, number, or array."); + throw new InvalidRuleOptionsError(ruleId, value); + } +} + +/** + * The error type when a rule's severity is invalid. + */ +class InvalidRuleSeverityError extends Error { + + /** + * @param {string} ruleId Rule name being configured. + * @param {any} value The invalid value. + */ + constructor(ruleId, value) { + super(`Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2.`); + this.messageTemplate = "invalid-rule-severity"; + this.messageData = { ruleId, value }; } } /** * Validates that a value is valid rule severity. + * @param {string} ruleId Rule name being configured. * @param {any} value The value to check. * @returns {void} - * @throws {TypeError} If the value isn't a valid rule severity. + * @throws {InvalidRuleSeverityError} If the value isn't a valid rule severity. */ -function assertIsRuleSeverity(value) { +function assertIsRuleSeverity(ruleId, value) { const severity = typeof value === "string" ? ruleSeverities.get(value.toLowerCase()) : ruleSeverities.get(value); if (typeof severity === "undefined") { - throw new TypeError("Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); + throw new InvalidRuleSeverityError(ruleId, value); } } @@ -357,39 +390,28 @@ const rulesSchema = { validate(value) { assertIsObject(value); - let lastRuleId; - - // Performance: One try-catch has less overhead than one per loop iteration - try { - - /* - * We are not checking the rule schema here because there is no - * guarantee that the rule definition is present at this point. Instead - * we wait and check the rule schema during the finalization step - * of calculating a config. - */ - for (const ruleId of Object.keys(value)) { - - // avoid hairy edge case - if (ruleId === "__proto__") { - continue; - } + /* + * We are not checking the rule schema here because there is no + * guarantee that the rule definition is present at this point. Instead + * we wait and check the rule schema during the finalization step + * of calculating a config. + */ + for (const ruleId of Object.keys(value)) { - lastRuleId = ruleId; + // avoid hairy edge case + if (ruleId === "__proto__") { + continue; + } - const ruleOptions = value[ruleId]; + const ruleOptions = value[ruleId]; - assertIsRuleOptions(ruleOptions); + assertIsRuleOptions(ruleId, ruleOptions); - if (Array.isArray(ruleOptions)) { - assertIsRuleSeverity(ruleOptions[0]); - } else { - assertIsRuleSeverity(ruleOptions); - } + if (Array.isArray(ruleOptions)) { + assertIsRuleSeverity(ruleId, ruleOptions[0]); + } else { + assertIsRuleSeverity(ruleId, ruleOptions); } - } catch (error) { - error.message = `Key "${lastRuleId}": ${error.message}`; - throw error; } } }; diff --git a/messages/invalid-rule-options.js b/messages/invalid-rule-options.js new file mode 100644 index 00000000000..ca19e01ce63 --- /dev/null +++ b/messages/invalid-rule-options.js @@ -0,0 +1,17 @@ +"use strict"; + +const { stringifyValueForError } = require("./shared"); + +module.exports = function({ ruleId, value }) { + return ` +Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2. + +You passed '${stringifyValueForError(value, 4)}'. + +If you're attempting to configure rule options, perhaps you meant: + + ["error", ${stringifyValueForError(value, 8)}] + +See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules. +`.trimStart(); +}; diff --git a/messages/invalid-rule-severity.js b/messages/invalid-rule-severity.js new file mode 100644 index 00000000000..3f13183c6a8 --- /dev/null +++ b/messages/invalid-rule-severity.js @@ -0,0 +1,13 @@ +"use strict"; + +const { stringifyValueForError } = require("./shared"); + +module.exports = function({ ruleId, value }) { + return ` +Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2. + +You passed '${stringifyValueForError(value, 4)}'. + +See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules. +`.trimStart(); +}; diff --git a/messages/shared.js b/messages/shared.js new file mode 100644 index 00000000000..970bd229f09 --- /dev/null +++ b/messages/shared.js @@ -0,0 +1,18 @@ +/** + * @fileoverview Shared utilities for error messages. + * @author Josh Goldberg + */ + +"use strict"; + +/** + * Converts a value to a string that may be printed in errors. + * @param {any} value The invalid value. + * @param {number} indentation How many spaces to indent + * @returns {string} The value, stringified. + */ +function stringifyValueForError(value, indentation) { + return JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`); +} + +module.exports = { stringifyValueForError }; diff --git a/tests/lib/config/flat-config-array.js b/tests/lib/config/flat-config-array.js index 7ca78a388f2..cc5dd0ea811 100644 --- a/tests/lib/config/flat-config-array.js +++ b/tests/lib/config/flat-config-array.js @@ -1712,7 +1712,7 @@ describe("FlatConfigArray", () => { foo: true } } - ], "Key \"rules\": Key \"foo\": Expected a string, number, or array."); + ], "Key \"rules\": Configuration for rule \"foo\" is invalid. Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); }); it("should error when an invalid rule severity of the right type is set", async () => { @@ -1723,7 +1723,7 @@ describe("FlatConfigArray", () => { foo: 3 } } - ], "Key \"rules\": Key \"foo\": Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); + ], "Key \"rules\": Configuration for rule \"foo\" is invalid. Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); }); it("should error when an invalid rule severity is set in an array", async () => { @@ -1734,7 +1734,7 @@ describe("FlatConfigArray", () => { foo: [true] } } - ], "Key \"rules\": Key \"foo\": Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); + ], "Configuration for rule \"foo\" is invalid. Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); }); it("should error when rule doesn't exist", async () => { diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 13824c9da49..6f709ee229a 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -8811,7 +8811,7 @@ describe("Linter with FlatConfigArray", () => { assert.throws(() => { linter.verify(code, config, filename, true); - }, /Key "rules": Key "semi"/u); + }, /Key "rules": Configuration for rule "semi"/u); }); it("should process empty config", () => { From 0ed9bad9ed7bdc7069f924416f5431d9850e0662 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josh=20Goldberg=20=E2=9C=A8?= Date: Mon, 1 May 2023 17:44:31 -0400 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Nicholas C. Zakas --- messages/invalid-rule-options.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/messages/invalid-rule-options.js b/messages/invalid-rule-options.js index ca19e01ce63..9a8acc934ae 100644 --- a/messages/invalid-rule-options.js +++ b/messages/invalid-rule-options.js @@ -4,13 +4,13 @@ const { stringifyValueForError } = require("./shared"); module.exports = function({ ruleId, value }) { return ` -Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2. +Configuration for rule "${ruleId}" is invalid. Each rule must have a severity ("off", 0, "warn", 1, "error", or 2) and may be followed by additional options for the rule. -You passed '${stringifyValueForError(value, 4)}'. +You passed '${stringifyValueForError(value, 4)}', which doesn't contain a valid severity. If you're attempting to configure rule options, perhaps you meant: - ["error", ${stringifyValueForError(value, 8)}] + "${ruleId}": ["error", ${stringifyValueForError(value, 8)}] See https://eslint.org/docs/latest/use/configure/rules#using-configuration-files for configuring rules. `.trimStart(); From 4c844268fac2df8f1c2483a3636982f91061365b Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Mon, 1 May 2023 17:50:39 -0400 Subject: [PATCH 3/5] Updated error messages and tests per review --- lib/config/flat-config-schema.js | 4 ++-- tests/lib/config/flat-config-array.js | 6 +++--- tests/lib/linter/linter.js | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/config/flat-config-schema.js b/lib/config/flat-config-schema.js index e0a3f9048fa..10d6b50ef1f 100644 --- a/lib/config/flat-config-schema.js +++ b/lib/config/flat-config-schema.js @@ -136,7 +136,7 @@ class InvalidRuleOptionsError extends Error { * @param {any} value The invalid value. */ constructor(ruleId, value) { - super(`Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2.`); + super(`Key "${ruleId}": Expected severity of "off", 0, "warn", 1, "error", or 2.`); this.messageTemplate = "invalid-rule-options"; this.messageData = { ruleId, value }; } @@ -165,7 +165,7 @@ class InvalidRuleSeverityError extends Error { * @param {any} value The invalid value. */ constructor(ruleId, value) { - super(`Configuration for rule "${ruleId}" is invalid. Expected severity of "off", 0, "warn", 1, "error", or 2.`); + super(`Key "${ruleId}": Expected severity of "off", 0, "warn", 1, "error", or 2.`); this.messageTemplate = "invalid-rule-severity"; this.messageData = { ruleId, value }; } diff --git a/tests/lib/config/flat-config-array.js b/tests/lib/config/flat-config-array.js index cc5dd0ea811..65d3a729f13 100644 --- a/tests/lib/config/flat-config-array.js +++ b/tests/lib/config/flat-config-array.js @@ -1712,7 +1712,7 @@ describe("FlatConfigArray", () => { foo: true } } - ], "Key \"rules\": Configuration for rule \"foo\" is invalid. Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); + ], "Key \"rules\": Key \"foo\": Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); }); it("should error when an invalid rule severity of the right type is set", async () => { @@ -1723,7 +1723,7 @@ describe("FlatConfigArray", () => { foo: 3 } } - ], "Key \"rules\": Configuration for rule \"foo\" is invalid. Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); + ], "Key \"rules\": Key \"foo\": Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); }); it("should error when an invalid rule severity is set in an array", async () => { @@ -1734,7 +1734,7 @@ describe("FlatConfigArray", () => { foo: [true] } } - ], "Configuration for rule \"foo\" is invalid. Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); + ], "Key \"rules\": Key \"foo\": Expected severity of \"off\", 0, \"warn\", 1, \"error\", or 2."); }); it("should error when rule doesn't exist", async () => { diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 6f709ee229a..650e6d4a503 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -8811,7 +8811,7 @@ describe("Linter with FlatConfigArray", () => { assert.throws(() => { linter.verify(code, config, filename, true); - }, /Key "rules": Configuration for rule "semi"/u); + }, /Key "rules": Key "semi": Expected severity/u); }); it("should process empty config", () => { From 617bfd534ca92694e89b34c65063015798900abb Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 4 May 2023 07:38:10 -0400 Subject: [PATCH 4/5] Account for undefined value in printing --- messages/shared.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/messages/shared.js b/messages/shared.js index 970bd229f09..4bfdcb3e1c2 100644 --- a/messages/shared.js +++ b/messages/shared.js @@ -12,7 +12,8 @@ * @returns {string} The value, stringified. */ function stringifyValueForError(value, indentation) { - return JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`); + // eslint-disable-next-line no-undefined -- Users may provide it + return value === undefined ? `${value}` : JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`); } module.exports = { stringifyValueForError }; From da0ddd99ca6f0d58a1e11e230cb0338858b8eebd Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Thu, 4 May 2023 07:44:51 -0400 Subject: [PATCH 5/5] More general truthiness check --- messages/shared.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/messages/shared.js b/messages/shared.js index 4bfdcb3e1c2..8c6e9b92145 100644 --- a/messages/shared.js +++ b/messages/shared.js @@ -12,8 +12,7 @@ * @returns {string} The value, stringified. */ function stringifyValueForError(value, indentation) { - // eslint-disable-next-line no-undefined -- Users may provide it - return value === undefined ? `${value}` : JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`); + return value ? JSON.stringify(value, null, 4).replace(/\n/gu, `\n${" ".repeat(indentation)}`) : `${value}`; } module.exports = { stringifyValueForError };