Skip to content

Commit 222a8f0

Browse files
overlookmoteloverlookmotel
committed
feat(linter/plugins): implement SourceCode#isSpaceBetween (#15498)
Implement `sourceCode.isSpaceBetween`. Implementation is not quite correct, because we don't have tokens yet. But it's better than no implementation at all. We can correct it once we have tokens. The reason it's not correct is explained in comment on `isSpaceBetween` implementation, and the incorrect behavior is demonstrated in the test fixture.
1 parent 79e6842 commit 222a8f0

7 files changed

Lines changed: 314 additions & 14 deletions

File tree

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

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import * as scopeMethods from './scope.js';
2020
import * as tokenMethods from './tokens.js';
2121

2222
import type { Program } from '../generated/types.d.ts';
23-
import type { BufferWithArrays, Node, NodeOrToken, Ranged } from './types.ts';
23+
import type { BufferWithArrays, Node, Ranged } from './types.ts';
2424
import type { ScopeManager } from './scope.ts';
2525

2626
const { max } = Math;
@@ -191,19 +191,6 @@ export const SOURCE_CODE = Object.freeze({
191191
return ancestors.reverse();
192192
},
193193

194-
/**
195-
* Determine if two nodes or tokens have at least one whitespace character between them.
196-
* Order does not matter. Returns `false` if the given nodes or tokens overlap.
197-
* @param nodeOrToken1 - The first node or token to check between.
198-
* @param nodeOrToken2 - The second node or token to check between.
199-
* @returns `true` if there is a whitespace character between
200-
* any of the tokens found between the two given nodes or tokens.
201-
*/
202-
// oxlint-disable-next-line no-unused-vars
203-
isSpaceBetween(nodeOrToken1: NodeOrToken, nodeOrToken2: NodeOrToken): boolean {
204-
throw new Error('`sourceCode.isSpaceBetween` not implemented yet'); // TODO
205-
},
206-
207194
/**
208195
* Get the deepest node containing a range index.
209196
* @param index Range index of the desired node.
@@ -246,6 +233,7 @@ export const SOURCE_CODE = Object.freeze({
246233
getLastTokenBetween: tokenMethods.getLastTokenBetween,
247234
getLastTokensBetween: tokenMethods.getLastTokensBetween,
248235
getTokenByRangeStart: tokenMethods.getTokenByRangeStart,
236+
isSpaceBetween: tokenMethods.isSpaceBetween,
249237
});
250238

251239
export type SourceCode = typeof SOURCE_CODE;

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
* `SourceCode` methods related to tokens.
33
*/
44

5+
import { sourceText, initSourceText } from './source_code.js';
6+
57
import type { Comment, Node, NodeOrToken, Token } from './types.ts';
68

79
// Options for various `SourceCode` methods e.g. `getFirstToken`.
@@ -276,3 +278,63 @@ export function getLastTokensBetween(
276278
export function getTokenByRangeStart(index: number, rangeOptions?: RangeOptions | null | undefined): Token | null {
277279
throw new Error('`sourceCode.getTokenByRangeStart` not implemented yet'); // TODO
278280
}
281+
282+
// Regex that tests for whitespace.
283+
// TODO: Is this too liberal? Should it be a more constrained set of whitespace characters?
284+
const WHITESPACE_REGEXP = /\s/;
285+
286+
/**
287+
* Determine if two nodes or tokens have at least one whitespace character between them.
288+
* Order does not matter.
289+
*
290+
* Returns `false` if the given nodes or tokens overlap.
291+
*
292+
* Checks for whitespace *between tokens*, not including whitespace *inside tokens*.
293+
* e.g. Returns `false` for `isSpaceBetween(x, y)` in `x+" "+y`.
294+
*
295+
* TODO: Implementation is not quite right at present.
296+
* We don't use tokens, so return `true` for `isSpaceBetween(x, y)` in `x+" "+y`, but should return `false`.
297+
* Note: `checkInsideOfJSXText === false` in ESLint's implementation of `sourceCode.isSpaceBetween`.
298+
* https://github.com/eslint/eslint/blob/523c076866400670fb2192a3f55dbf7ad3469247/lib/languages/js/source-code/source-code.js#L182-L230
299+
*
300+
* @param nodeOrToken1 - The first node or token to check between.
301+
* @param nodeOrToken2 - The second node or token to check between.
302+
* @returns `true` if there is a whitespace character between
303+
* any of the tokens found between the two given nodes or tokens.
304+
*/
305+
export function isSpaceBetween(nodeOrToken1: NodeOrToken, nodeOrToken2: NodeOrToken): boolean {
306+
const range1 = nodeOrToken1.range,
307+
range2 = nodeOrToken2.range,
308+
start1 = range1[0],
309+
start2 = range2[0];
310+
311+
// Find the gap between the two nodes/tokens.
312+
//
313+
// 1 node/token can completely enclose another, but they can't *partially* overlap.
314+
// ```
315+
// Possible:
316+
// |------------|
317+
// |------|
318+
//
319+
// Impossible:
320+
// |------------|
321+
// |------------|
322+
// ```
323+
// We use that invariant to reduce this to a single branch.
324+
let gapStart, gapEnd;
325+
if (start1 < start2) {
326+
gapStart = range1[1]; // end1
327+
gapEnd = start2;
328+
} else {
329+
gapStart = range2[1]; // end2;
330+
gapEnd = start1;
331+
}
332+
333+
// If `gapStart >= gapEnd`, one node encloses the other, or the two are directly adjacent
334+
if (gapStart >= gapEnd) return false;
335+
336+
// Check if there's any whitespace in the gap
337+
if (sourceText === null) initSourceText();
338+
339+
return WHITESPACE_REGEXP.test(sourceText.slice(gapStart, gapEnd));
340+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,4 +267,8 @@ describe('oxlint CLI', () => {
267267
it('wrapping context should work', async () => {
268268
await testFixture('context_wrapping');
269269
});
270+
271+
it('should support `isSpaceBetween` in `context.sourceCode`', async () => {
272+
await testFixture('isSpaceBetween');
273+
});
270274
});

‎apps/oxlint/test/fixtures/isSpaceBetween/.oxlintrc.json‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"jsPlugins": ["./plugin.ts"],
3+
"categories": { "correctness": "off" },
4+
"rules": {
5+
"test-plugin/is-space-between": "error"
6+
}
7+
}

‎apps/oxlint/test/fixtures/isSpaceBetween/files/index.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
noSpace=1;
2+
3+
singleSpaceBefore =2;
4+
5+
singleSpaceAfter= 3;
6+
7+
multipleSpaces = 4;
8+
9+
newlineBefore=
10+
5;
11+
12+
newlineAfter
13+
=6;
14+
15+
nested = 7 + 8;
16+
17+
// We should return `false` for `isSpaceBetween(beforeString, afterString)`, but we currently return `true`
18+
beforeString," ",afterString;

‎apps/oxlint/test/fixtures/isSpaceBetween/output.snap.md‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
# Exit code
2+
1
3+
4+
# stdout
5+
```
6+
x test-plugin(is-space-between):
7+
| isSpaceBetween(left, right): false
8+
| isSpaceBetween(right, left): false
9+
| isSpaceBetween(left, node): false
10+
| isSpaceBetween(node, left): false
11+
| isSpaceBetween(right, node): false
12+
| isSpaceBetween(node, right): false
13+
,-[files/index.js:1:1]
14+
1 | noSpace=1;
15+
: ^^^^^^^^^
16+
2 |
17+
`----
18+
19+
x test-plugin(is-space-between):
20+
| isSpaceBetween(leftExtended, right): false
21+
| isSpaceBetween(right, leftExtended): false
22+
,-[files/index.js:1:1]
23+
1 | noSpace=1;
24+
: ^^^^^^^^^
25+
2 |
26+
`----
27+
28+
x test-plugin(is-space-between):
29+
| isSpaceBetween(left, right): true
30+
| isSpaceBetween(right, left): true
31+
| isSpaceBetween(left, node): false
32+
| isSpaceBetween(node, left): false
33+
| isSpaceBetween(right, node): false
34+
| isSpaceBetween(node, right): false
35+
,-[files/index.js:3:1]
36+
2 |
37+
3 | singleSpaceBefore =2;
38+
: ^^^^^^^^^^^^^^^^^^^^
39+
4 |
40+
`----
41+
42+
x test-plugin(is-space-between):
43+
| isSpaceBetween(left, right): true
44+
| isSpaceBetween(right, left): true
45+
| isSpaceBetween(left, node): false
46+
| isSpaceBetween(node, left): false
47+
| isSpaceBetween(right, node): false
48+
| isSpaceBetween(node, right): false
49+
,-[files/index.js:5:1]
50+
4 |
51+
5 | singleSpaceAfter= 3;
52+
: ^^^^^^^^^^^^^^^^^^^
53+
6 |
54+
`----
55+
56+
x test-plugin(is-space-between):
57+
| isSpaceBetween(left, right): true
58+
| isSpaceBetween(right, left): true
59+
| isSpaceBetween(left, node): false
60+
| isSpaceBetween(node, left): false
61+
| isSpaceBetween(right, node): false
62+
| isSpaceBetween(node, right): false
63+
,-[files/index.js:7:1]
64+
6 |
65+
7 | multipleSpaces = 4;
66+
: ^^^^^^^^^^^^^^^^^^^^^^
67+
8 |
68+
`----
69+
70+
x test-plugin(is-space-between):
71+
| isSpaceBetween(left, right): true
72+
| isSpaceBetween(right, left): true
73+
| isSpaceBetween(left, node): false
74+
| isSpaceBetween(node, left): false
75+
| isSpaceBetween(right, node): false
76+
| isSpaceBetween(node, right): false
77+
,-[files/index.js:9:1]
78+
8 |
79+
9 | ,-> newlineBefore=
80+
10 | `-> 5;
81+
11 |
82+
`----
83+
84+
x test-plugin(is-space-between):
85+
| isSpaceBetween(left, right): true
86+
| isSpaceBetween(right, left): true
87+
| isSpaceBetween(left, node): false
88+
| isSpaceBetween(node, left): false
89+
| isSpaceBetween(right, node): false
90+
| isSpaceBetween(node, right): false
91+
,-[files/index.js:12:1]
92+
11 |
93+
12 | ,-> newlineAfter
94+
13 | `-> =6;
95+
14 |
96+
`----
97+
98+
x test-plugin(is-space-between):
99+
| isSpaceBetween(node, binaryLeft): false
100+
| isSpaceBetween(binaryLeft, node): false
101+
,-[files/index.js:15:1]
102+
14 |
103+
15 | nested = 7 + 8;
104+
: ^^^^^^^^^^^^^^
105+
16 |
106+
`----
107+
108+
x test-plugin(is-space-between):
109+
| isSpaceBetween(left, right): true
110+
| isSpaceBetween(right, left): true
111+
| isSpaceBetween(left, node): false
112+
| isSpaceBetween(node, left): false
113+
| isSpaceBetween(right, node): false
114+
| isSpaceBetween(node, right): false
115+
,-[files/index.js:15:1]
116+
14 |
117+
15 | nested = 7 + 8;
118+
: ^^^^^^^^^^^^^^
119+
16 |
120+
`----
121+
122+
x test-plugin(is-space-between):
123+
| isSpaceBetween(beforeString, afterString): true
124+
| isSpaceBetween(afterString, beforeString): true
125+
,-[files/index.js:18:1]
126+
17 | // We should return `false` for `isSpaceBetween(beforeString, afterString)`, but we currently return `true`
127+
18 | beforeString," ",afterString;
128+
: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
129+
`----
130+
131+
Found 0 warnings and 10 errors.
132+
Finished in Xms on 1 file using X threads.
133+
```
134+
135+
# stderr
136+
```
137+
WARNING: JS plugins are experimental and not subject to semver.
138+
Breaking changes are possible while JS plugins support is under development.
139+
```

‎apps/oxlint/test/fixtures/isSpaceBetween/plugin.ts‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import assert from 'node:assert';
2+
3+
import type { Plugin, Rule, Node } from '../../../dist/index.js';
4+
5+
const testRule: Rule = {
6+
create(context) {
7+
return {
8+
AssignmentExpression(node) {
9+
const { isSpaceBetween } = context.sourceCode,
10+
{ left, right } = node;
11+
12+
context.report({
13+
message:
14+
'\n' +
15+
// Test where 2 nodes are separated, maybe with whitespace in between
16+
`isSpaceBetween(left, right): ${isSpaceBetween(left, right)}\n` +
17+
`isSpaceBetween(right, left): ${isSpaceBetween(right, left)}\n` +
18+
// Test where 1 node is inside another, sharing same `start` or `end`
19+
`isSpaceBetween(left, node): ${isSpaceBetween(left, node)}\n` +
20+
`isSpaceBetween(node, left): ${isSpaceBetween(node, left)}\n` +
21+
`isSpaceBetween(right, node): ${isSpaceBetween(right, node)}\n` +
22+
`isSpaceBetween(node, right): ${isSpaceBetween(node, right)}`,
23+
node,
24+
});
25+
26+
// Test where 1 node is inside another, not sharing same `start` or `end`
27+
if (right.type === 'BinaryExpression') {
28+
const binaryLeft = right.left;
29+
context.report({
30+
message:
31+
'\n' +
32+
`isSpaceBetween(node, binaryLeft): ${isSpaceBetween(node, binaryLeft)}\n` +
33+
`isSpaceBetween(binaryLeft, node): ${isSpaceBetween(binaryLeft, node)}`,
34+
node,
35+
});
36+
}
37+
38+
// Test where 2 nodes are completely adjacent to each other.
39+
// We don't have tokens yet, so adjust ranges of 1 node so they touch.
40+
assert(left.type === 'Identifier');
41+
if (left.name === 'noSpace') {
42+
const leftExtended: Node = { ...left, end: left.end + 1, range: [left.range[0], left.range[1] + 1] };
43+
assert(leftExtended.end === right.start);
44+
assert(leftExtended.range[1] === right.range[0]);
45+
46+
context.report({
47+
message:
48+
'\n' +
49+
`isSpaceBetween(leftExtended, right): ${isSpaceBetween(leftExtended, right)}\n` +
50+
`isSpaceBetween(right, leftExtended): ${isSpaceBetween(right, leftExtended)}`,
51+
node,
52+
});
53+
}
54+
},
55+
56+
SequenceExpression(node) {
57+
const { isSpaceBetween } = context.sourceCode,
58+
[beforeString, , afterString] = node.expressions;
59+
60+
// We get this wrong. Should be `false`, but we get `true`.
61+
context.report({
62+
message:
63+
'\n' +
64+
`isSpaceBetween(beforeString, afterString): ${isSpaceBetween(beforeString, afterString)}\n` +
65+
`isSpaceBetween(afterString, beforeString): ${isSpaceBetween(afterString, beforeString)}`,
66+
node,
67+
});
68+
},
69+
};
70+
},
71+
};
72+
73+
const plugin: Plugin = {
74+
meta: {
75+
name: 'test-plugin',
76+
},
77+
rules: {
78+
'is-space-between': testRule,
79+
},
80+
};
81+
82+
export default plugin;

0 commit comments

Comments
 (0)