From 31145a9c6de6f52f163f4aeee971cc961b8d54f9 Mon Sep 17 00:00:00 2001 From: overlookmotel <557937+overlookmotel@users.noreply.github.com> Date: Wed, 25 Mar 2026 01:02:05 +0000 Subject: [PATCH] fix(linter/plugins): fire `after` hook after CFG events in ESLint compat (#20720) Fix a bug in `eslintCompatPlugin`. `after` hooks should fire after all other visit functions, including CFG event handlers. Previously `eslintCompatPlugin` added a `Program:exit` visit function to run the `after` hook, but it turns out this isn't the last to fire during AST visitation - last is the `onCodePathEnd` CFG event on `Program` node. Fix this by running `after` hook in `onCodePathEnd` event handler instead. This simplifies the code somewhat as we no longer need to make sure that the visit function's key is a selector with higher specificity than all other `:exit` visit functions. The downside is that the `onCodePathEnd` handler needs to check each time that the node it's called with is root `Program` node. But we make that check as cheap as possible by keeping a reference to `Program` node in a local var. --- apps/oxlint/src-js/package/compat.ts | 107 ++++++++---------- .../test/fixtures/eslintCompat/.oxlintrc.json | 31 +++-- .../fixtures/eslintCompat/eslint.config.js | 11 +- .../test/fixtures/eslintCompat/eslint.snap.md | 14 ++- .../test/fixtures/eslintCompat/files/cfg.js | 4 + .../test/fixtures/eslintCompat/output.snap.md | 12 +- .../test/fixtures/eslintCompat/plugin.ts | 79 ++++++++++++- 7 files changed, 180 insertions(+), 78 deletions(-) create mode 100644 apps/oxlint/test/fixtures/eslintCompat/files/cfg.js diff --git a/apps/oxlint/src-js/package/compat.ts b/apps/oxlint/src-js/package/compat.ts index a99d99929e950..52a9b2ffe0698 100644 --- a/apps/oxlint/src-js/package/compat.ts +++ b/apps/oxlint/src-js/package/compat.ts @@ -10,6 +10,7 @@ import type { CreateOnceRule, Plugin, Rule } from "../plugins/load.ts"; import type { Settings } from "../plugins/settings.ts"; import type { SourceCode } from "../plugins/source_code.ts"; import type { BeforeHook, Visitor, VisitorWithHooks } from "../plugins/types.ts"; +import type { Program, Node as ESTreeNode } from "../generated/types.d.ts"; import type { SetNullable } from "../utils/types.ts"; // Empty visitor object, returned by `create` when `before` hook returns `false`. @@ -63,12 +64,13 @@ function convertRule(rule: Rule) { // Add `create` function to `rule` let context: Context | null = null, visitor: Visitor | undefined, - beforeHook: BeforeHook | null; + beforeHook: BeforeHook | null, + setupAfterHook: ((program: Program) => void) | null; rule.create = (eslintContext) => { // Lazily call `createOnce` on first invocation of `create` if (context === null) { - ({ context, visitor, beforeHook } = createContextAndVisitor(rule)); + ({ context, visitor, beforeHook, setupAfterHook } = createContextAndVisitor(rule)); } debugAssertIsNonNull(visitor); @@ -80,7 +82,8 @@ function convertRule(rule: Rule) { options: { value: eslintContext.options }, report: { value: eslintContext.report }, }); - Object.setPrototypeOf(context, Object.getPrototypeOf(eslintContext)); + const eslintFileContext = Object.getPrototypeOf(eslintContext); + Object.setPrototypeOf(context, eslintFileContext); // If `before` hook returns `false`, skip traversal by returning an empty object as visitor if (beforeHook !== null) { @@ -88,6 +91,9 @@ function convertRule(rule: Rule) { if (shouldRun === false) return EMPTY_VISITOR; } + // If there's an `after` hook, call `setupAfterHook` with the `Program` node of the current file + if (setupAfterHook !== null) setupAfterHook(eslintFileContext.sourceCode.ast); + // Return same visitor each time return visitor; }; @@ -159,12 +165,14 @@ const FILE_CONTEXT: FileContext = Object.freeze({ * Call `createOnce` method of rule, and return `Context`, `Visitor`, and `beforeHook` (if any). * * @param rule - Rule with `createOnce` method - * @returns Object with `context`, `visitor`, and `beforeHook` properties + * @returns Object with `context`, `visitor`, and `beforeHook` properties, + * and `setupAfterHook` function if visitor has an `after` hook */ function createContextAndVisitor(rule: CreateOnceRule): { context: Context; visitor: Visitor; beforeHook: BeforeHook | null; + setupAfterHook: ((program: Program) => void) | null; } { // Validate type of `createOnce` const { createOnce } = rule; @@ -201,66 +209,47 @@ function createContextAndVisitor(rule: CreateOnceRule): { throw new Error("`before` property of visitor must be a function if defined"); } - // Add `after` hook to `Program:exit` visit fn + // Handle `after` hook + let setupAfterHook: ((program: Program) => void) | null = null; + if (afterHook != null) { if (typeof afterHook !== "function") { throw new Error("`after` property of visitor must be a function if defined"); } - // 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(); + let program: Program | null = null; + + // Pass function to populate `program` var back out to the `create` function generated by `convertRule`. + // `create` will call this function at the start of linting each file. + // Having `program` in a local var makes the `node === program` check in `onCodePathEnd` as cheap as it can be. + // Otherwise it'd have to be `node === context.sourceCode.ast`, which would be slower. + setupAfterHook = (ast: Program) => { + program = ast; + }; + + // Add `onCodePathEnd` CFG event handler to run `after` hook at end of AST traversal. + // This fires after all visit fns have been called (after `Program:exit`), and after all other CFG event handlers. + type CodePathHandler = (this: Visitor, codePath: unknown, node: ESTreeNode) => void; + + const onCodePathEnd = visitor.onCodePathEnd as CodePathHandler | null | undefined; + + (visitor as unknown as { onCodePathEnd: CodePathHandler }).onCodePathEnd = + onCodePathEnd == null + ? function (this: Visitor, _codePath: unknown, node: ESTreeNode) { + if (node === program) { + program = null; + afterHook(); + } + } + : function (this: Visitor, codePath: unknown, node: ESTreeNode) { + onCodePathEnd.call(this, codePath, node); + + if (node === program) { + program = null; + afterHook(); + } + }; } - return { context, visitor, beforeHook }; + return { context, visitor, beforeHook, setupAfterHook }; } diff --git a/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json b/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json index 635b803224e70..76e808fb6af5c 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json +++ b/apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json @@ -1,14 +1,25 @@ { "jsPlugins": ["./plugin.ts"], "categories": { "correctness": "off" }, - "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", - "eslint-compat-plugin/create-once-hooks-only": "error", - "eslint-compat-plugin/create-once-no-hooks": "error" - } + "overrides": [ + { + "files": ["files/1.js", "files/2.js"], + "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", + "eslint-compat-plugin/create-once-hooks-only": "error", + "eslint-compat-plugin/create-once-no-hooks": "error" + } + }, + { + "files": ["files/cfg.js"], + "rules": { + "eslint-compat-plugin/create-once-cfg": "error" + } + } + ] } diff --git a/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js b/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js index 852e14f1fbe52..da470a3c2a65a 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js +++ b/apps/oxlint/test/fixtures/eslintCompat/eslint.config.js @@ -2,7 +2,7 @@ import plugin from "./plugin.ts"; export default [ { - files: ["files/*.js"], + files: ["files/1.js", "files/2.js"], plugins: { "eslint-compat-plugin": plugin, }, @@ -17,4 +17,13 @@ export default [ "eslint-compat-plugin/create-once-no-hooks": "error", }, }, + { + files: ["files/cfg.js"], + plugins: { + "eslint-compat-plugin": plugin, + }, + rules: { + "eslint-compat-plugin/create-once-cfg": "error", + }, + }, ]; diff --git a/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md b/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md index 2ae2280bd4424..906342715d564 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md +++ b/apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md @@ -20,11 +20,11 @@ filename: /files/1.js eslint-compat-plugin identNum: 2 filename: /files/1.js eslint-compat-plugin/create-once 0:1 error after hook: +filename: /files/1.js eslint-compat-plugin/create-once-selector + 0:1 error after hook: 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: @@ -73,13 +73,13 @@ filename: /files/2.js eslint-compat-plugin identNum: 2 filename: /files/2.js eslint-compat-plugin/create-once 0:1 error after hook: +filename: /files/2.js eslint-compat-plugin/create-once-selector + 0:1 error after hook: filename: /files/2.js eslint-compat-plugin/create-once-before-false 0:1 error after hook: 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: @@ -115,7 +115,11 @@ 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 -✖ 49 problems (49 errors, 0 warnings) +/files/cfg.js + 0:1 error after hook: +filename: /files/cfg.js eslint-compat-plugin/create-once-cfg + +✖ 50 problems (50 errors, 0 warnings) ``` # stderr diff --git a/apps/oxlint/test/fixtures/eslintCompat/files/cfg.js b/apps/oxlint/test/fixtures/eslintCompat/files/cfg.js new file mode 100644 index 0000000000000..8627456076639 --- /dev/null +++ b/apps/oxlint/test/fixtures/eslintCompat/files/cfg.js @@ -0,0 +1,4 @@ +for (let i = 0; i < 3; i++) { + console.log(i); + break; +} diff --git a/apps/oxlint/test/fixtures/eslintCompat/output.snap.md b/apps/oxlint/test/fixtures/eslintCompat/output.snap.md index 26b91c847876b..774713bb05736 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/output.snap.md +++ b/apps/oxlint/test/fixtures/eslintCompat/output.snap.md @@ -328,8 +328,16 @@ : ^ `---- -Found 0 warnings and 45 errors. -Finished in Xms on 2 files with 8 rules using X threads. + x eslint-compat-plugin(create-once-cfg): after hook: + | filename: /files/cfg.js + ,-[files/cfg.js:1:1] + 1 | for (let i = 0; i < 3; i++) { + : ^ + 2 | console.log(i); + `---- + +Found 0 warnings and 46 errors. +Finished in Xms on 3 files with 0 rules using X threads. ``` # stderr diff --git a/apps/oxlint/test/fixtures/eslintCompat/plugin.ts b/apps/oxlint/test/fixtures/eslintCompat/plugin.ts index 292650b2e311f..605c26f9f49e9 100644 --- a/apps/oxlint/test/fixtures/eslintCompat/plugin.ts +++ b/apps/oxlint/test/fixtures/eslintCompat/plugin.ts @@ -1,6 +1,8 @@ import { eslintCompatPlugin } from "#oxlint/plugins"; -import type { Node, Rule } from "#oxlint/plugins"; +import type { Node, Rule, Visitor, ESTree } from "#oxlint/plugins"; + +type ESTreeNode = ESTree.Node; const SPAN: Node = { start: 0, @@ -198,6 +200,80 @@ const createOnceSelectorRule: Rule = { }, }; +// This tests that `after` hook runs after all CFG event handlers. +// This rules is only run on `files/cfg.js`, to ensure CFG events handlers do not affect behavior of other rules +// which don't use CFG event listeners. +const createOnceCfgRule: Rule = { + createOnce(context) { + // Collect all visits and check them in `after` hook + const visits: { event: string; nodeType?: string }[] = []; + + return { + onCodePathStart(_codePath: unknown, node: ESTreeNode) { + visits.push({ event: "onCodePathStart", nodeType: node.type }); + }, + onCodePathEnd(_codePath: unknown, node: ESTreeNode) { + visits.push({ event: "onCodePathEnd", nodeType: node.type }); + }, + onCodePathSegmentStart(_segment: unknown, node: ESTreeNode) { + visits.push({ event: "onCodePathSegmentStart", nodeType: node.type }); + }, + onCodePathSegmentEnd(_segment: unknown, node: ESTreeNode) { + visits.push({ event: "onCodePathSegmentEnd", nodeType: node.type }); + }, + onUnreachableCodePathSegmentStart(_segment: unknown, node: ESTreeNode) { + visits.push({ event: "onUnreachableCodePathSegmentStart", nodeType: node.type }); + }, + onUnreachableCodePathSegmentEnd(_segment: unknown, node: ESTreeNode) { + visits.push({ event: "onUnreachableCodePathSegmentEnd", nodeType: node.type }); + }, + onCodePathSegmentLoop(_fromSegment: unknown, _toSegment: unknown, node: ESTreeNode) { + visits.push({ event: "onCodePathSegmentLoop", nodeType: node.type }); + }, + after() { + context.report({ + message: "after hook:\n" + `filename: ${context.filename}`, + node: SPAN, + }); + + visits.push({ event: "after" }); + + const expectedVisits: typeof visits = [ + { event: "onCodePathStart", nodeType: "Program" }, + { event: "onCodePathSegmentStart", nodeType: "Program" }, + { event: "onCodePathSegmentEnd", nodeType: "BinaryExpression" }, + { event: "onCodePathSegmentStart", nodeType: "BinaryExpression" }, + { event: "onCodePathSegmentEnd", nodeType: "UpdateExpression" }, + { event: "onCodePathSegmentStart", nodeType: "UpdateExpression" }, + { event: "onCodePathSegmentLoop", nodeType: "BlockStatement" }, + { event: "onCodePathSegmentEnd", nodeType: "BlockStatement" }, + { event: "onCodePathSegmentStart", nodeType: "BlockStatement" }, + { event: "onCodePathSegmentEnd", nodeType: "BlockStatement" }, + { event: "onUnreachableCodePathSegmentStart", nodeType: "BlockStatement" }, + { event: "onUnreachableCodePathSegmentEnd", nodeType: "ForStatement" }, + { event: "onCodePathSegmentStart", nodeType: "ForStatement" }, + { event: "onCodePathSegmentEnd", nodeType: "Program" }, + { event: "onCodePathEnd", nodeType: "Program" }, + { event: "after" }, + ]; + + if ( + visits.length !== expectedVisits.length || + visits.some( + (v, i) => + v.event !== expectedVisits[i].event || v.nodeType !== expectedVisits[i].nodeType, + ) + ) { + context.report({ + message: `Unexpected visits:\n${JSON.stringify(visits, null, 2)}`, + node: SPAN, + }); + } + }, + } as unknown as Visitor; // TODO: Our types don't include CFG event handlers at present + }, +}; + // Tests that `before` hook returning `false` disables visiting AST for the file. const createOnceBeforeFalseRule: Rule = { createOnce(context) { @@ -308,6 +384,7 @@ export default eslintCompatPlugin({ create: createRule, "create-once": createOnceRule, "create-once-selector": createOnceSelectorRule, + "create-once-cfg": createOnceCfgRule, "create-once-before-false": createOnceBeforeFalseRule, "create-once-before-only": createOnceBeforeOnlyRule, "create-once-after-only": createOnceAfterOnlyRule,