Skip to content

fix(linter/plugins): ensure after hook always runs last in rule converted for ESLint #18904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
graphite-app merged 1 commit into from
Feb 3, 2026
Merged

fix(linter/plugins): ensure after hook always runs last in rule converted for ESLint #18904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 53 additions & 8 deletions apps/oxlint/src-js/package/compat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down
1 change: 1 addition & 0 deletions apps/oxlint/test/fixtures/eslintCompat/.oxlintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions apps/oxlint/test/fixtures/eslintCompat/eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
22 changes: 21 additions & 1 deletion apps/oxlint/test/fixtures/eslintCompat/eslint.snap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ filename: <fixture>/files/1.js eslint-compat-plugin
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-after-only
0:1 error after hook:
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-hooks-only
0:1 error after hook:
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-selector
1:1 error *:exit visit fn:
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-selector
1:1 error Program:exit visit fn:
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-selector
1:1 error [body]:exit visit fn:
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-selector
1:1 error [body][body][body]:exit visit fn:
filename: <fixture>/files/1.js eslint-compat-plugin/create-once-selector
1:5 error ident visit fn "a":
filename: <fixture>/files/1.js eslint-compat-plugin/create
1:5 error ident visit fn "a":
Expand Down Expand Up @@ -68,6 +78,16 @@ filename: <fixture>/files/2.js eslint-compat-plugin
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-after-only
0:1 error after hook:
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-hooks-only
0:1 error after hook:
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-selector
1:1 error *:exit visit fn:
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-selector
1:1 error Program:exit visit fn:
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-selector
1:1 error [body]:exit visit fn:
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-selector
1:1 error [body][body][body]:exit visit fn:
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-selector
1:5 error ident visit fn "c":
filename: <fixture>/files/2.js eslint-compat-plugin/create
1:5 error ident visit fn "c":
Expand Down Expand Up @@ -95,7 +115,7 @@ filename: <fixture>/files/2.js eslint-compat-plugin
1:8 error ident visit fn "d":
filename: <fixture>/files/2.js eslint-compat-plugin/create-once-no-hooks

✖ 39 problems (39 errors, 0 warnings)
✖ 49 problems (49 errors, 0 warnings)
```

# stderr
Expand Down
74 changes: 72 additions & 2 deletions apps/oxlint/test/fixtures/eslintCompat/output.snap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,41 @@
: ^
`----

x eslint-compat-plugin(create-once-selector): after hook:
| filename: <fixture>/files/1.js
,-[files/1.js:1:1]
1 | let a, b;
: ^
`----

x eslint-compat-plugin(create-once-selector): *:exit visit fn:
| filename: <fixture>/files/1.js
,-[files/1.js:1:1]
1 | let a, b;
: ^^^^^^^^^^
`----

x eslint-compat-plugin(create-once-selector): Program:exit visit fn:
| filename: <fixture>/files/1.js
,-[files/1.js:1:1]
1 | let a, b;
: ^^^^^^^^^^
`----

x eslint-compat-plugin(create-once-selector): [body]:exit visit fn:
| filename: <fixture>/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: <fixture>/files/1.js
,-[files/1.js:1:1]
1 | let a, b;
: ^^^^^^^^^^
`----

x eslint-compat-plugin(create): ident visit fn "a":
| filename: <fixture>/files/1.js
,-[files/1.js:1:5]
Expand Down Expand Up @@ -172,6 +207,41 @@
: ^
`----

x eslint-compat-plugin(create-once-selector): after hook:
| filename: <fixture>/files/2.js
,-[files/2.js:1:1]
1 | let c, d;
: ^
`----

x eslint-compat-plugin(create-once-selector): *:exit visit fn:
| filename: <fixture>/files/2.js
,-[files/2.js:1:1]
1 | let c, d;
: ^^^^^^^^^^
`----

x eslint-compat-plugin(create-once-selector): Program:exit visit fn:
| filename: <fixture>/files/2.js
,-[files/2.js:1:1]
1 | let c, d;
: ^^^^^^^^^^
`----

x eslint-compat-plugin(create-once-selector): [body]:exit visit fn:
| filename: <fixture>/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: <fixture>/files/2.js
,-[files/2.js:1:1]
1 | let c, d;
: ^^^^^^^^^^
`----

x eslint-compat-plugin(create): ident visit fn "c":
| filename: <fixture>/files/2.js
,-[files/2.js:1:5]
Expand Down Expand Up @@ -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
Expand Down
91 changes: 91 additions & 0 deletions apps/oxlint/test/fixtures/eslintCompat/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
16 changes: 12 additions & 4 deletions npm/oxlint-plugins/index.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 12 additions & 4 deletions npm/oxlint-plugins/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading