Skip to content

Commit 591d87b

Browse files
overlookmoteloverlookmotel
committed
refactor(napi/oxlint): remove classes (#12454)
Flatten code in `napi/oxlint2`, removing the `PluginRegistry` and `Linter` classes. Only a single instance of these classes was created, so the classes added nothing in terms of utility, and added a (small) perf cost. The classes *did* provide a nice encapsulation of concerns, but in my opinion our aim with the JS code here should be performance above all else. I hope we can avoid the code descending into a spaghetti monstrosity by breaking things up into separate files and with good documentation (I assume we'll bundle and minify this code at some point, so splitting code into multiple files and verbose comments won't have a cost at run time).
1 parent aaec279 commit 591d87b

File tree

1 file changed

+116
-110
lines changed

1 file changed

+116
-110
lines changed

‎napi/oxlint2/src-js/index.js‎

Lines changed: 116 additions & 110 deletions
Original file line numberDiff line numberDiff line change
@@ -11,131 +11,138 @@ const { TOKEN } = require('../../parser/raw-transfer/lazy-common.js'),
1111
{ Visitor, getVisitorsArr } = require('../../parser/raw-transfer/visitor.js'),
1212
walkProgram = require('../../parser/generated/lazy/walk.js');
1313

14-
const textDecoder = new TextDecoder('utf-8', { ignoreBOM: true });
14+
// --------------------
15+
// Plugin loading
16+
// --------------------
1517

16-
class PluginRegistry {
17-
registeredPluginPaths = new Set();
18+
// Absolute paths of plugins which have been loaded
19+
const registeredPluginPaths = new Set();
1820

19-
registeredRules = [];
21+
// Rule objects for loaded rules.
22+
// Indexed by `ruleId`, passed to `lintFile`.
23+
const registeredRules = [];
2024

21-
isPluginRegistered(path) {
22-
return this.registeredPluginPaths.has(path);
25+
/**
26+
* Load a plugin.
27+
*
28+
* Main logic is in separate function `loadPluginImpl`, because V8 cannot optimize functions
29+
* containing try/catch.
30+
*
31+
* @param {string} path - Absolute path of plugin file
32+
* @returns {string} - JSON result
33+
*/
34+
async function loadPlugin(path) {
35+
try {
36+
return await loadPluginImpl(path);
37+
} catch (error) {
38+
const errorMessage = 'message' in error && typeof error.message === 'string'
39+
? error.message
40+
: 'An unknown error occurred';
41+
return JSON.stringify({ Failure: errorMessage });
2342
}
43+
}
2444

25-
registerPlugin(path, plugin) {
26-
// TODO: use a validation library to assert the shape of the plugin
27-
this.registeredPluginPaths.add(path);
28-
const ret = {
29-
name: plugin.meta.name,
30-
offset: this.registeredRules.length,
31-
rules: [],
32-
};
33-
34-
for (const [ruleName, rule] of Object.entries(plugin.rules)) {
35-
ret.rules.push(ruleName);
36-
this.registeredRules.push(rule);
37-
}
38-
39-
return ret;
45+
async function loadPluginImpl(path) {
46+
if (registeredPluginPaths.has(path)) {
47+
return JSON.stringify({ Failure: 'This plugin has already been registered' });
4048
}
4149

42-
*getRules(ruleIds) {
43-
for (const ruleId of ruleIds) {
44-
yield { rule: this.registeredRules[ruleId], ruleId };
45-
}
50+
const { default: plugin } = await import(path);
51+
52+
registeredPluginPaths.add(path);
53+
54+
// TODO: Use a validation library to assert the shape of the plugin, and of rules
55+
const rules = [];
56+
const ret = {
57+
name: plugin.meta.name,
58+
offset: registeredRules.length,
59+
rules,
60+
};
61+
62+
for (const [ruleName, rule] of Object.entries(plugin.rules)) {
63+
rules.push(ruleName);
64+
registeredRules.push(rule);
4665
}
66+
67+
return JSON.stringify({ Success: ret });
4768
}
4869

49-
// Buffers cache
70+
// --------------------
71+
// Running rules
72+
// --------------------
73+
74+
// Buffers cache.
75+
//
76+
// All buffers sent from Rust are stored in this array, indexed by `bufferId` (also sent from Rust).
77+
// Buffers are only added to this array, never removed, so no buffers will be garbage collected
78+
// until the process exits.
5079
const buffers = [];
5180

5281
// Diagnostics array. Reused for every file.
5382
const diagnostics = [];
5483

55-
class Linter {
56-
pluginRegistry = new PluginRegistry();
57-
58-
run() {
59-
return lint(this.loadPlugin.bind(this), this.lint.bind(this));
60-
}
61-
62-
/**
63-
* @param {string} path - The absolute path of the plugin we're loading
64-
*/
65-
async loadPlugin(path) {
66-
if (this.pluginRegistry.isPluginRegistered(path)) {
67-
return JSON.stringify({
68-
Failure: 'This plugin has already been registered',
69-
});
70-
}
84+
// Text decoder, for decoding source text from buffer
85+
const textDecoder = new TextDecoder('utf-8', { ignoreBOM: true });
7186

72-
try {
73-
const { default: plugin } = await import(path);
74-
const ret = this.pluginRegistry.registerPlugin(path, plugin);
75-
return JSON.stringify({ Success: ret });
76-
} catch (error) {
77-
const errorMessage = 'message' in error && typeof error.message === 'string'
78-
? error.message
79-
: 'An unknown error occurred';
80-
return JSON.stringify({ Failure: errorMessage });
87+
// Run rules on a file.
88+
//
89+
// TODO(camc314): why do we have to destructure here?
90+
// In `./bindings.d.ts`, it doesn't indicate that we have to
91+
// (typed as `(filePath: string, bufferId: number, buffer: Uint8Array | undefined | null, ruleIds: number[])`).
92+
function lintFile([filePath, bufferId, buffer, ruleIds]) {
93+
// If new buffer, add it to `buffers` array. Otherwise, get existing buffer from array.
94+
// Do this before checks below, to make sure buffer doesn't get garbage collected when not expected
95+
// if there's an error.
96+
// TODO: Is this enough to guarantee soundness?
97+
if (buffer !== null) {
98+
const { buffer: arrayBuffer, byteOffset } = buffer;
99+
buffer.uint32 = new Uint32Array(arrayBuffer, byteOffset);
100+
buffer.float64 = new Float64Array(arrayBuffer, byteOffset);
101+
102+
while (buffers.length <= bufferId) {
103+
buffers.push(null);
81104
}
105+
buffers[bufferId] = buffer;
106+
} else {
107+
buffer = buffers[bufferId];
82108
}
83109

84-
// TODO(camc314): why do we have to destructure here?
85-
// In `./bindings.d.ts`, it doesn't indicate that we have to
86-
// (typed as `(filePath: string, bufferId: number, buffer: Uint8Array | undefined | null, ruleIds: number[])`).
87-
lint([filePath, bufferId, buffer, ruleIds]) {
88-
// If new buffer, add it to `buffers` array. Otherwise, get existing buffer from array.
89-
// Do this before checks below, to make sure buffer doesn't get garbage collected when not expected
90-
// if there's an error.
91-
// TODO: Is this enough to guarantee soundness?
92-
if (buffer !== null) {
93-
const { buffer: arrayBuffer, byteOffset } = buffer;
94-
buffer.uint32 = new Uint32Array(arrayBuffer, byteOffset);
95-
buffer.float64 = new Float64Array(arrayBuffer, byteOffset);
96-
97-
while (buffers.length <= bufferId) {
98-
buffers.push(null);
99-
}
100-
buffers[bufferId] = buffer;
101-
} else {
102-
buffer = buffers[bufferId];
103-
}
104-
105-
if (typeof filePath !== 'string' || filePath.length === 0) {
106-
throw new Error('expected filePath to be a non-zero length string');
107-
}
108-
if (!Array.isArray(ruleIds) || ruleIds.length === 0) {
109-
throw new Error('Expected `ruleIds` to be a non-zero len array');
110-
}
111-
112-
// Get visitors for this file from all rules
113-
const visitors = [];
114-
for (const { rule, ruleId } of this.pluginRegistry.getRules(ruleIds)) {
115-
visitors.push(rule.create(new Context(ruleId, filePath)));
116-
}
110+
if (typeof filePath !== 'string' || filePath.length === 0) {
111+
throw new Error('expected filePath to be a non-zero length string');
112+
}
113+
if (!Array.isArray(ruleIds) || ruleIds.length === 0) {
114+
throw new Error('Expected `ruleIds` to be a non-zero len array');
115+
}
117116

118-
const visitor = new Visitor(
119-
visitors.length === 1 ? visitors[0] : combineVisitors(visitors),
120-
);
117+
// Get visitors for this file from all rules
118+
const visitors = ruleIds.map(
119+
ruleId => registeredRules[ruleId].create(new Context(ruleId, filePath)),
120+
);
121+
const visitor = new Visitor(
122+
visitors.length === 1 ? visitors[0] : combineVisitors(visitors),
123+
);
121124

122-
// Visit AST
123-
const programPos = buffer.uint32[DATA_POINTER_POS_32],
124-
sourceByteLen = buffer.uint32[SOURCE_LEN_POS_32];
125+
// Visit AST
126+
const programPos = buffer.uint32[DATA_POINTER_POS_32],
127+
sourceByteLen = buffer.uint32[SOURCE_LEN_POS_32];
125128

126-
const sourceText = textDecoder.decode(buffer.subarray(0, sourceByteLen));
127-
const sourceIsAscii = sourceText.length === sourceByteLen;
128-
const ast = { buffer, sourceText, sourceByteLen, sourceIsAscii, nodes: new Map(), token: TOKEN };
129+
const sourceText = textDecoder.decode(buffer.subarray(0, sourceByteLen));
130+
const sourceIsAscii = sourceText.length === sourceByteLen;
131+
const ast = { buffer, sourceText, sourceByteLen, sourceIsAscii, nodes: new Map(), token: TOKEN };
129132

130-
walkProgram(programPos, ast, getVisitorsArr(visitor));
133+
walkProgram(programPos, ast, getVisitorsArr(visitor));
131134

132-
// Send diagnostics back to Rust
133-
const ret = JSON.stringify(diagnostics);
134-
diagnostics.length = 0;
135-
return ret;
136-
}
135+
// Send diagnostics back to Rust
136+
const ret = JSON.stringify(diagnostics);
137+
diagnostics.length = 0;
138+
return ret;
137139
}
138140

141+
/**
142+
* Combine multiple visitor objects into a single visitor object.
143+
* @param {Array<Object>} visitors - Array of visitor objects
144+
* @returns {Object} - Combined visitor object
145+
*/
139146
function combineVisitors(visitors) {
140147
const combinedVisitor = {};
141148
for (const visitor of visitors) {
@@ -161,7 +168,7 @@ function combineVisitors(visitors) {
161168
* A `Context` is passed to each rule's `create` function.
162169
*/
163170
class Context {
164-
// Rule ID. Index into `PluginRegistry`'s `registeredRules` array.
171+
// Rule ID. Index into `registeredRules` array.
165172
#ruleId;
166173

167174
/**
@@ -192,15 +199,14 @@ class Context {
192199
}
193200
}
194201

195-
async function main() {
196-
const linter = new Linter();
202+
// --------------------
203+
// Run linter
204+
// --------------------
197205

198-
const success = await linter.run();
199-
200-
// Note: It's recommended to set `process.exitCode` instead of calling `process.exit()`.
201-
// `process.exit()` kills the process immediately and `stdout` may not be flushed before process dies.
202-
// https://nodejs.org/api/process.html#processexitcode
203-
if (!success) process.exitCode = 1;
204-
}
206+
// Call Rust, passing `loadPlugin` and `lintFile` as callbacks
207+
const success = await lint(loadPlugin, lintFile);
205208

206-
main();
209+
// Note: It's recommended to set `process.exitCode` instead of calling `process.exit()`.
210+
// `process.exit()` kills the process immediately and `stdout` may not be flushed before process dies.
211+
// https://nodejs.org/api/process.html#processexitcode
212+
if (!success) process.exitCode = 1;

0 commit comments

Comments
 (0)