Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/fix-addEventListener-cleanup.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"oxlint-plugin-react-doctor": patch
"react-doctor": patch
---

Recognize callable listener disposers, exhaustive cleanup of mapped subscription collections, and guarded timers owned by effect-local helpers in `effect-needs-cleanup`.
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// rule: effect-needs-cleanup
// weakness: control-flow
// source: PR #1559 generated ownership matrix
// verdict: pass

import { useEffect } from "react";

export const OwnedReplay = ({ condition, sources }) => {
useEffect(() => {
let timer = null;
const arm = () => {
if (timer != null) clearTimeout(timer);
timer = setTimeout(() => {}, 30000);
};
arm();
arm();
return () => clearTimeout(timer);
}, []);

useEffect(() => {
const unsubscribers = sources.map((source) => source.addListener("change", () => {}));
const ownedUnsubscribers = unsubscribers.slice();
return () => {
for (const unsubscribe of ownedUnsubscribers) {
unsubscribe();
if (condition) continue;
}
};
}, [condition, sources]);

return null;
};
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// rule: effect-needs-cleanup
// weakness: control-flow
// expect: diagnostic
// source: PR #1380 adversarial review — clearing the replay collection loses registrations
// verdict: fail

import { useEffect } from "react";

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// rule: effect-needs-cleanup
// weakness: async-lifecycle-provenance
// source: issue #1241 adversarial review
// verdict: fail
import { useEffect } from "react";

export const RepeatedReminder = ({ syncReminder }: { syncReminder: () => Promise<void> }) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// rule: effect-needs-cleanup
// weakness: library-idiom
// source: issue #1558
// verdict: pass

import NetInfo from "@react-native-community/netinfo";
import { AppState } from "react-native";
import { useEffect } from "react";

export const Connectivity = ({ tabs }) => {
useEffect(() => {
const unsubscribe = NetInfo.addEventListener(() => {});
return unsubscribe;
}, []);

useEffect(() => {
const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {}));
return () => unsubscribers.forEach((unsubscribe) => unsubscribe());
}, [tabs]);

useEffect(() => {
let timer = null;
const disarm = () => {
if (timer != null) {
clearTimeout(timer);
timer = null;
}
};
const arm = () => {
if (timer != null) return;
timer = setTimeout(() => {}, 30000);
};
const subscription = AppState.addEventListener("change", arm);
arm();
return () => {
disarm();
subscription.remove();
};
}, []);

return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// rule: effect-needs-cleanup
// weakness: copy-tracking
// source: PR #1559 parity false positive
// verdict: pass

import { useEffect } from "react";

export const ListenerTimerCollection = () => {
useEffect(() => {
const timers = [];
const handleResize = () => {
timers.push(setTimeout(() => {}, 100));
timers.push(setTimeout(() => {}, 200));
};
window.addEventListener("resize", handleResize);
return () => {
window.removeEventListener("resize", handleResize);
timers.forEach(clearTimeout);
};
}, []);
return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// rule: effect-needs-cleanup
// weakness: wrapper-transparency
// source: PR #1559 parity false positive
// verdict: pass

import { useEffect, useRef } from "react";

export const ListenerTimerHelper = () => {
const timerRef = useRef(null);
useEffect(() => {
const clearTimer = () => clearTimeout(timerRef.current);
const handleResize = () => {
clearTimer();
timerRef.current = setTimeout(() => {}, 100);
};
window.addEventListener("resize", handleResize);
return () => {
window.removeEventListener("resize", handleResize);
clearTimer();
};
}, []);
return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// rule: effect-needs-cleanup
// weakness: control-flow
// source: PR #1559 parity false positive
// verdict: pass

import { useEffect, useRef } from "react";

export const ListenerTimerRef = () => {
const timerRef = useRef(null);
useEffect(() => {
const handleResize = () => {
if (timerRef.current) clearTimeout(timerRef.current);
timerRef.current = setTimeout(() => {}, 100);
};
window.addEventListener("resize", handleResize);
return () => {
window.removeEventListener("resize", handleResize);
clearTimeout(timerRef.current);
};
}, []);
return null;
};
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// rule: effect-needs-cleanup
// weakness: control-flow
// expect: diagnostic
// source: PR #1380 Bugbot follow-up — overwriting an entry loses its registration pair
// verdict: fail

import { useEffect } from "react";

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// rule: effect-needs-cleanup
// weakness: control-flow
// source: PR #1559 parity false positive
// verdict: pass

import { useEffect } from "react";

export const RecursiveOneShotTimer = () => {
useEffect(() => {
let timer = null;
const schedule = () => {
timer = setTimeout(schedule, 100);
};
schedule();
return () => clearTimeout(timer);
}, []);
return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// rule: effect-needs-cleanup
// weakness: async-lifecycle-cleanup-control-flow
// source: PR #1559 generated ownership matrix
// verdict: fail

import { useEffect } from "react";

export const DeferredHelperAfterCleanup = () => {
useEffect(() => {
let timer = null;
const arm = () => {
if (timer != null) return;
timer = setTimeout(() => {}, 30000);
};
Promise.resolve().then(arm);
return () => clearTimeout(timer);
}, []);

return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// rule: effect-needs-cleanup
// weakness: cleanup-provenance
// source: PR #1559 generated ownership matrix
// verdict: fail

import { useEffect } from "react";

export const EscapedDisposerCollection = ({ register, sources }) => {
useEffect(() => {
const unsubscribers = sources.map((source) => source.addListener("change", () => {}));
register(unsubscribers);
return () => unsubscribers.forEach((unsubscribe) => unsubscribe());
}, [register, sources]);

return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// rule: effect-needs-cleanup
// weakness: cleanup-provenance
// source: PR #1559 ship review
// verdict: fail

import { useEffect } from "react";

export const HelperMutatedDisposerCollection = ({ sources }) => {
useEffect(() => {
const unsubscribers = sources.map((source) => source.addListener("change", () => {}));
const dropLast = () => unsubscribers.pop();
dropLast();
return () => unsubscribers.forEach((unsubscribe) => unsubscribe());
}, [sources]);

return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// rule: effect-needs-cleanup
// weakness: library-idiom
// source: PR #1559 generated ownership matrix
// verdict: fail

import { document as importedDocument } from "global-jsdom";
import { useEffect } from "react";

export const ImportedDomWrapperDisposer = () => {
useEffect(() => {
const dispose = importedDocument.addEventListener("change", () => {});
return () => dispose();
}, []);

return null;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// rule: effect-needs-cleanup
// weakness: control-flow
// source: PR #1559 adversarial review
// verdict: fail

import { useEffect, useLayoutEffect, useRef } from "react";

export const TimersAndListeners = ({ tabs, videoId }) => {
const timerRef = useRef(null);

useEffect(() => {
const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {}));
unsubscribers.pop();
return () => unsubscribers.forEach((unsubscribe) => unsubscribe());
}, [tabs]);

useLayoutEffect(() => {
timerRef.current = setTimeout(() => {}, 4000);
}, [videoId]);

useEffect(() => () => clearTimeout(timerRef.current), []);

return null;
};
9 changes: 5 additions & 4 deletions packages/fuzz/scripts/hunt-false-positives.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,8 @@ import { reactDoctorRules } from "../../oxlint-plugin-react-doctor/src/plugin/ru
import { runRule } from "../../oxlint-plugin-react-doctor/src/test-utils/run-rule.js";
import { loadFuzzCorpus } from "../src/load-fuzz-corpus.js";

// False-positive hunt over ground-truth-valid code. Every file in
// corpus/regressions/ is a CONFIRMED-valid program (that's the corpus
// contract), so:
// False-positive hunt over ground-truth-valid code. Files marked with
// `verdict: fail` are true-positive liveness fixtures and are excluded, so:
// - the seed's own named rule firing on it => regression (hard FP)
// - any OTHER rule firing on it => FP candidate for triage
// Optionally extends the hunt to real-world corpus files (FP candidates
Expand Down Expand Up @@ -58,7 +57,9 @@ const isHuntableRule = (entry: (typeof reactDoctorRules)[number]): boolean => {
return requires.every((capability) => capability === "react");
};

const seeds = loadFuzzCorpus(regressionsDirectory);
const seeds = loadFuzzCorpus(regressionsDirectory).filter(
(seed) => !/^\/\/ verdict: fail$/m.test(seed.code),
);
const hits: SeedHit[] = [];
for (const seed of seeds) {
const namedRules = namedRulesFor(seed.code);
Expand Down
49 changes: 49 additions & 0 deletions packages/fuzz/src/generate-fuzz-program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,55 @@ const SCENARIO_POOL: ReadonlyArray<SnippetBuilder> = [
`}, []);`,
].join("\n ");
},
(random) => {
const allocationBody = random.pick([
`if (fuzzTimer != null) return;\n fuzzTimer = setTimeout(handle, 250);`,
`if (fuzzTimer != null) clearTimeout(fuzzTimer);\n fuzzTimer = setTimeout(handle, 250);`,
`clearTimeout(fuzzTimer);\n fuzzTimer = setTimeout(handle, 250);`,
`if (condition) clearTimeout(fuzzTimer);\n fuzzTimer = setTimeout(handle, 250);`,
`fuzzTimer = setTimeout(handle, 250);`,
]);
const invocationKind = random.pick(["direct", "listener", "promise"]);
let invocationBody = `Promise.resolve().then(fuzzArm);`;
if (invocationKind === "direct") {
invocationBody = `fuzzArm(); fuzzArm();`;
} else if (invocationKind === "listener") {
invocationBody = `const fuzzUnsubscribe = config.addListener("change", fuzzArm);`;
}
const listenerCleanup = invocationKind === "listener" ? ` fuzzUnsubscribe();` : "";
return [
`useEffect(() => {`,
` let fuzzTimer = null;`,
` const fuzzArm = () => {`,
` ${allocationBody}`,
` };`,
` ${invocationBody}`,
` return () => { clearTimeout(fuzzTimer);${listenerCleanup} };`,
`}, [config]);`,
].join("\n ");
},
(random) => {
const storageBody = random.pick([
`const fuzzOwnedDisposers = items.map((item) => config.addListener(item, handle));`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); const fuzzOwnedDisposers = fuzzDisposers.slice();`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); const fuzzOwnedDisposers = [...fuzzDisposers];`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); const fuzzOwnedDisposers = Array.from(fuzzDisposers);`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); const fuzzOwnedDisposers = fuzzDisposers.slice(); fuzzDisposers.length = 0;`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); fuzzDisposers.length = 0; const fuzzOwnedDisposers = fuzzDisposers.slice();`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); const fuzzOwnedDisposers = fuzzDisposers.slice(); fuzzOwnedDisposers.length = 0;`,
`const fuzzDisposers = items.map((item) => config.addListener(item, handle)); const fuzzOwnedDisposers = fuzzDisposers.filter(Boolean);`,
`const fuzzOwnedDisposers = items.map((item) => config.addListener(item, handle)); register(fuzzOwnedDisposers);`,
]);
const cleanupBody = random.chance(0.5)
? `fuzzOwnedDisposers.forEach((dispose) => dispose());`
: `for (const dispose of fuzzOwnedDisposers) { dispose(); if (condition) continue; }`;
return [
`useEffect(() => {`,
` ${storageBody}`,
` return () => { ${cleanupBody} };`,
`}, [condition, config, items]);`,
].join("\n ");
},
(random) =>
[
`const response = await fetch(url);`,
Expand Down
4 changes: 4 additions & 0 deletions packages/fuzz/src/snippet-pools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export const EFFECT_SNIPPET_POOL = [
`useEffect(() => { const observer = new MutationObserver(handle); observer.observe(document.body, { childList: true, subtree: true }); return () => observer.disconnect(); }, []);`,
`useEffect(() => { let rafId; const loop = () => { handle(); rafId = requestAnimationFrame(loop); }; rafId = requestAnimationFrame(loop); return () => cancelAnimationFrame(rafId); }, []);`,
`useEffect(() => { const loop = () => { handle(); requestAnimationFrame(loop); }; requestAnimationFrame(loop); }, []);`,
`const fuzzListenerTimerRef = useRef(null); useEffect(() => { const handleFuzzResizeTimer = () => { if (fuzzListenerTimerRef.current) clearTimeout(fuzzListenerTimerRef.current); fuzzListenerTimerRef.current = setTimeout(handle, 100); }; window.addEventListener("resize", handleFuzzResizeTimer); return () => { window.removeEventListener("resize", handleFuzzResizeTimer); clearTimeout(fuzzListenerTimerRef.current); }; }, []);`,
`const fuzzHelperTimerRef = useRef(null); useEffect(() => { const clearFuzzHelperTimer = () => clearTimeout(fuzzHelperTimerRef.current); const handleFuzzHelperTimer = () => { clearFuzzHelperTimer(); fuzzHelperTimerRef.current = setTimeout(handle, 100); }; window.addEventListener("scroll", handleFuzzHelperTimer); return () => { window.removeEventListener("scroll", handleFuzzHelperTimer); clearFuzzHelperTimer(); }; }, []);`,
`useEffect(() => { const fuzzOwnedTimers = []; const handleFuzzOwnedTimers = () => { fuzzOwnedTimers.push(setTimeout(handle, 100)); fuzzOwnedTimers.push(setTimeout(handle, 200)); }; window.addEventListener("resize", handleFuzzOwnedTimers); return () => { window.removeEventListener("resize", handleFuzzOwnedTimers); fuzzOwnedTimers.forEach(clearTimeout); }; }, []);`,
`useEffect(() => { let fuzzRecursiveTimer = null; const scheduleFuzzRecursiveTimer = () => { fuzzRecursiveTimer = setTimeout(scheduleFuzzRecursiveTimer, 100); }; scheduleFuzzRecursiveTimer(); return () => clearTimeout(fuzzRecursiveTimer); }, []);`,
`useEffect(() => { const id = setInterval(() => setState((prev) => prev + 1), 1000); return () => clearInterval(id); }, []);`,
`useEffect(() => { const id = window.setTimeout(() => setState(0), 500); return () => window.clearTimeout(id); }, [value]);`,
`useEffect(() => { let cancelled = false; const load = async () => { const result = await fetch(url); if (!cancelled) setState(await result.json()); }; load(); return () => { cancelled = true; }; }, [url]);`,
Expand Down
Loading
Loading