Skip to content

Commit c245b9d

Browse files
aidenybaiaidenybai
authored
fix(no-flush-sync): recognize imperative DOM handoffs (#1340)
* fix(no-flush-sync): allow imperative DOM handoffs * fix(no-flush-sync): harden handoff adjacency * fix(no-flush-sync): tighten imperative handoffs
1 parent 093619c commit c245b9d

6 files changed

Lines changed: 281 additions & 16 deletions

File tree

‎.changeset/strong-kids-take.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"oxlint-plugin-react-doctor": patch
3+
---
4+
5+
Avoid `no-flush-sync` false positives when an adjacent imperative DOM selection or focus handoff requires the React commit to finish synchronously.

‎packages/fuzz/corpus/regressions/no-flush-sync--selection-restoration.tsx‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// rule: no-flush-sync
2+
// weakness: imperative-post-commit-handoff
3+
// source: react-bench write-react-softmaple-softmaple gxWkbFm
4+
5+
import { flushSync } from "react-dom";
6+
7+
interface SelectionSync {
8+
restoreSelection: (selection: { end: number; start: number }) => void;
9+
}
10+
11+
export const acceptRemoteText = (
12+
selectionSync: SelectionSync,
13+
selection: { end: number; start: number } | null,
14+
): void => {
15+
flushSync(() => {
16+
setText(readRemoteText());
17+
});
18+
if (selection) {
19+
selectionSync.restoreSelection(selection);
20+
}
21+
};

‎packages/fuzz/corpus/targets/no-flush-sync--bare-control-flow-handoff.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
import { flushSync } from "react-dom";
2+
3+
export const updateText = (textarea: HTMLTextAreaElement, shouldUpdate: boolean): void => {
4+
if (shouldUpdate) flushSync(() => setText(readRemoteText()));
5+
textarea.focus();
6+
};

‎packages/fuzz/corpus/targets/no-flush-sync--generic-select-handoff.tsx‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import { flushSync } from "react-dom";
2+
3+
interface Store {
4+
select: (name: string) => void;
5+
}
6+
7+
export const updateSelection = (store: Store): void => {
8+
flushSync(() => setText(readRemoteText()));
9+
store.select("activeDocument");
10+
};

‎packages/oxlint-plugin-react-doctor/src/plugin/rules/view-transitions/no-flush-sync.regressions.test.ts‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,149 @@ function ColumnHeader({ releaseWidth, columnIndex }) {
120120
tryToMeasureWidth();
121121
}, [tryToMeasureWidth, releaseWidth, columnIndex]);
122122
return <th ref={ref} onDoubleClick={autoResize} />;
123+
}`,
124+
);
125+
});
126+
127+
it("stays silent when selection restoration immediately follows flushSync", () => {
128+
expectPass(
129+
`import { flushSync } from "react-dom";
130+
const acceptRemoteEvents = (selectionSync, selection, operations) => {
131+
flushSync(() => {
132+
setText(readRemoteText());
133+
});
134+
if (selection && operations.length > 0) {
135+
selectionSync.restoreSelection(mapSelection(selection, operations));
136+
}
137+
};`,
138+
);
139+
});
140+
141+
it("stays silent on a nested selection restoration member", () => {
142+
expectPass(
143+
`import { flushSync } from "react-dom";
144+
const integrateRemoteEvents = (context, selection) => {
145+
flushSync(() => {
146+
context.setText(readRemoteText());
147+
});
148+
if (selection) {
149+
context.selectionSync.restoreSelection(selection);
150+
}
151+
};`,
152+
);
153+
});
154+
155+
it("stays silent when an adjacent local helper mutates the DOM", () => {
156+
expectPass(
157+
`import { flushSync } from "react-dom";
158+
const restoreSelection = (textarea, selection) => {
159+
textarea.setSelectionRange(selection.start, selection.end);
160+
};
161+
const updateText = (textarea, selection) => {
162+
flushSync(() => setText(readRemoteText()));
163+
restoreSelection(textarea, selection);
164+
};`,
165+
);
166+
});
167+
168+
it("still flags an adjacent unknown helper", () => {
169+
expectFail(
170+
`import { flushSync } from "react-dom";
171+
const updateText = () => {
172+
flushSync(() => setText(readRemoteText()));
173+
notifyTextUpdated();
174+
};`,
175+
);
176+
});
177+
178+
it("still flags a non-adjacent imperative mutation", () => {
179+
expectFail(
180+
`import { flushSync } from "react-dom";
181+
const updateText = (textarea, selection) => {
182+
flushSync(() => setText(readRemoteText()));
183+
notifyTextUpdated();
184+
textarea.setSelectionRange(selection.start, selection.end);
185+
};`,
186+
);
187+
});
188+
189+
it("still flags an imperative mutation outside a bare control-flow branch", () => {
190+
expectFail(
191+
`import { flushSync } from "react-dom";
192+
const updateText = (textarea, shouldUpdate) => {
193+
if (shouldUpdate) flushSync(() => setText(readRemoteText()));
194+
textarea.focus();
195+
};`,
196+
);
197+
});
198+
199+
it("still flags an adjacent generic select method", () => {
200+
expectFail(
201+
`import { flushSync } from "react-dom";
202+
const updateSelection = (store) => {
203+
flushSync(() => setText(readRemoteText()));
204+
store.select("activeDocument");
205+
};`,
206+
);
207+
});
208+
209+
it("still flags a deferred imperative helper call", () => {
210+
expectFail(
211+
`import { flushSync } from "react-dom";
212+
const restoreSelection = (textarea, selection) => {
213+
textarea.setSelectionRange(selection.start, selection.end);
214+
};
215+
const updateText = (textarea, selection) => {
216+
flushSync(() => setText(readRemoteText()));
217+
const restoreLater = () => restoreSelection(textarea, selection);
218+
queueMicrotask(restoreLater);
219+
};`,
220+
);
221+
});
222+
223+
it("still flags an adjacent imperative function declaration", () => {
224+
expectFail(
225+
`import { flushSync } from "react-dom";
226+
const updateText = (textarea) => {
227+
flushSync(() => setText(readRemoteText()));
228+
function restoreLater() {
229+
textarea.focus();
230+
}
231+
queueMicrotask(restoreLater);
232+
};`,
233+
);
234+
});
235+
236+
it("stays silent on a top-level imperative handoff", () => {
237+
expectPass(
238+
`import { flushSync } from "react-dom";
239+
flushSync(() => setText(readRemoteText()));
240+
textarea.focus();`,
241+
);
242+
});
243+
244+
it("stays silent on an imperative handoff in a switch case", () => {
245+
expectPass(
246+
`import { flushSync } from "react-dom";
247+
const updateText = (textarea, mode) => {
248+
switch (mode) {
249+
case "edit":
250+
flushSync(() => setText(readRemoteText()));
251+
textarea.focus();
252+
break;
253+
}
254+
};`,
255+
);
256+
});
257+
258+
it("stays silent on an imperative handoff in a static block", () => {
259+
expectPass(
260+
`import { flushSync } from "react-dom";
261+
class Editor {
262+
static {
263+
flushSync(() => setText(readRemoteText()));
264+
textarea.focus();
265+
}
123266
}`,
124267
);
125268
});

‎packages/oxlint-plugin-react-doctor/src/plugin/rules/view-transitions/no-flush-sync.ts‎

Lines changed: 96 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import { isFunctionLike } from "../../utils/is-function-like.js";
55
import { isNodeOfType } from "../../utils/is-node-of-type.js";
66
import { getImportedName } from "../../utils/get-imported-name.js";
77
import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js";
8+
import { getStaticPropertyName } from "../../utils/get-static-property-name.js";
9+
import { stripParenExpression } from "../../utils/strip-paren-expression.js";
810
import { walkAst } from "../../utils/walk-ast.js";
911

1012
// Libraries that position or attach to freshly committed DOM — the
@@ -38,6 +40,18 @@ const DOM_MEASUREMENT_NAMES: ReadonlySet<string> = new Set([
3840
const MEASUREMENT_HELPER_CALLEE_PATTERN =
3941
/^(?:get|measure|read)\w*(?:Width|Height|Rect|Rects|Size|Bounds|Position)$/;
4042

43+
const IMPERATIVE_DOM_MUTATION_NAMES: ReadonlySet<string> = new Set([
44+
"blur",
45+
"focus",
46+
"restoreSelection",
47+
"scroll",
48+
"scrollBy",
49+
"scrollIntoView",
50+
"scrollTo",
51+
"setRangeText",
52+
"setSelectionRange",
53+
]);
54+
4155
const subtreeReadsDomMeasurement = (root: EsTreeNode | null | undefined): boolean => {
4256
if (!root) return false;
4357
let found = false;
@@ -61,17 +75,14 @@ const subtreeReadsDomMeasurement = (root: EsTreeNode | null | undefined): boolea
6175
return found;
6276
};
6377

64-
// Local function bindings whose body reads DOM measurements, including
65-
// hook-wrapped ones (`const measure = useCallback(() => el.offsetWidth)`).
66-
const collectMeasuringFunctionNames = (program: EsTreeNode): Set<string> => {
78+
const collectFunctionNamesMatchingBody = (
79+
program: EsTreeNode,
80+
matchesBody: (body: EsTreeNode | null | undefined) => boolean,
81+
): Set<string> => {
6782
const names = new Set<string>();
6883
walkAst(program, (child: EsTreeNode) => {
6984
if (isNodeOfType(child, "FunctionDeclaration")) {
70-
if (
71-
child.id &&
72-
isNodeOfType(child.id, "Identifier") &&
73-
subtreeReadsDomMeasurement(child.body)
74-
) {
85+
if (child.id && isNodeOfType(child.id, "Identifier") && matchesBody(child.body)) {
7586
names.add(child.id.name);
7687
}
7788
return;
@@ -86,22 +97,50 @@ const collectMeasuringFunctionNames = (program: EsTreeNode): Set<string> => {
8697
) {
8798
functionValue = functionValue.arguments?.[0];
8899
}
89-
if (
90-
functionValue &&
91-
isFunctionLike(functionValue) &&
92-
subtreeReadsDomMeasurement(functionValue.body)
93-
) {
100+
if (functionValue && isFunctionLike(functionValue) && matchesBody(functionValue.body)) {
94101
names.add(child.id.name);
95102
}
96103
});
97104
return names;
98105
};
99106

100-
const callsAnyName = (root: EsTreeNode | null | undefined, names: ReadonlySet<string>): boolean => {
101-
if (!root || names.size === 0) return false;
107+
const collectMeasuringFunctionNames = (program: EsTreeNode): Set<string> =>
108+
collectFunctionNamesMatchingBody(program, subtreeReadsDomMeasurement);
109+
110+
const subtreeMutatesDomImperatively = (root: EsTreeNode | null | undefined): boolean => {
111+
if (!root || isFunctionLike(root)) return false;
112+
let found = false;
113+
walkAst(root, (child: EsTreeNode) => {
114+
if (found) return false;
115+
if (child !== root && isFunctionLike(child)) return false;
116+
if (!isNodeOfType(child, "CallExpression")) return;
117+
const callee = stripParenExpression(child.callee);
118+
const propertyName = isNodeOfType(callee, "MemberExpression")
119+
? getStaticPropertyName(callee)
120+
: null;
121+
if (propertyName !== null && IMPERATIVE_DOM_MUTATION_NAMES.has(propertyName)) {
122+
found = true;
123+
return false;
124+
}
125+
});
126+
return found;
127+
};
128+
129+
const collectImperativeDomFunctionNames = (program: EsTreeNode): Set<string> =>
130+
collectFunctionNamesMatchingBody(program, subtreeMutatesDomImperatively);
131+
132+
const callsAnyName = (
133+
root: EsTreeNode | null | undefined,
134+
names: ReadonlySet<string>,
135+
shouldSkipNestedFunctions = false,
136+
): boolean => {
137+
if (!root || names.size === 0 || (shouldSkipNestedFunctions && isFunctionLike(root))) {
138+
return false;
139+
}
102140
let found = false;
103141
walkAst(root, (child: EsTreeNode) => {
104142
if (found) return false;
143+
if (shouldSkipNestedFunctions && child !== root && isFunctionLike(child)) return false;
105144
if (
106145
isNodeOfType(child, "CallExpression") &&
107146
isNodeOfType(child.callee, "Identifier") &&
@@ -113,6 +152,45 @@ const callsAnyName = (root: EsTreeNode | null | undefined, names: ReadonlySet<st
113152
return found;
114153
};
115154

155+
const isFollowedByImperativeDomMutation = (
156+
call: EsTreeNode,
157+
imperativeDomFunctionNames: ReadonlySet<string>,
158+
): boolean => {
159+
let statement: EsTreeNode = call;
160+
let parent = statement.parent;
161+
while (parent) {
162+
const statements =
163+
isNodeOfType(parent, "BlockStatement") ||
164+
isNodeOfType(parent, "Program") ||
165+
isNodeOfType(parent, "StaticBlock")
166+
? parent.body
167+
: isNodeOfType(parent, "SwitchCase")
168+
? parent.consequent
169+
: null;
170+
if (statements) {
171+
const statementIndex = statements.findIndex(
172+
(siblingStatement) => siblingStatement === statement,
173+
);
174+
if (statementIndex >= 0) {
175+
const nextStatement = statements[statementIndex + 1];
176+
return (
177+
subtreeMutatesDomImperatively(nextStatement) ||
178+
callsAnyName(nextStatement, imperativeDomFunctionNames, true)
179+
);
180+
}
181+
}
182+
if (
183+
isFunctionLike(parent) ||
184+
(parent.type.endsWith("Statement") && !isNodeOfType(parent, "ExpressionStatement"))
185+
) {
186+
return false;
187+
}
188+
statement = parent;
189+
parent = parent.parent;
190+
}
191+
return false;
192+
};
193+
116194
const isInsideStartViewTransition = (node: EsTreeNode): boolean => {
117195
let cursor: EsTreeNode | null | undefined = node.parent;
118196
while (cursor) {
@@ -172,6 +250,7 @@ const importsImperativeDomLibrary = (program: EsTreeNode): boolean => {
172250
// diagnostic.
173251
const hasExemptFlushSyncCall = (program: EsTreeNode, localName: string): boolean => {
174252
const measuringFunctionNames = collectMeasuringFunctionNames(program);
253+
const imperativeDomFunctionNames = collectImperativeDomFunctionNames(program);
175254
let exempt = false;
176255
walkAst(program, (child: EsTreeNode) => {
177256
if (exempt) return false;
@@ -184,7 +263,8 @@ const hasExemptFlushSyncCall = (program: EsTreeNode, localName: string): boolean
184263
}
185264
if (
186265
isInsideStartViewTransition(child) ||
187-
enclosingFunctionChainReadsMeasurement(child, measuringFunctionNames)
266+
enclosingFunctionChainReadsMeasurement(child, measuringFunctionNames) ||
267+
isFollowedByImperativeDomMutation(child, imperativeDomFunctionNames)
188268
) {
189269
exempt = true;
190270
return false;

0 commit comments

Comments
 (0)