From d64bfdde4bb62d4a83476cb5b2c6109fbd9f56a5 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Tue, 3 Feb 2026 20:28:17 +0000 Subject: [PATCH] fix(linter/plugins): ensure `after` hook always runs last in rule converted for ESLint (#18904) Fix an edge case bug in conversion of a plugin using `createOnce` to an ESLint-compatible plugin. If a rule's visitor has an `after` hook, it needs to run after all other visitor functions. Previously we achieved this by adding `after` hook as `Program:exit` visitor fn. However, this will not work properly if the visitor has a selector which matches `Program` but has a higher specificity. e.g.: ```js const rule = { createOnce(context) { return { "Program[sourceType='module']:exit"(node) { // This should run before the `after` hook }, after() { // This should run only at very end of walking AST } }; }, }; ``` In this case `Program[sourceType='module']` has higher specificity than `Program:exit`, so if `after` hook is converted to `Program:exit`, then the `after` hook doesn't run last. Fix this by creating a more specific selector for the `after` hook - in this case `Program[type][type]`. `Program` always has a `type` property, so the selector always matches `Program`, but it executes last due to its higher specificity. --- apps/oxlint/src-js/package/compat.ts | 61 +++++++++++-- .../test/fixtures/eslintCompat/.oxlintrc.json | 1 + .../fixtures/eslintCompat/eslint.config.js | 1 + .../test/fixtures/eslintCompat/eslint.snap.md | 22 ++++- .../test/fixtures/eslintCompat/output.snap.md | 74 ++++++++++++++- .../test/fixtures/eslintCompat/plugin.ts | 91 +++++++++++++++++++ npm/oxlint-plugins/index.cjs | 16 +++- npm/oxlint-plugins/index.js | 16 +++- 8 files changed, 263 insertions(+), 19 deletions(-) diff --git a/apps/oxlint/src-js/package/compat.ts b/apps/oxlint/src-js/package/compat.ts index b66180f2be2b2..8d32c044f0cb3 100644 --- a/apps/oxlint/src-js/package/compat.ts +++ b/apps/oxlint/src-js/package/compat.ts @@ -203,14 +203,59 @@ function createContextAndVisitor(rule: CreateOnceRule): { throw new Error("`after` property of visitor must be a function if defined"); } - const programExit = visitor["Program:exit"]; - visitor["Program:exit"] = - programExit == null - ? (_node) => afterHook() - : (node) => { - programExit(node); - afterHook(); - }; + // We need to make sure that `after` hook is called after all visit fns have been called. + // Usually this is done by adding a `Program:exit` visit fn, but there's an odd edge case: + // Other visit fns could be called after `Program:exit` if they're selectors with a higher specificity. + // e.g. `[body]:exit` would match `Program`, but has higher specificity than `Program:exit`, so would run last. + // + // We don't want to parse every visitor key here to calculate their specificity, so we take a shortcut. + // Selectors which have highest specificity are of types `attribute`, `field`, `nth-child`, and `nth-last-child`. + // + // Examples of selectors of these types: + // * `[id]` (attribute) + // * `.id` (field) + // * `:first-child` (nth-child) + // * `:nth-child(2)` (nth-child) + // * `:last-child` (nth-last-child) + // * `:nth-last-child(2)` (nth-last-child) + // + // All these contain the characters `[`, `.`, or `:`. So just count these characters in all visitor keys, and create + // a selector which always matches `Program`, but with a higher specificity than the most specific exit selector. + // + // e.g. If visitor has key `[id]:first-child:exit`, that contains 2 special characters (not including `:exit`). + // So we use a selector `Program[type][type][type]:exit` (3 attributes = more specific than 2). + // + // ESLint will recognise that this `Program[type][type][type]` selector can only match `Program` nodes, + // and will only execute it only on `Program` node. So the additional cost of checking if the selector matches + // is only paid once per file - insignificant impact on performance. + // `nodeTypes` for this selector is `["Program"]`, so it only gets added to `exitSelectorsByNodeType` for `Program`. + // https://github.com/eslint/eslint/blob/4cecf8393ae9af18c4cfd50621115eb23b3d0cb6/lib/linter/esquery.js#L143-L231 + // https://github.com/eslint/eslint/blob/4cecf8393ae9af18c4cfd50621115eb23b3d0cb6/lib/linter/source-code-traverser.js#L93-L125 + // + // This is blunt tool. We may well create a selector which has a higher specificity than we need. + // But that doesn't really matter - as long as it's specific *enough*, it'll work correctly. + const CHAR_CODE_BRACKET = "[".charCodeAt(0); + const CHAR_CODE_DOT = ".".charCodeAt(0); + const CHAR_CODE_COLON = ":".charCodeAt(0); + + let maxAttrs = -1; + for (const key in visitor) { + if (!Object.hasOwn(visitor, key)) continue; + + // Only `:exit` visit functions matter here + if (!key.endsWith(":exit")) continue; + + const end = key.length - ":exit".length; + let count = 0; + for (let i = 0; i < end; i++) { + const c = key.charCodeAt(i); + if (c === CHAR_CODE_BRACKET || c === CHAR_CODE_DOT || c === CHAR_CODE_COLON) count++; + } + if (count > maxAttrs) maxAttrs = count; + } + + const key = `Program${"[type]".repeat(maxAttrs + 1)}:exit`; + visitor[key] = (_node) => afterHook(); } return { context, visitor, beforeHook }; diff --git a/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json b/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json index 71f69bdb13a72..635b803224e70 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json +++ b/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json @@ -4,6 +4,7 @@ "rules": { "eslint-compat-plugin/create": "error", "eslint-compat-plugin/create-once": "error", + "eslint-compat-plugin/create-once-selector": "error", "eslint-compat-plugin/create-once-before-false": "error", "eslint-compat-plugin/create-once-before-only": "error", "eslint-compat-plugin/create-once-after-only": "error", diff --git a/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js b/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js index 1c2edc11d1a8c..852e14f1fbe52 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js +++ b/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js @@ -9,6 +9,7 @@ export default [ rules: { "eslint-compat-plugin/create": "error", "eslint-compat-plugin/create-once": "error", + "eslint-compat-plugin/create-once-selector": "error", "eslint-compat-plugin/create-once-before-false": "error", "eslint-compat-plugin/create-once-before-only": "error", "eslint-compat-plugin/create-once-after-only": "error", diff --git a/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md b/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md index d74c9e727e942..2ae2280bd4424 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md +++ b/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md @@ -23,6 +23,16 @@ filename: /files/1.js eslint-compat-plugin filename: /files/1.js eslint-compat-plugin/create-once-after-only 0:1 error after hook: filename: /files/1.js eslint-compat-plugin/create-once-hooks-only + 0:1 error after hook: +filename: /files/1.js eslint-compat-plugin/create-once-selector + 1:1 error *:exit visit fn: +filename: /files/1.js eslint-compat-plugin/create-once-selector + 1:1 error Program:exit visit fn: +filename: /files/1.js eslint-compat-plugin/create-once-selector + 1:1 error [body]:exit visit fn: +filename: /files/1.js eslint-compat-plugin/create-once-selector + 1:1 error [body][body][body]:exit visit fn: +filename: /files/1.js eslint-compat-plugin/create-once-selector 1:5 error ident visit fn "a": filename: /files/1.js eslint-compat-plugin/create 1:5 error ident visit fn "a": @@ -68,6 +78,16 @@ filename: /files/2.js eslint-compat-plugin filename: /files/2.js eslint-compat-plugin/create-once-after-only 0:1 error after hook: filename: /files/2.js eslint-compat-plugin/create-once-hooks-only + 0:1 error after hook: +filename: /files/2.js eslint-compat-plugin/create-once-selector + 1:1 error *:exit visit fn: +filename: /files/2.js eslint-compat-plugin/create-once-selector + 1:1 error Program:exit visit fn: +filename: /files/2.js eslint-compat-plugin/create-once-selector + 1:1 error [body]:exit visit fn: +filename: /files/2.js eslint-compat-plugin/create-once-selector + 1:1 error [body][body][body]:exit visit fn: +filename: /files/2.js eslint-compat-plugin/create-once-selector 1:5 error ident visit fn "c": filename: /files/2.js eslint-compat-plugin/create 1:5 error ident visit fn "c": @@ -95,7 +115,7 @@ filename: /files/2.js eslint-compat-plugin 1:8 error ident visit fn "d": filename: /files/2.js eslint-compat-plugin/create-once-no-hooks -✖ 39 problems (39 errors, 0 warnings) +✖ 49 problems (49 errors, 0 warnings) ``` # stderr diff --git a/apps/oxlint/test/fixtures/eslintCompat/output.snap.md b/apps/oxlint/test/fixtures/eslintCompat/output.snap.md index b38ef26091092..ed57fa5f2c15d 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/output.snap.md +++ b/apps/oxlint/test/fixtures/eslintCompat/output.snap.md @@ -48,6 +48,41 @@ : ^ `---- + x eslint-compat-plugin(create-once-selector): after hook: + | filename: /files/1.js + ,-[files/1.js:1:1] + 1 | let a, b; + : ^ + `---- + + x eslint-compat-plugin(create-once-selector): *:exit visit fn: + | filename: /files/1.js + ,-[files/1.js:1:1] + 1 | let a, b; + : ^^^^^^^^^^ + `---- + + x eslint-compat-plugin(create-once-selector): Program:exit visit fn: + | filename: /files/1.js + ,-[files/1.js:1:1] + 1 | let a, b; + : ^^^^^^^^^^ + `---- + + x eslint-compat-plugin(create-once-selector): [body]:exit visit fn: + | filename: /files/1.js + ,-[files/1.js:1:1] + 1 | let a, b; + : ^^^^^^^^^^ + `---- + + x eslint-compat-plugin(create-once-selector): [body][body][body]:exit visit fn: + | filename: /files/1.js + ,-[files/1.js:1:1] + 1 | let a, b; + : ^^^^^^^^^^ + `---- + x eslint-compat-plugin(create): ident visit fn "a": | filename: /files/1.js ,-[files/1.js:1:5] @@ -172,6 +207,41 @@ : ^ `---- + x eslint-compat-plugin(create-once-selector): after hook: + | filename: /files/2.js + ,-[files/2.js:1:1] + 1 | let c, d; + : ^ + `---- + + x eslint-compat-plugin(create-once-selector): *:exit visit fn: + | filename: /files/2.js + ,-[files/2.js:1:1] + 1 | let c, d; + : ^^^^^^^^^^ + `---- + + x eslint-compat-plugin(create-once-selector): Program:exit visit fn: + | filename: /files/2.js + ,-[files/2.js:1:1] + 1 | let c, d; + : ^^^^^^^^^^ + `---- + + x eslint-compat-plugin(create-once-selector): [body]:exit visit fn: + | filename: /files/2.js + ,-[files/2.js:1:1] + 1 | let c, d; + : ^^^^^^^^^^ + `---- + + x eslint-compat-plugin(create-once-selector): [body][body][body]:exit visit fn: + | filename: /files/2.js + ,-[files/2.js:1:1] + 1 | let c, d; + : ^^^^^^^^^^ + `---- + x eslint-compat-plugin(create): ident visit fn "c": | filename: /files/2.js ,-[files/2.js:1:5] @@ -258,8 +328,8 @@ : ^ `---- -Found 0 warnings and 35 errors. -Finished in Xms on 2 files with 7 rules using X threads. +Found 0 warnings and 45 errors. +Finished in Xms on 2 files with 8 rules using X threads. ``` # stderr diff --git a/apps/oxlint/test/fixtures/eslintCompat/plugin.ts b/apps/oxlint/test/fixtures/eslintCompat/plugin.ts index fe139ccadaee2..292650b2e311f 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/plugin.ts +++ b/apps/oxlint/test/fixtures/eslintCompat/plugin.ts @@ -108,6 +108,96 @@ const createOnceRule: Rule = { }, }; +// This tests that `after` hook runs after all visit functions, even high-specificity ones matching `Program`. +const createOnceSelectorRule: Rule = { + createOnce(context) { + // `fileNum` should be different for each file + let fileNum = 0; + // Note: Files are processed in unpredictable order, so `files/1.js` may be `fileNum` 1 or 2. + // Therefore, collect all visits and check them in `after` hook of the 2nd file. + const visits: { fileNum: number; selector: string }[] = []; + + return { + before() { + fileNum++; + }, + "*:exit"(node) { + if (node.type !== "Program") return; + + visits.push({ fileNum, selector: "*" }); + + context.report({ + message: `*:exit visit fn:\n` + `filename: ${context.filename}`, + node, + }); + }, + "Program:exit"(node) { + visits.push({ fileNum, selector: "Program" }); + + context.report({ + message: `Program:exit visit fn:\n` + `filename: ${context.filename}`, + node, + }); + }, + "[body]:exit"(node) { + visits.push({ fileNum, selector: "[body]" }); + + context.report({ + message: `[body]:exit visit fn:\n` + `filename: ${context.filename}`, + node, + }); + }, + "[body][body][body]:exit"(node) { + visits.push({ fileNum, selector: "[body][body][body]" }); + + context.report({ + message: `[body][body][body]:exit visit fn:\n` + `filename: ${context.filename}`, + node, + }); + }, + after() { + context.report({ + message: "after hook:\n" + `filename: ${context.filename}`, + node: SPAN, + }); + + visits.push({ fileNum, selector: "after" }); + + if (fileNum === 2) { + visits.sort((v1, v2) => v1.fileNum - v2.fileNum); + + const expectedVisits = [ + { fileNum: 1, selector: "*" }, + { fileNum: 1, selector: "Program" }, + { fileNum: 1, selector: "[body]" }, + { fileNum: 1, selector: "[body][body][body]" }, + { fileNum: 1, selector: "after" }, + { fileNum: 2, selector: "*" }, + { fileNum: 2, selector: "Program" }, + { fileNum: 2, selector: "[body]" }, + { fileNum: 2, selector: "[body][body][body]" }, + { fileNum: 2, selector: "after" }, + ]; + + if ( + visits.length !== expectedVisits.length || + visits.some( + (v, i) => + v.fileNum !== expectedVisits[i].fileNum || + v.selector !== expectedVisits[i].selector, + ) + ) { + context.report({ + message: `Unexpected visits: ${JSON.stringify(visits)}`, + node: SPAN, + }); + } + } + }, + }; + }, +}; + // Tests that `before` hook returning `false` disables visiting AST for the file. const createOnceBeforeFalseRule: Rule = { createOnce(context) { @@ -217,6 +307,7 @@ export default eslintCompatPlugin({ rules: { create: createRule, "create-once": createOnceRule, + "create-once-selector": createOnceSelectorRule, "create-once-before-false": createOnceBeforeFalseRule, "create-once-before-only": createOnceBeforeOnlyRule, "create-once-after-only": createOnceAfterOnlyRule, diff --git a/npm/oxlint-plugins/index.cjs b/npm/oxlint-plugins/index.cjs index 62c80d160663d..0eb0eade72989 100644 --- a/npm/oxlint-plugins/index.cjs +++ b/npm/oxlint-plugins/index.cjs @@ -88,10 +88,18 @@ function createContextAndVisitor(rule) { else if (beforeHook !== null && typeof beforeHook != "function") throw Error("`before` property of visitor must be a function if defined"); if (afterHook != null) { if (typeof afterHook != "function") throw Error("`after` property of visitor must be a function if defined"); - let programExit = visitor["Program:exit"]; - visitor["Program:exit"] = programExit == null ? (_node) => afterHook() : (node) => { - programExit(node), afterHook(); - }; + let maxAttrs = -1; + for (let key in visitor) { + if (!Object.hasOwn(visitor, key) || !key.endsWith(":exit")) continue; + let end = key.length - 5, count = 0; + for (let i = 0; i < end; i++) { + let c = key.charCodeAt(i); + (c === 91 || c === 46 || c === 58) && count++; + } + count > maxAttrs && (maxAttrs = count); + } + let key = `Program${"[type]".repeat(maxAttrs + 1)}:exit`; + visitor[key] = (_node) => afterHook(); } return { context, diff --git a/npm/oxlint-plugins/index.js b/npm/oxlint-plugins/index.js index a40291f9819fd..6ac5b4b92ce3e 100644 --- a/npm/oxlint-plugins/index.js +++ b/npm/oxlint-plugins/index.js @@ -88,10 +88,18 @@ function createContextAndVisitor(rule) { else if (beforeHook !== null && typeof beforeHook != "function") throw Error("`before` property of visitor must be a function if defined"); if (afterHook != null) { if (typeof afterHook != "function") throw Error("`after` property of visitor must be a function if defined"); - let programExit = visitor["Program:exit"]; - visitor["Program:exit"] = programExit == null ? (_node) => afterHook() : (node) => { - programExit(node), afterHook(); - }; + let maxAttrs = -1; + for (let key in visitor) { + if (!Object.hasOwn(visitor, key) || !key.endsWith(":exit")) continue; + let end = key.length - 5, count = 0; + for (let i = 0; i < end; i++) { + let c = key.charCodeAt(i); + (c === 91 || c === 46 || c === 58) && count++; + } + count > maxAttrs && (maxAttrs = count); + } + let key = `Program${"[type]".repeat(maxAttrs + 1)}:exit`; + visitor[key] = (_node) => afterHook(); } return { context,