Skip to content

Commit e8617d5

Browse files
aidenybaiaidenybai
committed
fix: track effect resources through function declarations
1 parent e5824ab commit e8617d5

6 files changed

Lines changed: 307 additions & 29 deletions

File tree

‎.changeset/effect-helper-function-bindings.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"oxlint-plugin-react-doctor": patch
3+
"eslint-plugin-react-doctor": patch
4+
"react-doctor": patch
5+
---
6+
7+
Detect effect resources created by called function declarations while preserving synchronous helper cleanup and directly returned disposers.

‎packages/fuzz/corpus/regressions/effect-needs-cleanup--helper-returned-disposer.tsx‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// rule: effect-needs-cleanup
2+
// weakness: wrapper-transparency
3+
// source: synthetic function-binding regression
4+
// verdict: pass
5+
import { useEffect } from "react";
6+
7+
export const Probe = () => {
8+
useEffect(() => {
9+
function subscribe() {
10+
const timer = setInterval(() => {}, 1000);
11+
return () => clearInterval(timer);
12+
}
13+
return subscribe();
14+
}, []);
15+
return null;
16+
};

‎packages/fuzz/corpus/regressions/effect-needs-cleanup--helper-synchronous-release.tsx‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// rule: effect-needs-cleanup
2+
// weakness: wrapper-transparency
3+
// source: synthetic function-binding regression
4+
// verdict: pass
5+
import { useEffect } from "react";
6+
7+
export const Probe = () => {
8+
useEffect(() => {
9+
function measure() {
10+
const timer = setInterval(() => {}, 1000);
11+
clearInterval(timer);
12+
}
13+
measure();
14+
}, []);
15+
return null;
16+
};

‎packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup-function-bindings.test.ts‎

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
import { describe, expect, it } from "vite-plus/test";
2+
import { runRule } from "../../../test-utils/run-rule.js";
3+
import { effectNeedsCleanup } from "./effect-needs-cleanup.js";
4+
5+
describe("effect-needs-cleanup function bindings", () => {
6+
it.each([
7+
["discarded returned timer", "function start() { return setInterval(tick, 1000); } start();"],
8+
[
9+
"wrong returned timer cleanup",
10+
"function start() { return setInterval(tick, 1000); } const timer = start(); return () => clearInterval(other);",
11+
],
12+
[
13+
"two timer allocations",
14+
"function start() { return setInterval(tick, 1000); } start(); const timer = start(); return () => clearInterval(timer);",
15+
],
16+
[
17+
"conditional outer cleanup",
18+
"let timer; function start() { timer = setInterval(tick, 1000); } start(); return () => { if (ready) clearInterval(timer); };",
19+
],
20+
[
21+
"async returned timer",
22+
"async function start() { return setInterval(tick, 1000); } const timer = start(); return () => clearInterval(timer);",
23+
],
24+
["direct call", "function start() { setInterval(tick, 1000); } start();"],
25+
["hoisted call", "start(); function start() { setInterval(tick, 1000); }"],
26+
[
27+
"transitive call",
28+
"function start() { attach(); } function attach() { setInterval(tick, 1000); } start();",
29+
],
30+
[
31+
"recursive call",
32+
"function start() { setInterval(tick, 1000); if (retry) start(); } start();",
33+
],
34+
[
35+
"discarded disposer",
36+
"function start() { const timer = setInterval(tick, 1000); return () => clearInterval(timer); } start();",
37+
],
38+
[
39+
"second discarded disposer",
40+
"function start() { const timer = setInterval(tick, 1000); return () => clearInterval(timer); } start(); return start();",
41+
],
42+
[
43+
"wrong disposer",
44+
"function start() { const timer = setInterval(tick, 1000); return () => clearInterval(other); } return start();",
45+
],
46+
[
47+
"conditional disposer",
48+
"function start() { const timer = setInterval(tick, 1000); if (ready) return () => clearInterval(timer); } return start();",
49+
],
50+
[
51+
"async disposer",
52+
"async function start() { const timer = setInterval(tick, 1000); return () => clearInterval(timer); } return start();",
53+
],
54+
[
55+
"conditional clear",
56+
"function start() { const timer = setInterval(tick, 1000); if (ready) clearInterval(timer); } start();",
57+
],
58+
[
59+
"wrong handle",
60+
"function start() { const timer = setInterval(tick, 1000); clearInterval(other); } start();",
61+
],
62+
[
63+
"deferred clear",
64+
"function start() { const timer = setInterval(tick, 1000); queueMicrotask(() => clearInterval(timer)); } start();",
65+
],
66+
[
67+
"async clear",
68+
"async function start() { const timer = setInterval(tick, 1000); await pending; clearInterval(timer); } start();",
69+
],
70+
["arrow helper", "const start = () => { setInterval(tick, 1000); }; start();"],
71+
])("reports a leaked timer through a %s", (_label, body) => {
72+
const result = runRule(
73+
effectNeedsCleanup,
74+
`import { useEffect } from "react";
75+
export const Clock = () => {
76+
useEffect(() => { ${body} }, []);
77+
return null;
78+
};`,
79+
);
80+
expect(result.parseErrors).toEqual([]);
81+
expect(result.diagnostics).toHaveLength(1);
82+
expect(result.diagnostics[0]?.message).toContain("setInterval");
83+
});
84+
85+
it.each([
86+
[
87+
"outer stored timer cleanup",
88+
"let timer; function start() { timer = setInterval(tick, 1000); } start(); return () => clearInterval(timer);",
89+
],
90+
[
91+
"hoisted returned disposer",
92+
"return start(); function start() { const timer = setInterval(tick, 1000); return () => clearInterval(timer); }",
93+
],
94+
[
95+
"returned local timer cleanup",
96+
"function start() { const timer = setInterval(tick, 1000); return timer; } const timer = start(); return () => clearInterval(timer);",
97+
],
98+
[
99+
"returned timer cleanup",
100+
"function start() { return setInterval(tick, 1000); } const timer = start(); return () => clearInterval(timer);",
101+
],
102+
["uncalled generator body", "function* start() { setInterval(tick, 1000); yield 1; } start();"],
103+
[
104+
"returned disposer",
105+
"function start() { const timer = setInterval(tick, 1000); return () => clearInterval(timer); } return start();",
106+
],
107+
[
108+
"cleared arrow helper",
109+
"const start = () => { const timer = setInterval(tick, 1000); clearInterval(timer); }; start();",
110+
],
111+
["uncalled helper", "function start() { setInterval(tick, 1000); }"],
112+
[
113+
"reassigned helper",
114+
"function start() { setInterval(tick, 1000); } start = () => {}; start();",
115+
],
116+
[
117+
"shadowed helper",
118+
"function start() { setInterval(tick, 1000); } { const start = () => {}; start(); }",
119+
],
120+
[
121+
"parameter shadow",
122+
"function start() { setInterval(tick, 1000); } ((start) => start())(() => {});",
123+
],
124+
[
125+
"cleaned helper",
126+
"function start() { const timer = setInterval(tick, 1000); clearInterval(timer); } start();",
127+
],
128+
])("does not report a %s", (_label, body) => {
129+
const result = runRule(
130+
effectNeedsCleanup,
131+
`import { useEffect } from "react";
132+
export const Clock = () => {
133+
useEffect(() => { ${body} }, []);
134+
return null;
135+
};`,
136+
);
137+
expect(result.parseErrors).toEqual([]);
138+
expect(result.diagnostics).toEqual([]);
139+
});
140+
141+
it("does not call an unreferenced block-scoped declaration", () => {
142+
const result = runRule(
143+
effectNeedsCleanup,
144+
`import { useEffect } from "react";
145+
export const Clock = () => {
146+
useEffect(() => {
147+
function start() {}
148+
{ function start() { setInterval(tick, 1000); } }
149+
start();
150+
}, []);
151+
return null;
152+
};`,
153+
);
154+
expect(result.parseErrors).toEqual([]);
155+
expect(result.diagnostics).toEqual([]);
156+
});
157+
});

‎packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts‎

Lines changed: 89 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@ import { getFinalSequenceExpressionValue } from "../../utils/get-final-sequence-
4040
import { doNodesCoverEveryPathAfterNode } from "../../utils/do-nodes-cover-every-path-after-node.js";
4141
import { doNodesCoverEveryPathFromFunctionEntry } from "../../utils/do-nodes-cover-every-path-from-function-entry.js";
4242
import { getFunctionBindingIdentifier } from "../../utils/get-function-binding-name.js";
43+
import { getEquivalentSymbols } from "../../utils/get-equivalent-symbols.js";
44+
import { getSingleReturnExpression } from "../../utils/get-single-return-expression.js";
45+
import { functionReturnsMatchingExpressionOnEveryPathAfterNode } from "../../utils/function-returns-matching-expression.js";
4346
import { getImportDeclarationForSymbol } from "../../utils/get-import-declaration-for-symbol.js";
4447
import { getRangeStart } from "../../utils/get-range-start.js";
4548
import { getStaticPropertyKeyName } from "../../utils/get-static-property-key-name.js";
@@ -1064,6 +1067,31 @@ const findAssignedResourceKey = (
10641067
const parentNode = currentNode.parent;
10651068
if (isNodeOfType(parentNode, "VariableDeclarator") && parentNode.init === currentNode) {
10661069
const localKey = resolveExpressionKey(parentNode.id, context);
1070+
const symbol = isNodeOfType(parentNode.id, "Identifier")
1071+
? context.scopes.symbolFor(parentNode.id)
1072+
: null;
1073+
const factory = findEnclosingFunction(currentNode);
1074+
const caller = factory ? findEnclosingFunction(factory) : null;
1075+
if (
1076+
symbol?.kind === "const" &&
1077+
isFunctionLike(factory) &&
1078+
!factory.async &&
1079+
!factory.generator &&
1080+
factory.params.length === 0 &&
1081+
caller &&
1082+
functionReturnsMatchingExpressionOnEveryPathAfterNode(
1083+
factory,
1084+
currentNode,
1085+
context.scopes,
1086+
(expression) => context.scopes.symbolFor(stripParenExpression(expression)) === symbol,
1087+
context.cfg,
1088+
)
1089+
) {
1090+
const invocation = findSingleDirectInvocation(factory, caller, context);
1091+
if (isNodeOfType(invocation, "CallExpression") && invocation.arguments.length === 0) {
1092+
return findAssignedResourceKey(invocation, context, includeRetainedStorage) ?? localKey;
1093+
}
1094+
}
10671095
if (!includeRetainedStorage) return localKey;
10681096
const retainedStorage = findRetainedResourceStorage(resourceNode, context);
10691097
return retainedStorage?.key ?? localKey;
@@ -1079,6 +1107,23 @@ const findAssignedResourceKey = (
10791107
}
10801108
return resolveExpressionKey(parentNode.left, context);
10811109
}
1110+
if (isNodeOfType(parentNode, "ReturnStatement") && parentNode.argument === currentNode) {
1111+
const factory = findEnclosingFunction(currentNode);
1112+
const caller = factory ? findEnclosingFunction(factory) : null;
1113+
if (
1114+
isFunctionLike(factory) &&
1115+
!factory.async &&
1116+
!factory.generator &&
1117+
factory.params.length === 0 &&
1118+
getSingleReturnExpression(factory) === currentNode &&
1119+
caller
1120+
) {
1121+
const invocation = findSingleDirectInvocation(factory, caller, context);
1122+
if (isNodeOfType(invocation, "CallExpression") && invocation.arguments.length === 0) {
1123+
return findAssignedResourceKey(invocation, context, includeRetainedStorage);
1124+
}
1125+
}
1126+
}
10821127
return includeRetainedStorage
10831128
? (resolveRetainedResourceStorage(currentNode, context)?.key ?? null)
10841129
: null;
@@ -1699,11 +1744,19 @@ const removeSynchronouslyReleasedUsages = (
16991744
}
17001745
if (!isNodeOfType(callback.body, "BlockStatement")) return usages;
17011746
const releaseCalls: EsTreeNode[] = [];
1702-
walkInsideStatementBlocks(callback.body, (child: EsTreeNode) => {
1703-
const callNode = isNodeOfType(child, "ChainExpression") ? child.expression : child;
1704-
if (!isNodeOfType(callNode, "CallExpression")) return;
1705-
releaseCalls.push(child);
1706-
});
1747+
for (const invokedFunction of collectSynchronouslyEffectInvokedFunctions(
1748+
callback,
1749+
context.scopes,
1750+
)) {
1751+
if (!isFunctionLike(invokedFunction)) continue;
1752+
if (invokedFunction !== callback && (invokedFunction.async || invokedFunction.generator))
1753+
continue;
1754+
walkInsideStatementBlocks(invokedFunction.body, (child: EsTreeNode) => {
1755+
const callNode = isNodeOfType(child, "ChainExpression") ? child.expression : child;
1756+
if (!isNodeOfType(callNode, "CallExpression")) return;
1757+
releaseCalls.push(child);
1758+
});
1759+
}
17071760
if (releaseCalls.length === 0) return usages;
17081761
return usages.filter((usage) => {
17091762
const usageStart = getRangeStart(usage.node);
@@ -2799,13 +2852,14 @@ const findSingleDirectInvocation = (
27992852
if (!bindingIdentifier || resolveStableValue(bindingIdentifier, context) !== functionNode) {
28002853
return null;
28012854
}
2802-
const symbol = context.scopes.symbolFor(bindingIdentifier);
2803-
if (!symbol) return null;
2804-
const invocationCalls = symbol.references.flatMap((reference) => {
2855+
const references = getEquivalentSymbols(bindingIdentifier, context.scopes).flatMap(
2856+
(symbol) => symbol.references,
2857+
);
2858+
const invocationCalls = references.flatMap((reference) => {
28052859
const callNode = findDirectCallForReference(reference.identifier);
28062860
return callNode ? [callNode] : [];
28072861
});
2808-
if (invocationCalls.length !== 1 || symbol.references.length !== 1) return null;
2862+
if (invocationCalls.length !== 1 || references.length !== 1) return null;
28092863
const invocationCall = invocationCalls[0];
28102864
return findEnclosingFunction(invocationCall) === caller &&
28112865
isNodeReachableWithinFunction(invocationCall, context)
@@ -3410,13 +3464,7 @@ const callbackReturnsCleanupForUsage = (
34103464
context: RuleContext,
34113465
isReactRefCallback = false,
34123466
): boolean => {
3413-
if (
3414-
!isNodeOfType(callback, "ArrowFunctionExpression") &&
3415-
!isNodeOfType(callback, "FunctionExpression")
3416-
) {
3417-
return false;
3418-
}
3419-
if (callback.async) return false;
3467+
if (!isFunctionLike(callback) || callback.async || callback.generator) return false;
34203468
const doesReturnedValueReleaseUsage = (returnedValue: EsTreeNode): boolean => {
34213469
if (doesBoundCleanupReleaseUsage(returnedValue, usage, context)) return true;
34223470
const stableCleanupValue = resolveStableValue(returnedValue, context);
@@ -6718,13 +6766,36 @@ const effectHasCleanupForUsage = (
67186766
findEnclosingFunction(assignedHandleSymbol.declarationNode) === callback,
67196767
)));
67206768
const matchingCleanupReturns: EsTreeNode[] = [];
6769+
let hasReturnedFactoryCleanup = false;
67216770
walkInsideStatementBlocks(callback.body, (child: EsTreeNode) => {
67226771
if (!isNodeOfType(child, "ReturnStatement")) return;
67236772
const returnStart = getRangeStart(child);
67246773
const usageStart = getRangeStart(usage.node);
6725-
if (returnStart !== null && usageStart !== null && returnStart < usageStart) return;
67266774
const returnedValue = child.argument ? stripParenExpression(child.argument) : null;
67276775
if (!returnedValue) return;
6776+
if (isNodeOfType(returnedValue, "CallExpression") && returnedValue.arguments.length === 0) {
6777+
const factory = resolveStableValue(returnedValue.callee, context);
6778+
if (
6779+
isFunctionLike(factory) &&
6780+
factory.params.length === 0 &&
6781+
findEnclosingFunction(factory) === callback &&
6782+
findEnclosingFunction(usage.node) === factory &&
6783+
callbackReturnsCleanupForUsage(factory, usage, context)
6784+
) {
6785+
const factoryReferences: EsTreeNode[] = [];
6786+
walkAst(callback, (candidate) => {
6787+
const reference = context.scopes.referenceFor(candidate);
6788+
if (reference && resolveStableValue(candidate, context) === factory) {
6789+
factoryReferences.push(candidate);
6790+
}
6791+
});
6792+
if (factoryReferences.length === 1 && factoryReferences[0] === returnedValue.callee) {
6793+
hasReturnedFactoryCleanup = true;
6794+
return;
6795+
}
6796+
}
6797+
}
6798+
if (returnStart !== null && usageStart !== null && returnStart < usageStart) return;
67286799
if (doesBoundCleanupReleaseUsage(returnedValue, usage, context)) {
67296800
matchingCleanupReturns.push(child);
67306801
return;
@@ -6772,6 +6843,7 @@ const effectHasCleanupForUsage = (
67726843
matchingCleanupReturns.push(child);
67736844
}
67746845
});
6846+
if (hasReturnedFactoryCleanup) return true;
67756847
if (
67766848
hasOwnedObserverHelperCleanup(callback, usage, allUsages, matchingCleanupReturns, context) ||
67776849
hasGuardedDeferredCleanup(callback, usage, matchingCleanupReturns, context)

0 commit comments

Comments
 (0)