From e4e02cc6938f38ad5028bb8ad82f52460a18dea5 Mon Sep 17 00:00:00 2001 From: "Nicholas C. Zakas" Date: Thu, 19 Sep 2024 04:37:54 -0400 Subject: [PATCH] refactor: Extract processor logic into ProcessorService (#18818) * refactor: Extract processor logic into ProcessorService * Remove .only * Fix failing tests * Remove extra file * Add test for BOM * Update lib/linter/vfile.js Co-authored-by: Milos Djermanovic * Apply feedback and add tests --------- Co-authored-by: Milos Djermanovic --- lib/linter/linter.js | 253 +++++++++++++++++------------- lib/linter/vfile.js | 9 +- lib/services/processor-service.js | 109 +++++++++++++ tests/lib/eslint/eslint.js | 30 ++++ tests/lib/linter/linter.js | 41 +++++ tests/lib/linter/vfile.js | 5 + 6 files changed, 338 insertions(+), 109 deletions(-) create mode 100644 lib/services/processor-service.js diff --git a/lib/linter/linter.js b/lib/linter/linter.js index e4a537c0c9a6..c717634778de 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -54,6 +54,7 @@ const { LATEST_ECMA_VERSION } = require("../../conf/ecma-version"); const { VFile } = require("./vfile"); const { ParserService } = require("../services/parser-service"); const { FileContext } = require("./file-context"); +const { ProcessorService } = require("../services/processor-service"); const STEP_KIND_VISIT = 1; const STEP_KIND_CALL = 2; @@ -1292,27 +1293,18 @@ class Linter { } /** - * Same as linter.verify, except without support for processors. - * @param {string|SourceCode} textOrSourceCode The text to parse or a SourceCode object. + * Lint using eslintrc and without processors. + * @param {VFile} file The file to lint. * @param {ConfigData} providedConfig An ESLintConfig instance to configure everything. * @param {VerifyOptions} [providedOptions] The optional filename of the file being checked. * @throws {Error} If during rule execution. * @returns {(LintMessage|SuppressedLintMessage)[]} The results as an array of messages or an empty array if no messages. */ - _verifyWithoutProcessors(textOrSourceCode, providedConfig, providedOptions) { + #eslintrcVerifyWithoutProcessors(file, providedConfig, providedOptions) { + const slots = internalSlotsMap.get(this); const config = providedConfig || {}; const options = normalizeVerifyOptions(providedOptions, config); - let text; - - // evaluate arguments - if (typeof textOrSourceCode === "string") { - slots.lastSourceCode = null; - text = textOrSourceCode; - } else { - slots.lastSourceCode = textOrSourceCode; - text = textOrSourceCode.text; - } // Resolve parser. let parserName = DEFAULT_PARSER_NAME; @@ -1339,7 +1331,7 @@ class Linter { // search and apply "eslint-env *". const envInFile = options.allowInlineConfig && !options.warnInlineConfig - ? findEslintEnv(text) + ? findEslintEnv(file.body) : {}; const resolvedEnvConfig = Object.assign({ builtin: true }, config.env, envInFile); const enabledEnvs = Object.keys(resolvedEnvConfig) @@ -1355,9 +1347,6 @@ class Linter { parser, parserOptions }); - const file = new VFile(options.filename, text, { - physicalPath: providedOptions.physicalFilename - }); if (!slots.lastSourceCode) { let t; @@ -1468,6 +1457,36 @@ class Linter { .sort((problemA, problemB) => problemA.line - problemB.line || problemA.column - problemB.column), reportUnusedDisableDirectives: options.reportUnusedDisableDirectives }); + + } + + /** + * Same as linter.verify, except without support for processors. + * @param {string|SourceCode} textOrSourceCode The text to parse or a SourceCode object. + * @param {ConfigData} providedConfig An ESLintConfig instance to configure everything. + * @param {VerifyOptions} [providedOptions] The optional filename of the file being checked. + * @throws {Error} If during rule execution. + * @returns {(LintMessage|SuppressedLintMessage)[]} The results as an array of messages or an empty array if no messages. + */ + _verifyWithoutProcessors(textOrSourceCode, providedConfig, providedOptions) { + const slots = internalSlotsMap.get(this); + const filename = normalizeFilename(providedOptions.filename || ""); + let text; + + // evaluate arguments + if (typeof textOrSourceCode === "string") { + slots.lastSourceCode = null; + text = textOrSourceCode; + } else { + slots.lastSourceCode = textOrSourceCode; + text = textOrSourceCode.text; + } + + const file = new VFile(filename, text, { + physicalPath: providedOptions.physicalFilename + }); + + return this.#eslintrcVerifyWithoutProcessors(file, providedConfig, providedOptions); } /** @@ -1537,102 +1556,91 @@ class Linter { * @returns {(LintMessage|SuppressedLintMessage)[]} The found problems. */ _verifyWithFlatConfigArrayAndProcessor(textOrSourceCode, config, options, configForRecursive) { + const slots = internalSlotsMap.get(this); const filename = options.filename || ""; const filenameToExpose = normalizeFilename(filename); const physicalFilename = options.physicalFilename || filenameToExpose; const text = ensureText(textOrSourceCode); + const file = new VFile(filenameToExpose, text, { + physicalPath: physicalFilename + }); + const preprocess = options.preprocess || (rawText => [rawText]); const postprocess = options.postprocess || (messagesList => messagesList.flat()); + + const processorService = new ProcessorService(); + const preprocessResult = processorService.preprocessSync(file, { + processor: { + preprocess, + postprocess + } + }); + + if (!preprocessResult.ok) { + return preprocessResult.errors; + } + const filterCodeBlock = options.filterCodeBlock || (blockFilename => blockFilename.endsWith(".js")); const originalExtname = path.extname(filename); + const { files } = preprocessResult; - let blocks; - - try { - blocks = preprocess(text, filenameToExpose); - } catch (ex) { - - // If the message includes a leading line number, strip it: - const message = `Preprocessing error: ${ex.message.replace(/^line \d+:/iu, "").trim()}`; - - debug("%s\n%s", message, ex.stack); - - return [ - { - ruleId: null, - fatal: true, - severity: 2, - message, - line: ex.lineNumber, - column: ex.column, - nodeType: null - } - ]; - } - - const messageLists = blocks.map((block, i) => { - debug("A code block was found: %o", block.filename || "(unnamed)"); + const messageLists = files.map(block => { + debug("A code block was found: %o", block.path || "(unnamed)"); // Keep the legacy behavior. if (typeof block === "string") { return this._verifyWithFlatConfigArrayAndWithoutProcessors(block, config, options); } - const blockText = block.text; - const blockName = path.join(filename, `${i}_${block.filename}`); - // Skip this block if filtered. - if (!filterCodeBlock(blockName, blockText)) { + if (!filterCodeBlock(block.path, block.body)) { debug("This code block was skipped."); return []; } // Resolve configuration again if the file content or extension was changed. - if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) { + if (configForRecursive && (text !== block.rawBody || path.extname(block.path) !== originalExtname)) { debug("Resolving configuration again because the file content or extension was changed."); return this._verifyWithFlatConfigArray( - blockText, + block.rawBody, configForRecursive, - { ...options, filename: blockName, physicalFilename } + { ...options, filename: block.path, physicalFilename: block.physicalPath } ); } + slots.lastSourceCode = null; + // Does lint. - return this._verifyWithFlatConfigArrayAndWithoutProcessors( - blockText, + return this.#flatVerifyWithoutProcessors( + block, config, - { ...options, filename: blockName, physicalFilename } + { ...options, filename: block.path, physicalFilename: block.physicalPath } ); }); - return postprocess(messageLists, filenameToExpose); + return processorService.postprocessSync(file, messageLists, { + processor: { + preprocess, + postprocess + } + }); } /** - * Same as linter.verify, except without support for processors. - * @param {string|SourceCode} textOrSourceCode The text to parse or a SourceCode object. + * Verify using flat config and without any processors. + * @param {VFile} file The file to lint. * @param {FlatConfig} providedConfig An ESLintConfig instance to configure everything. * @param {VerifyOptions} [providedOptions] The optional filename of the file being checked. * @throws {Error} If during rule execution. * @returns {(LintMessage|SuppressedLintMessage)[]} The results as an array of messages or an empty array if no messages. */ - _verifyWithFlatConfigArrayAndWithoutProcessors(textOrSourceCode, providedConfig, providedOptions) { + #flatVerifyWithoutProcessors(file, providedConfig, providedOptions) { + const slots = internalSlotsMap.get(this); const config = providedConfig || {}; const options = normalizeVerifyOptions(providedOptions, config); - let text; - - // evaluate arguments - if (typeof textOrSourceCode === "string") { - slots.lastSourceCode = null; - text = textOrSourceCode; - } else { - slots.lastSourceCode = textOrSourceCode; - text = textOrSourceCode.text; - } - const languageOptions = config.languageOptions; languageOptions.ecmaVersion = normalizeEcmaVersionForLanguageOptions( @@ -1663,9 +1671,6 @@ class Linter { } const settings = config.settings || {}; - const file = new VFile(options.filename, text, { - physicalPath: providedOptions.physicalFilename - }); if (!slots.lastSourceCode) { let t; @@ -1957,6 +1962,37 @@ class Linter { ruleFilter: options.ruleFilter, configuredRules }); + + + } + + /** + * Same as linter.verify, except without support for processors. + * @param {string|SourceCode} textOrSourceCode The text to parse or a SourceCode object. + * @param {FlatConfig} providedConfig An ESLintConfig instance to configure everything. + * @param {VerifyOptions} [providedOptions] The optional filename of the file being checked. + * @throws {Error} If during rule execution. + * @returns {(LintMessage|SuppressedLintMessage)[]} The results as an array of messages or an empty array if no messages. + */ + _verifyWithFlatConfigArrayAndWithoutProcessors(textOrSourceCode, providedConfig, providedOptions) { + const slots = internalSlotsMap.get(this); + const filename = normalizeFilename(providedOptions.filename || ""); + let text; + + // evaluate arguments + if (typeof textOrSourceCode === "string") { + slots.lastSourceCode = null; + text = textOrSourceCode; + } else { + slots.lastSourceCode = textOrSourceCode; + text = textOrSourceCode.text; + } + + const file = new VFile(filename, text, { + physicalPath: providedOptions.physicalFilename + }); + + return this.#flatVerifyWithoutProcessors(file, providedConfig, providedOptions); } /** @@ -2057,77 +2093,78 @@ class Linter { * @returns {(LintMessage|SuppressedLintMessage)[]} The found problems. */ _verifyWithProcessor(textOrSourceCode, config, options, configForRecursive) { + const slots = internalSlotsMap.get(this); const filename = options.filename || ""; const filenameToExpose = normalizeFilename(filename); const physicalFilename = options.physicalFilename || filenameToExpose; const text = ensureText(textOrSourceCode); + const file = new VFile(filenameToExpose, text, { + physicalPath: physicalFilename + }); + const preprocess = options.preprocess || (rawText => [rawText]); const postprocess = options.postprocess || (messagesList => messagesList.flat()); - const filterCodeBlock = - options.filterCodeBlock || - (blockFilename => blockFilename.endsWith(".js")); - const originalExtname = path.extname(filename); - - let blocks; - try { - blocks = preprocess(text, filenameToExpose); - } catch (ex) { + const processorService = new ProcessorService(); + const preprocessResult = processorService.preprocessSync(file, { + processor: { + preprocess, + postprocess + } + }); - // If the message includes a leading line number, strip it: - const message = `Preprocessing error: ${ex.message.replace(/^line \d+:/iu, "").trim()}`; + if (!preprocessResult.ok) { + return preprocessResult.errors; + } - debug("%s\n%s", message, ex.stack); + const filterCodeBlock = + options.filterCodeBlock || + (blockFilePath => blockFilePath.endsWith(".js")); + const originalExtname = path.extname(filename); - return [ - { - ruleId: null, - fatal: true, - severity: 2, - message, - line: ex.lineNumber, - column: ex.column, - nodeType: null - } - ]; - } + const { files } = preprocessResult; - const messageLists = blocks.map((block, i) => { - debug("A code block was found: %o", block.filename || "(unnamed)"); + const messageLists = files.map(block => { + debug("A code block was found: %o", block.path ?? "(unnamed)"); // Keep the legacy behavior. if (typeof block === "string") { return this._verifyWithoutProcessors(block, config, options); } - const blockText = block.text; - const blockName = path.join(filename, `${i}_${block.filename}`); - // Skip this block if filtered. - if (!filterCodeBlock(blockName, blockText)) { + if (!filterCodeBlock(block.path, block.body)) { debug("This code block was skipped."); return []; } // Resolve configuration again if the file content or extension was changed. - if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) { + if (configForRecursive && (text !== block.rawBody || path.extname(block.path) !== originalExtname)) { debug("Resolving configuration again because the file content or extension was changed."); return this._verifyWithConfigArray( - blockText, + block.rawBody, configForRecursive, - { ...options, filename: blockName, physicalFilename } + { ...options, filename: block.path, physicalFilename: block.physicalPath } ); } + slots.lastSourceCode = null; + // Does lint. - return this._verifyWithoutProcessors( - blockText, + return this.#eslintrcVerifyWithoutProcessors( + block, config, - { ...options, filename: blockName, physicalFilename } + { ...options, filename: block.path, physicalFilename: block.physicalPath } ); }); - return postprocess(messageLists, filenameToExpose); + return processorService.postprocessSync(file, messageLists, { + processor: { + preprocess, + postprocess + } + }); + } /** diff --git a/lib/linter/vfile.js b/lib/linter/vfile.js index 8528a5197b05..bb2da0a7795d 100644 --- a/lib/linter/vfile.js +++ b/lib/linter/vfile.js @@ -85,6 +85,13 @@ class VFile { */ body; + /** + * The raw body of the file, including a BOM if present. + * @type {string|Uint8Array} + * @readonly + */ + rawBody; + /** * Indicates whether the file has a byte order mark (BOM). * @type {boolean} @@ -104,8 +111,8 @@ class VFile { this.physicalPath = physicalPath ?? path; this.bom = hasUnicodeBOM(body); this.body = stripUnicodeBOM(body); + this.rawBody = body; } - } module.exports = { VFile }; diff --git a/lib/services/processor-service.js b/lib/services/processor-service.js new file mode 100644 index 000000000000..403b97c1a484 --- /dev/null +++ b/lib/services/processor-service.js @@ -0,0 +1,109 @@ +/** + * @fileoverview ESLint Processor Service + * @author Nicholas C. Zakas + */ +/* eslint class-methods-use-this: off -- Anticipate future constructor arguments. */ + +"use strict"; + +//----------------------------------------------------------------------------- +// Requirements +//----------------------------------------------------------------------------- + +const path = require("node:path"); +const { VFile } = require("../linter/vfile.js"); + +//----------------------------------------------------------------------------- +// Types +//----------------------------------------------------------------------------- + +/** @typedef {import("../shared/types.js").LintMessage} LintMessage */ +/** @typedef {import("../linter/vfile.js").VFile} VFile */ +/** @typedef {import("@eslint/core").Language} Language */ +/** @typedef {import("@eslint/core").LanguageOptions} LanguageOptions */ +/** @typedef {import("eslint").Linter.Processor} Processor */ + +//----------------------------------------------------------------------------- +// Exports +//----------------------------------------------------------------------------- + +/** + * The service that applies processors to files. + */ +class ProcessorService { + + /** + * Preprocesses the given file synchronously. + * @param {VFile} file The file to preprocess. + * @param {{processor:Processor}} config The configuration to use. + * @returns {{ok:boolean, files?: Array, errors?: Array}} An array of preprocessed files or errors. + * @throws {Error} If the preprocessor returns a promise. + */ + preprocessSync(file, config) { + + const { processor } = config; + let blocks; + + try { + blocks = processor.preprocess(file.rawBody, file.path); + } catch (ex) { + + // If the message includes a leading line number, strip it: + const message = `Preprocessing error: ${ex.message.replace(/^line \d+:/iu, "").trim()}`; + + return { + ok: false, + errors: [ + { + ruleId: null, + fatal: true, + severity: 2, + message, + line: ex.lineNumber, + column: ex.column, + nodeType: null + } + ] + }; + } + + if (typeof blocks.then === "function") { + throw new Error("Unsupported: Preprocessor returned a promise."); + } + + return { + ok: true, + files: blocks.map((block, i) => { + + // Legacy behavior: return the block as a string + if (typeof block === "string") { + return block; + } + + const filePath = path.join(file.path, `${i}_${block.filename}`); + + return new VFile(filePath, block.text, { + physicalPath: file.physicalPath + }); + }) + }; + + } + + /** + * Postprocesses the given messages synchronously. + * @param {VFile} file The file to postprocess. + * @param {LintMessage[][]} messages The messages to postprocess. + * @param {{processor:Processor}} config The configuration to use. + * @returns {LintMessage[]} The postprocessed messages. + */ + postprocessSync(file, messages, config) { + + const { processor } = config; + + return processor.postprocess(messages, file.path); + } + +} + +module.exports = { ProcessorService }; diff --git a/tests/lib/eslint/eslint.js b/tests/lib/eslint/eslint.js index d53b069403c8..af8809139211 100644 --- a/tests/lib/eslint/eslint.js +++ b/tests/lib/eslint/eslint.js @@ -1246,6 +1246,36 @@ describe("ESLint", () => { }); }); + + it("should pass BOM through processors", async () => { + eslint = new ESLint({ + overrideConfigFile: true, + overrideConfig: [ + { + files: ["**/*.myjs"], + processor: { + preprocess(text, filename) { + return [{ text, filename }]; + }, + postprocess(messages) { + return messages.flat(); + }, + supportsAutofix: true + }, + rules: { + "unicode-bom": ["error", "never"] + } + } + ], + cwd: path.join(fixtureDir) + }); + const results = await eslint.lintText("\uFEFFvar foo = 'bar';", { filePath: "test.myjs" }); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].messages.length, 1); + assert.strictEqual(results[0].messages[0].severity, 2); + assert.strictEqual(results[0].messages[0].ruleId, "unicode-bom"); + }); }); describe("lintFiles()", () => { diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 4d8366bba85f..2673ef396061 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -16071,6 +16071,47 @@ var a = "test2"; assert.strictEqual(logs.length, 1, "preprocess() should only be called once."); }); + it("should pass the BOM to preprocess", () => { + const logs = []; + const code = "\uFEFFfoo"; + const config = { + files: ["**/*.myjs"], + processor: { + preprocess(text, filenameForText) { + logs.push({ + text, + filename: filenameForText + }); + + return [{ text, filename: filenameForText }]; + }, + postprocess(messages) { + return messages.flat(); + } + }, + rules: { + "unicode-bom": ["error", "never"] + } + }; + + const results = linter.verify(code, config, { + filename: "a.myjs", + filterCodeBlock() { + return true; + } + }); + + assert.deepStrictEqual(logs, [ + { + text: code, + filename: "a.myjs" + } + ]); + + assert.strictEqual(results.length, 1); + assert.strictEqual(results[0].ruleId, "unicode-bom"); + }); + it("should apply a preprocessor to the code, and lint each code sample separately", () => { const code = "foo bar baz"; const configs = createFlatConfigArray([ diff --git a/tests/lib/linter/vfile.js b/tests/lib/linter/vfile.js index 2de2e69bbca7..10f13b4815db 100644 --- a/tests/lib/linter/vfile.js +++ b/tests/lib/linter/vfile.js @@ -26,6 +26,7 @@ describe("VFile", () => { assert.strictEqual(vfile.path, "foo.js"); assert.strictEqual(vfile.physicalPath, "foo.js"); assert.strictEqual(vfile.body, "var foo = bar;"); + assert.strictEqual(vfile.rawBody, "var foo = bar;"); assert.isFalse(vfile.bom); }); @@ -35,6 +36,7 @@ describe("VFile", () => { assert.strictEqual(vfile.path, "foo.js"); assert.strictEqual(vfile.physicalPath, "foo.js"); assert.strictEqual(vfile.body, "var foo = bar;"); + assert.strictEqual(vfile.rawBody, "\uFEFFvar foo = bar;"); assert.isTrue(vfile.bom); }); @@ -44,6 +46,7 @@ describe("VFile", () => { assert.strictEqual(vfile.path, "foo.js"); assert.strictEqual(vfile.physicalPath, "foo/bar"); assert.strictEqual(vfile.body, "var foo = bar;"); + assert.strictEqual(vfile.rawBody, "var foo = bar;"); assert.isFalse(vfile.bom); }); @@ -55,6 +58,7 @@ describe("VFile", () => { assert.strictEqual(vfile.path, "foo.js"); assert.strictEqual(vfile.physicalPath, "foo.js"); assert.deepStrictEqual(vfile.body, body); + assert.deepStrictEqual(vfile.rawBody, body); assert.isFalse(vfile.bom); }); @@ -66,6 +70,7 @@ describe("VFile", () => { assert.strictEqual(vfile.path, "foo.js"); assert.strictEqual(vfile.physicalPath, "foo.js"); assert.deepStrictEqual(vfile.body, body.slice(3)); + assert.deepStrictEqual(vfile.rawBody, body); assert.isTrue(vfile.bom); });