Skip to content

Commit 7bd01ed

Browse files
overlookmoteloverlookmotel
committed
fix(linter/plugins): defineRule call createOnce lazily (#14062)
Previously `defineRule`, when passed a rule with a `createOnce` method, would call `createOnce` immediately. When the rule is used in Oxlint, this resulted in `createOnce` being called twice. `createOnce` might do some fairly expensive set-up work, so we only want to call it once (as the name suggests!). Instead, `defineRule` don't call `createOnce` immediately. Call it the first time the `create` method which `defineRule` creates is called. This means that, when using Oxlint: 1. `createOnce` is only called once. 2. `defineRule` call is cheaper. i.e. the cost of making the rule ESLint-compatible is only paid when the user is running the rule in ESLint, not in Oxlint.
1 parent f2b3934 commit 7bd01ed

File tree

6 files changed

+117
-71
lines changed

6 files changed

+117
-71
lines changed

‎apps/oxlint/src-js/index.ts‎

Lines changed: 50 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { Context } from './plugins/context.ts';
2-
import type { Plugin, Rule } from './plugins/load.ts';
2+
import type { CreateOnceRule, Plugin, Rule } from './plugins/load.ts';
3+
import type { BeforeHook, Visitor } from './plugins/types.ts';
34

45
const { defineProperty, getPrototypeOf, setPrototypeOf } = Object;
56

@@ -17,7 +18,48 @@ export function defineRule(rule: Rule): Rule {
1718
if (!('createOnce' in rule)) return rule;
1819
if ('create' in rule) throw new Error('Rules must define only `create` or `createOnce` methods, not both');
1920

20-
// Run `createOnce` with empty context object.
21+
// Add `create` function to `rule`
22+
let context: Context = null, visitor: Visitor, beforeHook: BeforeHook | null;
23+
24+
rule.create = (eslintContext) => {
25+
// Lazily call `createOnce` on first invocation of `create`
26+
if (context === null) {
27+
({ context, visitor, beforeHook } = createContextAndVisitor(rule));
28+
}
29+
30+
// Copy properties from ESLint's context object to `context`.
31+
// ESLint's context object is an object of form `{ id, options, report }`, with all other properties
32+
// and methods on another object which is its prototype.
33+
defineProperty(context, 'id', { value: eslintContext.id });
34+
defineProperty(context, 'options', { value: eslintContext.options });
35+
defineProperty(context, 'report', { value: eslintContext.report });
36+
setPrototypeOf(context, getPrototypeOf(eslintContext));
37+
38+
// If `before` hook returns `false`, skip traversal by returning an empty object as visitor
39+
if (beforeHook !== null) {
40+
const shouldRun = beforeHook();
41+
if (shouldRun === false) return {};
42+
}
43+
44+
// Return same visitor each time
45+
return visitor;
46+
};
47+
48+
return rule;
49+
}
50+
51+
/**
52+
* Call `createOnce` method of rule, and return `Context`, `Visitor`, and `beforeHook` (if any).
53+
*
54+
* @param rule - Rule with `createOnce` method
55+
* @returns Object with `context`, `visitor`, and `beforeHook` properties
56+
*/
57+
function createContextAndVisitor(rule: CreateOnceRule): {
58+
context: Context;
59+
visitor: Visitor;
60+
beforeHook: BeforeHook | null;
61+
} {
62+
// Call `createOnce` with empty context object.
2163
// Really, `context` should be an instance of `Context`, which would throw error on accessing e.g. `id`
2264
// in body of `createOnce`. But any such bugs should have been caught when testing the rule in Oxlint,
2365
// so should be OK to take this shortcut.
@@ -27,7 +69,7 @@ export function defineRule(rule: Rule): Rule {
2769
report: { value: dummyReport, enumerable: true, configurable: true },
2870
});
2971

30-
let { before: beforeHook, after: afterHook, ...visitor } = rule.createOnce(context as Context);
72+
let { before: beforeHook, after: afterHook, ...visitor } = rule.createOnce(context);
3173

3274
if (beforeHook === void 0) {
3375
beforeHook = null;
@@ -40,33 +82,13 @@ export function defineRule(rule: Rule): Rule {
4082
if (typeof afterHook !== 'function') throw new Error('`after` property of visitor must be a function if defined');
4183

4284
const programExit = visitor['Program:exit'];
43-
visitor['Program:exit'] = programExit
44-
? (node) => {
85+
visitor['Program:exit'] = programExit == null
86+
? (_node) => afterHook()
87+
: (node) => {
4588
programExit(node);
4689
afterHook();
47-
}
48-
: (_node) => afterHook();
90+
};
4991
}
5092

51-
// Create `create` function
52-
rule.create = (eslintContext) => {
53-
// Copy properties from ESLint's context object to `context`.
54-
// ESLint's context object is an object of form `{ id, options, report }`, with all other properties
55-
// and methods on another object which is its prototype.
56-
defineProperty(context, 'id', { value: eslintContext.id });
57-
defineProperty(context, 'options', { value: eslintContext.options });
58-
defineProperty(context, 'report', { value: eslintContext.report });
59-
setPrototypeOf(context, getPrototypeOf(eslintContext));
60-
61-
// If `before` hook returns `false`, skip rest of traversal by returning an empty object as visitor
62-
if (beforeHook !== null) {
63-
const shouldRun = beforeHook();
64-
if (shouldRun === false) return {};
65-
}
66-
67-
// Return same visitor each time
68-
return visitor;
69-
};
70-
71-
return rule;
93+
return { context, visitor, beforeHook };
7294
}

‎apps/oxlint/src-js/plugins/load.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ interface CreateRule {
2222
create: (context: Context) => Visitor;
2323
}
2424

25-
interface CreateOnceRule {
25+
export interface CreateOnceRule {
2626
create?: (context: Context) => Visitor;
2727
createOnce: (context: Context) => VisitorWithHooks;
2828
}

‎apps/oxlint/test/__snapshots__/e2e.test.ts.snap‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -652,6 +652,12 @@ exports[`oxlint CLI > should support \`createOnce\` 1`] = `
652652
: ^
653653
\`----
654654
655+
x create-once-plugin(always-run): createOnce: call count: 1
656+
,-[files/1.js:1:1]
657+
1 | let x;
658+
: ^
659+
\`----
660+
655661
x create-once-plugin(always-run): createOnce: filename: Cannot access \`context.filename\` in \`createOnce\`
656662
,-[files/1.js:1:1]
657663
1 | let x;
@@ -772,6 +778,12 @@ exports[`oxlint CLI > should support \`createOnce\` 1`] = `
772778
: ^
773779
\`----
774780
781+
x create-once-plugin(always-run): createOnce: call count: 1
782+
,-[files/2.js:1:1]
783+
1 | let y;
784+
: ^
785+
\`----
786+
775787
x create-once-plugin(always-run): createOnce: filename: Cannot access \`context.filename\` in \`createOnce\`
776788
,-[files/2.js:1:1]
777789
1 | let y;
@@ -880,7 +892,7 @@ exports[`oxlint CLI > should support \`createOnce\` 1`] = `
880892
: ^
881893
\`----
882894
883-
Found 0 warnings and 40 errors.
895+
Found 0 warnings and 42 errors.
884896
Finished in Xms on 2 files using X threads."
885897
`;
886898
@@ -893,16 +905,17 @@ exports[`oxlint CLI > should support \`defineRule\` + \`definePlugin\` 1`] = `
893905
: ^
894906
\`----
895907
896-
x define-rule-plugin(create-once): after hook:
897-
| identNum: 2
908+
x define-rule-plugin(create-once): before hook:
909+
| createOnce call count: 1
910+
| this === rule: true
898911
| filename: files/1.js
899912
,-[files/1.js:1:1]
900913
1 | let a, b;
901914
: ^
902915
\`----
903916
904-
x define-rule-plugin(create-once): before hook:
905-
| this === rule: true
917+
x define-rule-plugin(create-once): after hook:
918+
| identNum: 2
906919
| filename: files/1.js
907920
,-[files/1.js:1:1]
908921
1 | let a, b;
@@ -1009,16 +1022,17 @@ exports[`oxlint CLI > should support \`defineRule\` + \`definePlugin\` 1`] = `
10091022
: ^
10101023
\`----
10111024
1012-
x define-rule-plugin(create-once): after hook:
1013-
| identNum: 2
1025+
x define-rule-plugin(create-once): before hook:
1026+
| createOnce call count: 1
1027+
| this === rule: true
10141028
| filename: files/2.js
10151029
,-[files/2.js:1:1]
10161030
1 | let c, d;
10171031
: ^
10181032
\`----
10191033
1020-
x define-rule-plugin(create-once): before hook:
1021-
| this === rule: true
1034+
x define-rule-plugin(create-once): after hook:
1035+
| identNum: 2
10221036
| filename: files/2.js
10231037
,-[files/2.js:1:1]
10241038
1 | let c, d;

‎apps/oxlint/test/__snapshots__/eslint-compat.test.ts.snap‎

Lines changed: 35 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -4,85 +4,87 @@ exports[`ESLint compatibility > \`defineRule\` + \`definePlugin\` should work 1`
44
"
55
<root>/apps/oxlint/test/fixtures/define/files/1.js
66
0:1 error create body:
7-
this === rule: true define-rule-plugin/create
7+
this === rule: true define-rule-plugin/create
88
0:1 error before hook:
9+
createOnce call count: 1
910
this === rule: true
1011
filename: files/1.js define-rule-plugin/create-once
1112
0:1 error before hook:
12-
filename: files/1.js define-rule-plugin/create-once-before-false
13+
filename: files/1.js define-rule-plugin/create-once-before-false
1314
0:1 error before hook:
14-
filename: files/1.js define-rule-plugin/create-once-before-only
15+
filename: files/1.js define-rule-plugin/create-once-before-only
1516
0:1 error after hook:
1617
identNum: 2
17-
filename: files/1.js define-rule-plugin/create-once
18+
filename: files/1.js define-rule-plugin/create-once
1819
0:1 error after hook:
19-
filename: files/1.js define-rule-plugin/create-once-after-only
20+
filename: files/1.js define-rule-plugin/create-once-after-only
2021
1:5 error ident visit fn "a":
21-
filename: files/1.js define-rule-plugin/create
22+
filename: files/1.js define-rule-plugin/create
2223
1:5 error ident visit fn "a":
2324
identNum: 1
24-
filename: files/1.js define-rule-plugin/create-once
25+
filename: files/1.js define-rule-plugin/create-once
2526
1:5 error ident visit fn "a":
26-
filename: files/1.js define-rule-plugin/create-once-before-only
27+
filename: files/1.js define-rule-plugin/create-once-before-only
2728
1:5 error ident visit fn "a":
28-
filename: files/1.js define-rule-plugin/create-once-after-only
29+
filename: files/1.js define-rule-plugin/create-once-after-only
2930
1:5 error ident visit fn "a":
30-
filename: files/1.js define-rule-plugin/create-once-no-hooks
31+
filename: files/1.js define-rule-plugin/create-once-no-hooks
3132
1:8 error ident visit fn "b":
32-
filename: files/1.js define-rule-plugin/create
33+
filename: files/1.js define-rule-plugin/create
3334
1:8 error ident visit fn "b":
3435
identNum: 2
35-
filename: files/1.js define-rule-plugin/create-once
36+
filename: files/1.js define-rule-plugin/create-once
3637
1:8 error ident visit fn "b":
37-
filename: files/1.js define-rule-plugin/create-once-before-only
38+
filename: files/1.js define-rule-plugin/create-once-before-only
3839
1:8 error ident visit fn "b":
39-
filename: files/1.js define-rule-plugin/create-once-after-only
40+
filename: files/1.js define-rule-plugin/create-once-after-only
4041
1:8 error ident visit fn "b":
41-
filename: files/1.js define-rule-plugin/create-once-no-hooks
42+
filename: files/1.js define-rule-plugin/create-once-no-hooks
4243
4344
<root>/apps/oxlint/test/fixtures/define/files/2.js
4445
0:1 error create body:
45-
this === rule: true define-rule-plugin/create
46+
this === rule: true define-rule-plugin/create
4647
0:1 error before hook:
48+
createOnce call count: 1
4749
this === rule: true
4850
filename: files/2.js define-rule-plugin/create-once
4951
0:1 error before hook:
50-
filename: files/2.js define-rule-plugin/create-once-before-false
52+
filename: files/2.js define-rule-plugin/create-once-before-false
5153
0:1 error before hook:
52-
filename: files/2.js define-rule-plugin/create-once-before-only
54+
filename: files/2.js define-rule-plugin/create-once-before-only
5355
0:1 error after hook:
5456
identNum: 2
55-
filename: files/2.js define-rule-plugin/create-once
57+
filename: files/2.js define-rule-plugin/create-once
5658
0:1 error after hook:
57-
filename: files/2.js define-rule-plugin/create-once-before-false
59+
filename: files/2.js define-rule-plugin/create-once-before-false
5860
0:1 error after hook:
59-
filename: files/2.js define-rule-plugin/create-once-after-only
61+
filename: files/2.js define-rule-plugin/create-once-after-only
6062
1:5 error ident visit fn "c":
61-
filename: files/2.js define-rule-plugin/create
63+
filename: files/2.js define-rule-plugin/create
6264
1:5 error ident visit fn "c":
6365
identNum: 1
64-
filename: files/2.js define-rule-plugin/create-once
66+
filename: files/2.js define-rule-plugin/create-once
6567
1:5 error ident visit fn "c":
66-
filename: files/2.js define-rule-plugin/create-once-before-false
68+
filename: files/2.js define-rule-plugin/create-once-before-false
6769
1:5 error ident visit fn "c":
68-
filename: files/2.js define-rule-plugin/create-once-before-only
70+
filename: files/2.js define-rule-plugin/create-once-before-only
6971
1:5 error ident visit fn "c":
70-
filename: files/2.js define-rule-plugin/create-once-after-only
72+
filename: files/2.js define-rule-plugin/create-once-after-only
7173
1:5 error ident visit fn "c":
72-
filename: files/2.js define-rule-plugin/create-once-no-hooks
74+
filename: files/2.js define-rule-plugin/create-once-no-hooks
7375
1:8 error ident visit fn "d":
74-
filename: files/2.js define-rule-plugin/create
76+
filename: files/2.js define-rule-plugin/create
7577
1:8 error ident visit fn "d":
7678
identNum: 2
77-
filename: files/2.js define-rule-plugin/create-once
79+
filename: files/2.js define-rule-plugin/create-once
7880
1:8 error ident visit fn "d":
79-
filename: files/2.js define-rule-plugin/create-once-before-false
81+
filename: files/2.js define-rule-plugin/create-once-before-false
8082
1:8 error ident visit fn "d":
81-
filename: files/2.js define-rule-plugin/create-once-before-only
83+
filename: files/2.js define-rule-plugin/create-once-before-only
8284
1:8 error ident visit fn "d":
83-
filename: files/2.js define-rule-plugin/create-once-after-only
85+
filename: files/2.js define-rule-plugin/create-once-after-only
8486
1:8 error ident visit fn "d":
85-
filename: files/2.js define-rule-plugin/create-once-no-hooks
87+
filename: files/2.js define-rule-plugin/create-once-no-hooks
8688
8789
✖ 35 problems (35 errors, 0 warnings)
8890
"

‎apps/oxlint/test/fixtures/createOnce/test_plugin/index.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,11 @@ const relativePath = sep === '/'
88
? path => path.slice(PARENT_DIR_PATH_LEN)
99
: path => path.slice(PARENT_DIR_PATH_LEN).replace(/\\/g, '/');
1010

11+
let createOnceCallCount = 0;
1112
const alwaysRunRule = {
1213
createOnce(context) {
14+
createOnceCallCount++;
15+
1316
const topLevelThis = this;
1417

1518
// Check that these APIs throw here
@@ -21,6 +24,7 @@ const alwaysRunRule = {
2124

2225
return {
2326
before() {
27+
context.report({ message: `createOnce: call count: ${createOnceCallCount}`, node: SPAN });
2428
context.report({ message: `createOnce: this === rule: ${topLevelThis === alwaysRunRule}`, node: SPAN });
2529
context.report({ message: `createOnce: id: ${idError?.message}`, node: SPAN });
2630
context.report({ message: `createOnce: filename: ${filenameError?.message}`, node: SPAN });

‎apps/oxlint/test/fixtures/define/test_plugin/index.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,11 @@ const createRule = defineRule({
3535

3636
// This aims to test that `createOnce` is called once only, and `before` hook is called once per file.
3737
// i.e. Oxlint calls `createOnce` directly, and not the `create` method that `defineRule` adds to the rule.
38+
let createOnceCallCount = 0;
3839
const createOnceRule = defineRule({
3940
createOnce(context) {
41+
createOnceCallCount++;
42+
4043
// `fileNum` should be different for each file.
4144
// `identNum` should start at 1 for each file.
4245
let fileNum = 0, identNum;
@@ -54,6 +57,7 @@ const createOnceRule = defineRule({
5457

5558
context.report({
5659
message: 'before hook:\n'
60+
+ `createOnce call count: ${createOnceCallCount}\n`
5761
+ `this === rule: ${topLevelThis === createOnceRule}\n`
5862
+ `filename: ${relativePath(context.filename)}`,
5963
node: SPAN,

0 commit comments

Comments
 (0)