From fc74cc04e639bd197510dba09e65ae48d7b197d2 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Sat, 25 Nov 2023 13:09:29 +0100 Subject: [PATCH 1/6] drop support for function-style rules --- lib/config-array/config-array.js | 19 +++---------------- 1 file changed, 3 insertions(+), 16 deletions(-) diff --git a/lib/config-array/config-array.js b/lib/config-array/config-array.js index 133f5a24..5766fc46 100644 --- a/lib/config-array/config-array.js +++ b/lib/config-array/config-array.js @@ -319,31 +319,18 @@ function createConfig(instance, indices) { * @param {string} pluginId The plugin ID for prefix. * @param {Record} defs The definitions to collect. * @param {Map} map The map to output. - * @param {function(T): U} [normalize] The normalize function for each value. * @returns {void} */ -function collect(pluginId, defs, map, normalize) { +function collect(pluginId, defs, map) { if (defs) { const prefix = pluginId && `${pluginId}/`; for (const [key, value] of Object.entries(defs)) { - map.set( - `${prefix}${key}`, - normalize ? normalize(value) : value - ); + map.set(`${prefix}${key}`, value); } } } -/** - * Normalize a rule definition. - * @param {Function|Rule} rule The rule definition to normalize. - * @returns {Rule} The normalized rule definition. - */ -function normalizePluginRule(rule) { - return typeof rule === "function" ? { create: rule } : rule; -} - /** * Delete the mutation methods from a given map. * @param {Map} map The map object to delete. @@ -385,7 +372,7 @@ function initPluginMemberMaps(elements, slots) { collect(pluginId, plugin.environments, slots.envMap); collect(pluginId, plugin.processors, slots.processorMap); - collect(pluginId, plugin.rules, slots.ruleMap, normalizePluginRule); + collect(pluginId, plugin.rules, slots.ruleMap); } } From b7205d35d097a04ed6be4f8610d911e7859c5893 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Wed, 29 Nov 2023 11:06:37 +0100 Subject: [PATCH 2/6] implement schema changes --- lib/shared/config-validator.js | 81 ++++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/lib/shared/config-validator.js b/lib/shared/config-validator.js index 32174a56..531ccbf7 100644 --- a/lib/shared/config-validator.js +++ b/lib/shared/config-validator.js @@ -5,6 +5,12 @@ /* eslint class-methods-use-this: "off" */ +//------------------------------------------------------------------------------ +// Typedefs +//------------------------------------------------------------------------------ + +/** @typedef {import("../shared/types").Rule} Rule */ + //------------------------------------------------------------------------------ // Requirements //------------------------------------------------------------------------------ @@ -33,6 +39,13 @@ const severityMap = { const validated = new WeakSet(); +// JSON schema that disallows passing any options +const noOptionsSchema = Object.freeze({ + type: "array", + minItems: 0, + maxItems: 0 +}); + //----------------------------------------------------------------------------- // Exports //----------------------------------------------------------------------------- @@ -44,17 +57,36 @@ export default class ConfigValidator { /** * Gets a complete options schema for a rule. - * @param {{create: Function, schema: (Array|null)}} rule A new-style rule object - * @returns {Object} JSON Schema for the rule's options. + * @param {Rule} rule A rule object + * @throws {TypeError} If `meta.schema` is specified but is not an array, object or `false`. + * @returns {Object|null} JSON Schema for the rule's options. + * `null` if rule wasn't passed or its `meta.schema` is `false`. */ getRuleOptionsSchema(rule) { if (!rule) { return null; } - const schema = rule.schema || rule.meta && rule.meta.schema; + if (!rule.meta) { + return { ...noOptionsSchema }; // default if `meta.schema` is not specified + } - // Given a tuple of schemas, insert warning level at the beginning + const schema = rule.meta.schema; + + if (typeof schema === "undefined") { + return { ...noOptionsSchema }; // default if `meta.schema` is not specified + } + + // `schema:false` is an allowed explicit opt-out of options validation for the rule + if (schema === false) { + return null; + } + + if (typeof schema !== "object" || schema === null) { + throw new TypeError("Rule's `meta.schema` must be an array or object"); + } + + // ESLint-specific array form needs to be converted into a valid JSON Schema definition if (Array.isArray(schema)) { if (schema.length) { return { @@ -64,16 +96,13 @@ export default class ConfigValidator { maxItems: schema.length }; } - return { - type: "array", - minItems: 0, - maxItems: 0 - }; + // `schema:[]` is an explicit way to specify that the rule does not accept any options + return { ...noOptionsSchema }; } - // Given a full schema, leave it alone - return schema || null; + // `schema:` is assumed to be a valid JSON Schema definition + return schema; } /** @@ -101,10 +130,18 @@ export default class ConfigValidator { */ validateRuleSchema(rule, localOptions) { if (!ruleValidators.has(rule)) { - const schema = this.getRuleOptionsSchema(rule); + try { + const schema = this.getRuleOptionsSchema(rule); + + if (schema) { + ruleValidators.set(rule, ajv.compile(schema)); + } + } catch (err) { + const errorWithCode = new Error(err.message, { cause: err }); - if (schema) { - ruleValidators.set(rule, ajv.compile(schema)); + errorWithCode.code = "ESLINT_INVALID_RULE_OPTIONS_SCHEMA"; + + throw errorWithCode; } } @@ -137,13 +174,21 @@ export default class ConfigValidator { this.validateRuleSchema(rule, Array.isArray(options) ? options.slice(1) : []); } } catch (err) { - const enhancedMessage = `Configuration for rule "${ruleId}" is invalid:\n${err.message}`; + let enhancedMessage = err.code === "ESLINT_INVALID_RULE_OPTIONS_SCHEMA" + ? `Error while processing options validation schema of rule '${ruleId}': ${err.message}` + : `Configuration for rule "${ruleId}" is invalid:\n${err.message}`; if (typeof source === "string") { - throw new Error(`${source}:\n\t${enhancedMessage}`); - } else { - throw new Error(enhancedMessage); + enhancedMessage = `${source}:\n\t${enhancedMessage}`; } + + const enhancedError = new Error(enhancedMessage, { cause: err }); + + if (err.code) { + enhancedError.code = err.code; + } + + throw enhancedError; } } From d5f11bdcaf96459d8d54493d57d5f99dfbc83908 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Wed, 29 Nov 2023 11:39:39 +0100 Subject: [PATCH 3/6] add unit tests for ConfigValidator#getRuleOptionsSchema --- tests/lib/shared/config-validator.js | 144 +++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) create mode 100644 tests/lib/shared/config-validator.js diff --git a/tests/lib/shared/config-validator.js b/tests/lib/shared/config-validator.js new file mode 100644 index 00000000..12d36935 --- /dev/null +++ b/tests/lib/shared/config-validator.js @@ -0,0 +1,144 @@ +/** + * @fileoverview Tests for ConfigValidator class + */ + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +import { assert } from "chai"; + +import ConfigValidator from "../../../lib/shared/config-validator.js"; + +//------------------------------------------------------------------------------ +// Helpers +//------------------------------------------------------------------------------ + +const mockObjectRule = { + meta: { + schema: { + enum: ["first", "second"] + } + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + +//------------------------------------------------------------------------------ +// Tests +//------------------------------------------------------------------------------ + +describe("ConfigValidator", () => { + let validator; + + beforeEach(() => { + validator = new ConfigValidator(); + }); + + describe("getRuleOptionsSchema", () => { + + const noOptionsSchema = { + type: "array", + minItems: 0, + maxItems: 0 + }; + + it("should return null for a missing rule", () => { + assert.strictEqual(validator.getRuleOptionsSchema(void 0), null); + assert.strictEqual(validator.getRuleOptionsSchema(null), null); + }); + + it("should not modify object schema", () => { + assert.deepStrictEqual(validator.getRuleOptionsSchema(mockObjectRule), { + enum: ["first", "second"] + }); + }); + + it("should return schema that doesn't accept options if rule doesn't have `meta`", () => { + const rule = {}; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return schema that doesn't accept options if rule doesn't have `meta.schema`", () => { + const rule = { meta: {} }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return schema that doesn't accept options if `meta.schema` is `undefined`", () => { + const rule = { meta: { schema: void 0 } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return schema that doesn't accept options if `meta.schema` is `[]`", () => { + const rule = { meta: { schema: [] } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + it("should return JSON Schema definition object if `meta.schema` is in the array form", () => { + const firstOption = { enum: ["always", "never"] }; + const rule = { meta: { schema: [firstOption] } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual( + result, + { + type: "array", + items: [firstOption], + minItems: 0, + maxItems: 1 + } + ); + }); + + it("should return `meta.schema` as is if `meta.schema` is an object", () => { + const schema = { + type: "array", + items: [{ + enum: ["always", "never"] + }] + }; + const rule = { meta: { schema } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, schema); + }); + + it("should return `null` if `meta.schema` is `false`", () => { + const rule = { meta: { schema: false } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.strictEqual(result, null); + }); + + [null, true, 0, 1, "", "always", () => {}].forEach(schema => { + it(`should throw an error if \`meta.schema\` is ${typeof schema} ${schema}`, () => { + const rule = { meta: { schema } }; + + assert.throws(() => { + validator.getRuleOptionsSchema(rule); + }, "Rule's `meta.schema` must be an array or object"); + }); + }); + + it("should ignore top-level `schema` property", () => { + const rule = { schema: { enum: ["always", "never"] } }; + const result = validator.getRuleOptionsSchema(rule); + + assert.deepStrictEqual(result, noOptionsSchema); + }); + + }); +}); From c3e9aa0639f460a4f37217bb7ee57404a6d5b935 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Wed, 29 Nov 2023 11:55:19 +0100 Subject: [PATCH 4/6] add regression tests for ConfigValidator#validateRuleOptions --- tests/lib/shared/config-validator.js | 59 ++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/lib/shared/config-validator.js b/tests/lib/shared/config-validator.js index 12d36935..7c1d411a 100644 --- a/tests/lib/shared/config-validator.js +++ b/tests/lib/shared/config-validator.js @@ -14,6 +14,21 @@ import ConfigValidator from "../../../lib/shared/config-validator.js"; // Helpers //------------------------------------------------------------------------------ +const mockRule = { + meta: { + schema: [{ + enum: ["first", "second"] + }] + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + const mockObjectRule = { meta: { schema: { @@ -141,4 +156,48 @@ describe("ConfigValidator", () => { }); }); + + describe("validateRuleOptions", () => { + + it("should throw for incorrect warning level number", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", 3, "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); + }); + + it("should throw for incorrect warning level string", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", "booya", "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '\"booya\"').\n"); + }); + + it("should throw for invalid-type warning level", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", [["error"]], "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '[ \"error\" ]').\n"); + }); + + it("should only check warning level for nonexistent rules", () => { + const fn1 = validator.validateRuleOptions.bind(validator, void 0, "non-existent-rule", [3, "foobar"], "tests"); + + assert.throws(fn1, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); + + const fn2 = validator.validateRuleOptions.bind(validator, null, "non-existent-rule", [3, "foobar"], "tests"); + + assert.throws(fn2, "tests:\n\tConfiguration for rule \"non-existent-rule\" is invalid:\n\tSeverity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '3').\n"); + }); + + it("should throw for incorrect configuration values", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", [2, "frist"], "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue \"frist\" should be equal to one of the allowed values.\n"); + }); + + it("should throw for too many configuration values", () => { + const fn = validator.validateRuleOptions.bind(validator, mockRule, "mock-rule", [2, "first", "second"], "tests"); + + assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue [\"first\",\"second\"] should NOT have more than 1 items.\n"); + }); + + }); }); From fd8fad4a62f095addac461fe35543f07a4b09525 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Wed, 29 Nov 2023 13:00:56 +0100 Subject: [PATCH 5/6] add tests for error code --- tests/lib/shared/config-validator.js | 53 ++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/lib/shared/config-validator.js b/tests/lib/shared/config-validator.js index 7c1d411a..abed9832 100644 --- a/tests/lib/shared/config-validator.js +++ b/tests/lib/shared/config-validator.js @@ -7,6 +7,7 @@ //------------------------------------------------------------------------------ import { assert } from "chai"; +import nodeAssert from "assert"; import ConfigValidator from "../../../lib/shared/config-validator.js"; @@ -44,6 +45,34 @@ const mockObjectRule = { } }; +const mockInvalidSchemaTypeRule = { + meta: { + schema: true + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + +const mockInvalidJSONSchemaRule = { + meta: { + schema: { + minItems: [] + } + }, + create(context) { + return { + Program(node) { + context.report(node, "Expected a validation error."); + } + }; + } +}; + //------------------------------------------------------------------------------ // Tests //------------------------------------------------------------------------------ @@ -199,5 +228,29 @@ describe("ConfigValidator", () => { assert.throws(fn, "tests:\n\tConfiguration for rule \"mock-rule\" is invalid:\n\tValue [\"first\",\"second\"] should NOT have more than 1 items.\n"); }); + it("should throw with error code ESLINT_INVALID_RULE_OPTIONS_SCHEMA for rule with an invalid schema type", () => { + const fn = validator.validateRuleOptions.bind(validator, mockInvalidSchemaTypeRule, "invalid-schema-rule", [2], "tests"); + + nodeAssert.throws( + fn, + { + code: "ESLINT_INVALID_RULE_OPTIONS_SCHEMA", + message: "tests:\n\tError while processing options validation schema of rule 'invalid-schema-rule': Rule's `meta.schema` must be an array or object" + } + ); + }); + + it("should throw with error code ESLINT_INVALID_RULE_OPTIONS_SCHEMA for rule with an invalid JSON schema", () => { + const fn = validator.validateRuleOptions.bind(validator, mockInvalidJSONSchemaRule, "invalid-schema-rule", [2], "tests"); + + nodeAssert.throws( + fn, + { + code: "ESLINT_INVALID_RULE_OPTIONS_SCHEMA", + message: "tests:\n\tError while processing options validation schema of rule 'invalid-schema-rule': minItems must be number" + } + ); + }); + }); }); From dd3efc05dc910105a331acb3698042e469d27ef7 Mon Sep 17 00:00:00 2001 From: Milos Djermanovic Date: Wed, 29 Nov 2023 13:30:41 +0100 Subject: [PATCH 6/6] update fixture rules to use object-style --- tests/fixtures/rules/custom-rule.cjs | 24 ++++++++------- tests/fixtures/rules/dir1/no-strings.cjs | 17 ++++++----- tests/fixtures/rules/dir2/no-literals.cjs | 15 +++++----- .../fixtures/rules/make-syntax-error-rule.cjs | 30 +++++++++++-------- tests/fixtures/rules/wrong/custom-rule.cjs | 8 +++-- 5 files changed, 52 insertions(+), 42 deletions(-) diff --git a/tests/fixtures/rules/custom-rule.cjs b/tests/fixtures/rules/custom-rule.cjs index 6d8b662b..a7cc1340 100644 --- a/tests/fixtures/rules/custom-rule.cjs +++ b/tests/fixtures/rules/custom-rule.cjs @@ -1,15 +1,17 @@ -module.exports = function(context) { +"use strict"; - "use strict"; +module.exports = { + meta: { + schema: [] + }, - return { - "Identifier": function(node) { - if (node.name === "foo") { - context.report(node, "Identifier cannot be named 'foo'."); + create(context) { + return { + "Identifier": function(node) { + if (node.name === "foo") { + context.report(node, "Identifier cannot be named 'foo'."); + } } - } - }; - + }; + } }; - -module.exports.schema = []; diff --git a/tests/fixtures/rules/dir1/no-strings.cjs b/tests/fixtures/rules/dir1/no-strings.cjs index 1f566ac0..4997bbbe 100644 --- a/tests/fixtures/rules/dir1/no-strings.cjs +++ b/tests/fixtures/rules/dir1/no-strings.cjs @@ -1,14 +1,15 @@ "use strict"; -module.exports = function(context) { +module.exports = { + create(context) { + return { - return { + "Literal": function(node) { + if (typeof node.value === 'string') { + context.report(node, "String!"); + } - "Literal": function(node) { - if (typeof node.value === 'string') { - context.report(node, "String!"); } - - } - }; + }; + } }; diff --git a/tests/fixtures/rules/dir2/no-literals.cjs b/tests/fixtures/rules/dir2/no-literals.cjs index fdaa2d08..d872718a 100644 --- a/tests/fixtures/rules/dir2/no-literals.cjs +++ b/tests/fixtures/rules/dir2/no-literals.cjs @@ -1,11 +1,12 @@ "use strict"; -module.exports = function(context) { +module.exports = { + create (context) { + return { - return { - - "Literal": function(node) { - context.report(node, "Literal!"); - } - }; + "Literal": function(node) { + context.report(node, "Literal!"); + } + }; + } }; diff --git a/tests/fixtures/rules/make-syntax-error-rule.cjs b/tests/fixtures/rules/make-syntax-error-rule.cjs index 528b4b0f..3d34d9d1 100644 --- a/tests/fixtures/rules/make-syntax-error-rule.cjs +++ b/tests/fixtures/rules/make-syntax-error-rule.cjs @@ -1,14 +1,18 @@ -module.exports = function(context) { - return { - Program: function(node) { - context.report({ - node: node, - message: "ERROR", - fix: function(fixer) { - return fixer.insertTextAfter(node, "this is a syntax error."); - } - }); - } - }; +module.exports = { + meta: { + schema: [] + }, + create(context) { + return { + Program: function(node) { + context.report({ + node: node, + message: "ERROR", + fix: function(fixer) { + return fixer.insertTextAfter(node, "this is a syntax error."); + } + }); + } + }; + } }; -module.exports.schema = []; diff --git a/tests/fixtures/rules/wrong/custom-rule.cjs b/tests/fixtures/rules/wrong/custom-rule.cjs index 9a64230c..b3923e4d 100644 --- a/tests/fixtures/rules/wrong/custom-rule.cjs +++ b/tests/fixtures/rules/wrong/custom-rule.cjs @@ -1,5 +1,7 @@ -module.exports = function() { +"use strict"; - "use strict"; - return (null).something; +module.exports = { + create() { + return (null).something; + } };