Skip to content

Commit 65c6c33

Browse files
committed
refactor(linter/plugins): simplify fix type and validation (oxc-project#19078)
Refactor. Simplify the type used to represent a fix when sending from JS to Rust. Previously we avoided creating new objects by using the same fix object returned by `fix` function, and so were locked to ESLint's design. That worked, but the code was excessively complicated. Fixes should be fairly rare, so this optimization is on a cold path - it's not worth it. Simplify implementation by converting `Fix`es to a simpler type (`FixReport`) before sending to Rust.
1 parent 0a71fff commit 65c6c33

8 files changed

Lines changed: 70 additions & 105 deletions

File tree

apps/oxlint/src-js/plugins/fix.ts

Lines changed: 44 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import { getMessage } from "./report.ts";
2-
import { typeAssertIs } from "../utils/asserts.ts";
32

43
import type { RuleDetails } from "./load.ts";
54
import type { Range, Ranged } from "./location.ts";
@@ -17,8 +16,22 @@ export type FixFn = (
1716
| null
1817
| undefined;
1918

20-
// Type of a fix, as returned by `fix` function.
21-
export type Fix = { range: Range; text: string };
19+
/**
20+
* Fix, as returned by `fix` function.
21+
*/
22+
export interface Fix {
23+
range: Range;
24+
text: string;
25+
}
26+
27+
/**
28+
* Fix, in form sent to Rust.
29+
*/
30+
export interface FixReport {
31+
start: number;
32+
end: number;
33+
text: string;
34+
}
2235

2336
// Fixer, passed as argument to `fix` function passed to `Context#report()`.
2437
//
@@ -65,11 +78,11 @@ export type Fixer = typeof FIXER;
6578
*
6679
* @param diagnostic - Diagnostic object
6780
* @param ruleDetails - `RuleDetails` object, containing rule-specific `isFixable` value
68-
* @returns Non-empty array of `Fix` objects, or `null` if none
81+
* @returns Non-empty array of `FixReport` objects, or `null` if none
6982
* @throws {Error} If rule is not marked as fixable but `fix` function returns fixes,
7083
* or if `fix` function returns any invalid `Fix` objects
7184
*/
72-
export function getFixes(diagnostic: Diagnostic, ruleDetails: RuleDetails): Fix[] | null {
85+
export function getFixes(diagnostic: Diagnostic, ruleDetails: RuleDetails): FixReport[] | null {
7386
// ESLint silently ignores non-function `fix` values, so we do the same
7487
const { fix } = diagnostic;
7588
if (typeof fix !== "function") return null;
@@ -150,119 +163,70 @@ export function getSuggestions(
150163
}
151164

152165
/**
153-
* Call a `FixFn` and process its return value into an array of `Fix` objects.
166+
* Call a `FixFn` and process its return value into an array of `FixReport` objects.
154167
*
155168
* Returns `null` if any of:
156169
*
157170
* 1. `fixFn` returns a falsy value.
158171
* 2. `fixFn` returns an empty array/iterator.
159172
* 3. `fixFn` returns an array/iterator containing only falsy values.
160173
*
161-
* Otherwise, returns a non-empty array of `Fix` objects.
174+
* Otherwise, returns a non-empty array of `FixReport` objects.
162175
*
163-
* `Fix` objects are validated and conformed to expected shape.
164-
* Does not mutate the `fixes` array returned by `fixFn`, but avoids cloning if possible.
176+
* `Fix` objects are validated.
165177
*
166178
* This function aims to replicate ESLint's behavior as closely as possible.
167179
*
168-
* TODO: Are prototype checks, and checks for `toJSON` methods excessive?
169-
* We're not handling all possible edge cases e.g. `fixes` or individual `Fix` objects being `Proxy`s or objects
170-
* with getters. As we're not managing to be 100% bulletproof anyway, maybe we don't need to be quite so defensive.
171-
*
172180
* @param fixFn - Fix function to call
173181
* @param thisArg - `this` value for the fix function call
174-
* @returns Non-empty array of `Fix` objects, or `null` if none
182+
* @returns Non-empty array of `FixReport` objects, or `null` if none
175183
* @throws {Error} If `fixFn` returns any invalid `Fix` objects
176184
*/
177-
function getFixesFromFixFn(fixFn: FixFn, thisArg: Diagnostic | Suggestion): Fix[] | null {
185+
function getFixesFromFixFn(fixFn: FixFn, thisArg: Diagnostic | Suggestion): FixReport[] | null {
178186
// In ESLint, `fix` is called with `this` as a clone of the `Diagnostic` or `Suggestion` object.
179187
// We just use the original object - that should be close enough.
180-
let fixes = fixFn.call(thisArg, FIXER);
188+
const fixes = fixFn.call(thisArg, FIXER);
181189

182190
// ESLint ignores falsy values
183191
if (!fixes) return null;
184192

185193
// `fixes` can be any iterator, not just an array e.g. `fix: function*() { yield fix1; yield fix2; }`
186194
if (Symbol.iterator in fixes) {
187-
let isCloned = false;
188-
189-
// Check prototype instead of using `Array.isArray()`, to ensure it is a native `Array`,
190-
// not a subclass which may have overridden `toJSON()` in a way which could make `JSON.stringify()` throw
191-
if (Object.getPrototypeOf(fixes) !== Array.prototype || Object.hasOwn(fixes, "toJSON")) {
192-
fixes = Array.from(fixes);
193-
isCloned = true;
195+
const fixReports: FixReport[] = [];
196+
for (const fix of fixes) {
197+
// ESLint ignores falsy values
198+
if (fix) fixReports.push(validateAndConvertFix(fix));
194199
}
195200

196-
const fixesLen = fixes.length;
197-
if (fixesLen === 0) return null;
198-
199-
for (let i = 0; i < fixesLen; i++) {
200-
const fix = fixes[i];
201-
202-
// ESLint ignores falsy values.
203-
// Filter them out. This branch can only be taken once.
204-
if (!fix) {
205-
fixes = fixes.filter(Boolean);
206-
if (fixes.length === 0) return null;
207-
isCloned = true;
208-
i--;
209-
continue;
210-
}
211-
212-
const conformedFix = validateAndConformFix(fix);
213-
if (conformedFix !== fix) {
214-
// Don't mutate `fixes` array
215-
if (isCloned === false) {
216-
fixes = fixes.slice();
217-
isCloned = true;
218-
}
219-
fixes[i] = conformedFix;
220-
}
221-
}
222-
223-
return fixes;
201+
return fixReports.length === 0 ? null : fixReports;
224202
}
225203

226-
return [validateAndConformFix(fixes)];
204+
return [validateAndConvertFix(fixes)];
227205
}
228206

229207
/**
230-
* Validate that a `Fix` object is well-formed, and conform it to expected shape.
208+
* Validate that a `Fix` object is well-formed, and convert it to a `FixReport`.
231209
*
232-
* - Convert `text` to string if needed.
233-
* - Shorten `range` to 2 elements if it has extra elements.
234-
* - Remove any additional properties on the object.
210+
* Check that `range` has 2 numeric elements, and convert `text` to string if needed.
235211
*
236-
* Purpose is to ensure any input which ESLint accepts does not cause an error in `JSON.stringify()`,
212+
* Purpose of validation is to ensure any input which ESLint accepts does not cause an error in `JSON.stringify()`,
237213
* or in deserializing on Rust side.
238214
*
239215
* @param fix - Fix object to validate, possibly malformed
240-
* @returns `Fix` object
216+
* @returns `FixReport` object
217+
* @throws {Error} If `fix` has invalid `range`
241218
*/
242-
function validateAndConformFix(fix: unknown): Fix {
243-
typeAssertIs<Fix>(fix);
219+
function validateAndConvertFix(fix: Fix): FixReport {
244220
const { range, text } = fix;
245221

246-
// These checks follow ESLint, which throws if `range` is missing or invalid
247-
if (!range || typeof range[0] !== "number" || typeof range[1] !== "number") {
248-
throw new Error(`Fix has invalid range: ${JSON.stringify(fix, null, 2)}`);
249-
}
250-
251-
// If `fix` is already well-formed, return it as-is.
252-
// Note: `ownKeys(fix).length === 2` rules out `fix` having a custom `toJSON` method.
253-
const fixPrototype = Object.getPrototypeOf(fix);
254-
if (
255-
(fixPrototype === Object.prototype || fixPrototype === null) &&
256-
Reflect.ownKeys(fix).length === 2 &&
257-
Object.getPrototypeOf(range) === Array.prototype &&
258-
!Object.hasOwn(range, "toJSON") &&
259-
range.length === 2 &&
260-
typeof text === "string"
261-
) {
262-
return fix;
222+
if (range != null) {
223+
const start = range[0],
224+
end = range[1];
225+
if (typeof start === "number" && typeof end === "number") {
226+
// Converting `text` to string follows ESLint, which does that implicitly
227+
return { start, end, text: String(text) };
228+
}
263229
}
264230

265-
// Conform fix object to expected shape.
266-
// Converting `text` to string follows ESLint, which does that implicitly.
267-
return { range: [range[0], range[1]], text: String(text) };
231+
throw new Error(`Fix has invalid range: ${JSON.stringify(fix, null, 2)}`);
268232
}

apps/oxlint/src-js/plugins/report.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { sourceText } from "./source_code.ts";
99
import { debugAssertIsNonNull, typeAssertIs } from "../utils/asserts.ts";
1010

1111
import type { RequireAtLeastOne } from "type-fest";
12-
import type { Fix, FixFn } from "./fix.ts";
12+
import type { FixFn, FixReport } from "./fix.ts";
1313
import type { RuleDetails } from "./load.ts";
1414
import type { LineColumn, Ranged } from "./location.ts";
1515

@@ -67,7 +67,7 @@ interface SuggestionBase {
6767
*/
6868
export interface SuggestionReport {
6969
message: string;
70-
fixes: Fix[];
70+
fixes: FixReport[];
7171
}
7272

7373
// Diagnostic in form sent to Rust.
@@ -77,7 +77,7 @@ export interface DiagnosticReport {
7777
start: number;
7878
end: number;
7979
ruleIndex: number;
80-
fixes: Fix[] | null;
80+
fixes: FixReport[] | null;
8181
suggestions: SuggestionReport[] | null;
8282
messageId: string | null;
8383
// Only used in conformance tests

apps/oxlint/test/fixtures/fixes/fix.snap.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
```
66
x Error running JS plugin.
77
| File path: <fixture>/files/range_end_negative.js
8-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 111
8+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 116
99
1010
x Plugin `fixes-plugin/fixes` returned invalid fixes.
1111
| File path: <fixture>/files/range_end_out_of_bounds.js
@@ -17,7 +17,7 @@
1717
1818
x Error running JS plugin.
1919
| File path: <fixture>/files/range_end_too_large.js
20-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 119
20+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 124
2121
2222
x Plugin `fixes-plugin/fixes` returned invalid fixes.
2323
| File path: <fixture>/files/range_start_after_end.js
@@ -29,11 +29,11 @@
2929
3030
x Error running JS plugin.
3131
| File path: <fixture>/files/range_start_negative.js
32-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 111
32+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 110
3333
3434
x Error running JS plugin.
3535
| File path: <fixture>/files/range_start_too_large.js
36-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 119
36+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 118
3737
3838
x fixes-plugin(fixes): end out of bounds
3939
,-[files/range_end_out_of_bounds.js:1:5]

apps/oxlint/test/fixtures/fixes/output.snap.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
```
66
x Error running JS plugin.
77
| File path: <fixture>/files/range_end_negative.js
8-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 111
8+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 116
99
1010
x Plugin `fixes-plugin/fixes` returned invalid fixes.
1111
| File path: <fixture>/files/range_end_out_of_bounds.js
@@ -17,7 +17,7 @@
1717
1818
x Error running JS plugin.
1919
| File path: <fixture>/files/range_end_too_large.js
20-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 119
20+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 124
2121
2222
x Plugin `fixes-plugin/fixes` returned invalid fixes.
2323
| File path: <fixture>/files/range_start_after_end.js
@@ -29,11 +29,11 @@
2929
3030
x Error running JS plugin.
3131
| File path: <fixture>/files/range_start_negative.js
32-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 111
32+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 110
3333
3434
x Error running JS plugin.
3535
| File path: <fixture>/files/range_start_too_large.js
36-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 119
36+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 118
3737
3838
x fixes-plugin(fixes): Replace "a" with "daddy"
3939
,-[files/bom.js:1:4]

apps/oxlint/test/fixtures/suggestions/fix-suggestions.snap.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
```
66
x Error running JS plugin.
77
| File path: <fixture>/files/range_end_negative.js
8-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 158
8+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 163
99
1010
x Plugin `suggestions-plugin/suggestions` returned invalid suggestions.
1111
| File path: <fixture>/files/range_end_out_of_bounds.js
@@ -17,7 +17,7 @@
1717
1818
x Error running JS plugin.
1919
| File path: <fixture>/files/range_end_too_large.js
20-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 166
20+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 171
2121
2222
x Plugin `suggestions-plugin/suggestions` returned invalid suggestions.
2323
| File path: <fixture>/files/range_start_after_end.js
@@ -29,11 +29,11 @@
2929
3030
x Error running JS plugin.
3131
| File path: <fixture>/files/range_start_negative.js
32-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 158
32+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 157
3333
3434
x Error running JS plugin.
3535
| File path: <fixture>/files/range_start_too_large.js
36-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 166
36+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 165
3737
3838
x suggestions-plugin(suggestions): end out of bounds
3939
,-[files/range_end_out_of_bounds.js:1:5]

apps/oxlint/test/fixtures/suggestions/fix.snap.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,19 @@
55
```
66
x Error running JS plugin.
77
| File path: <fixture>/files/range_end_negative.js
8-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 158
8+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 163
99
1010
x Error running JS plugin.
1111
| File path: <fixture>/files/range_end_too_large.js
12-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 166
12+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 171
1313
1414
x Error running JS plugin.
1515
| File path: <fixture>/files/range_start_negative.js
16-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 158
16+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 157
1717
1818
x Error running JS plugin.
1919
| File path: <fixture>/files/range_start_too_large.js
20-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 166
20+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 165
2121
2222
x suggestions-plugin(suggestions): Replace "a" with "daddy"
2323
,-[files/bom.js:1:4]

apps/oxlint/test/fixtures/suggestions/output.snap.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,19 +5,19 @@
55
```
66
x Error running JS plugin.
77
| File path: <fixture>/files/range_end_negative.js
8-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 158
8+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 163
99
1010
x Error running JS plugin.
1111
| File path: <fixture>/files/range_end_too_large.js
12-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 166
12+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 171
1313
1414
x Error running JS plugin.
1515
| File path: <fixture>/files/range_start_negative.js
16-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 158
16+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `-10`, expected u32 at line 1 column 157
1717
1818
x Error running JS plugin.
1919
| File path: <fixture>/files/range_start_too_large.js
20-
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 166
20+
| Failed to deserialize JSON returned by `lintFile`: invalid value: integer `4294967296`, expected u32 at line 1 column 165
2121
2222
x suggestions-plugin(suggestions): Replace "a" with "daddy"
2323
,-[files/bom.js:1:4]

crates/oxc_linter/src/external_linter.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,8 @@ pub struct LintFileResult {
8686
#[derive(Clone, Debug, Deserialize)]
8787
#[serde(rename_all = "camelCase")]
8888
pub struct JsFix {
89-
pub range: [u32; 2],
89+
pub start: u32,
90+
pub end: u32,
9091
pub text: String,
9192
}
9293

@@ -109,7 +110,7 @@ pub fn convert_and_merge_js_fixes(
109110
let is_single = fixes.len() == 1;
110111

111112
let mut fixes = fixes.into_iter().map(|fix| {
112-
let mut span = Span::new(fix.range[0], fix.range[1]);
113+
let mut span = Span::new(fix.start, fix.end);
113114
span_converter.convert_span_back(&mut span);
114115
Fix::new(fix.text, span)
115116
});

0 commit comments

Comments
 (0)