diff --git a/.changeset/fix-addEventListener-cleanup.md b/.changeset/fix-addEventListener-cleanup.md new file mode 100644 index 0000000000..8ac5bcc333 --- /dev/null +++ b/.changeset/fix-addEventListener-cleanup.md @@ -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`. diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--adversarial-owned-replay.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--adversarial-owned-replay.tsx new file mode 100644 index 0000000000..37833497b7 --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--adversarial-owned-replay.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--cleared-projected-collection.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--cleared-projected-collection.tsx index b97e4105f3..2eba5d9335 100644 --- a/packages/fuzz/corpus/regressions/effect-needs-cleanup--cleared-projected-collection.tsx +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--cleared-projected-collection.tsx @@ -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"; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--guarded-promise-timer-repeated.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--guarded-promise-timer-repeated.tsx index ff42de9b75..12c22d1174 100644 --- a/packages/fuzz/corpus/regressions/effect-needs-cleanup--guarded-promise-timer-repeated.tsx +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--guarded-promise-timer-repeated.tsx @@ -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 }) => { diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1558-owned-resources.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1558-owned-resources.tsx new file mode 100644 index 0000000000..77f2c97f0b --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--issue-1558-owned-resources.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-collection.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-collection.tsx new file mode 100644 index 0000000000..0fdee788b2 --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-collection.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-helper.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-helper.tsx new file mode 100644 index 0000000000..17090468f2 --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-helper.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-ref.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-ref.tsx new file mode 100644 index 0000000000..61c1cbe8e9 --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--listener-timer-ref.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--overwritten-projected-entry.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--overwritten-projected-entry.tsx index b10bfbcd89..09700b100e 100644 --- a/packages/fuzz/corpus/regressions/effect-needs-cleanup--overwritten-projected-entry.tsx +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--overwritten-projected-entry.tsx @@ -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"; diff --git a/packages/fuzz/corpus/regressions/effect-needs-cleanup--recursive-one-shot-timer.tsx b/packages/fuzz/corpus/regressions/effect-needs-cleanup--recursive-one-shot-timer.tsx new file mode 100644 index 0000000000..a2e5bd5b1f --- /dev/null +++ b/packages/fuzz/corpus/regressions/effect-needs-cleanup--recursive-one-shot-timer.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/true-positives/effect-needs-cleanup--deferred-helper-after-cleanup.tsx b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--deferred-helper-after-cleanup.tsx new file mode 100644 index 0000000000..cfc87775dd --- /dev/null +++ b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--deferred-helper-after-cleanup.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/true-positives/effect-needs-cleanup--escaped-disposer-collection.tsx b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--escaped-disposer-collection.tsx new file mode 100644 index 0000000000..6d6c658b88 --- /dev/null +++ b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--escaped-disposer-collection.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/true-positives/effect-needs-cleanup--helper-mutated-disposer-collection.tsx b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--helper-mutated-disposer-collection.tsx new file mode 100644 index 0000000000..f2d1641262 --- /dev/null +++ b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--helper-mutated-disposer-collection.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/true-positives/effect-needs-cleanup--imported-dom-wrapper-disposer.tsx b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--imported-dom-wrapper-disposer.tsx new file mode 100644 index 0000000000..3711bdda7a --- /dev/null +++ b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--imported-dom-wrapper-disposer.tsx @@ -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; +}; diff --git a/packages/fuzz/corpus/true-positives/effect-needs-cleanup--issue-1558-incomplete-ownership.tsx b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--issue-1558-incomplete-ownership.tsx new file mode 100644 index 0000000000..3e2219a114 --- /dev/null +++ b/packages/fuzz/corpus/true-positives/effect-needs-cleanup--issue-1558-incomplete-ownership.tsx @@ -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; +}; diff --git a/packages/fuzz/scripts/hunt-false-positives.ts b/packages/fuzz/scripts/hunt-false-positives.ts index 29b7670198..1a8bc65c4e 100644 --- a/packages/fuzz/scripts/hunt-false-positives.ts +++ b/packages/fuzz/scripts/hunt-false-positives.ts @@ -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 @@ -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); diff --git a/packages/fuzz/src/generate-fuzz-program.ts b/packages/fuzz/src/generate-fuzz-program.ts index 15f633671f..850f696e94 100644 --- a/packages/fuzz/src/generate-fuzz-program.ts +++ b/packages/fuzz/src/generate-fuzz-program.ts @@ -141,6 +141,55 @@ const SCENARIO_POOL: ReadonlyArray = [ `}, []);`, ].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);`, diff --git a/packages/fuzz/src/snippet-pools.ts b/packages/fuzz/src/snippet-pools.ts index 8cafc900ec..4b81237f3f 100644 --- a/packages/fuzz/src/snippet-pools.ts +++ b/packages/fuzz/src/snippet-pools.ts @@ -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]);`, diff --git a/packages/fuzz/src/verdict-preserving-variants.ts b/packages/fuzz/src/verdict-preserving-variants.ts index dd9aa94450..e8db81d9f0 100644 --- a/packages/fuzz/src/verdict-preserving-variants.ts +++ b/packages/fuzz/src/verdict-preserving-variants.ts @@ -1,6 +1,8 @@ import { parseFixture } from "../../oxlint-plugin-react-doctor/src/test-utils/parse-fixture.js"; +import { attachParentReferences } from "../../oxlint-plugin-react-doctor/src/test-utils/attach-parent-references.js"; import { walkAst } from "../../oxlint-plugin-react-doctor/src/plugin/utils/walk-ast.js"; import { isNodeOfType } from "../../oxlint-plugin-react-doctor/src/plugin/utils/is-node-of-type.js"; +import { findTransparentExpressionRoot } from "../../oxlint-plugin-react-doctor/src/plugin/utils/find-transparent-expression-root.js"; import type { EsTreeNode } from "../../oxlint-plugin-react-doctor/src/plugin/utils/es-tree-node.js"; import { MAX_VERDICT_VARIANT_ANCHORS } from "./constants.js"; @@ -36,6 +38,10 @@ interface SpannedNode { readonly end: number; } +interface CallReceiverSpan extends SpannedNode { + readonly needsLeadingSemicolon: boolean; +} + const hasSpan = (node: EsTreeNode | null | undefined): node is EsTreeNode & SpannedNode => Boolean(node) && typeof (node as unknown as SpannedNode).start === "number" && @@ -55,7 +61,9 @@ const applyEdits = (code: string, edits: ReadonlyArray): string => { const parseProgram = (code: string, filename: string): EsTreeNode | null => { try { const { program, errors } = parseFixture(code, { filename, forceJsx: true }); - return errors.length > 0 ? null : program; + if (errors.length > 0) return null; + attachParentReferences(program); + return program; } catch { return null; } @@ -64,8 +72,8 @@ const parseProgram = (code: string, filename: string): EsTreeNode | null => { // Anchors: `obj` in every `obj.method(...)` call — the receiver position // rules most often match structurally. `super` cannot be parenthesized and // JSX member callees don't exist, so only plain expression objects anchor. -const collectCallReceiverSpans = (program: EsTreeNode): SpannedNode[] => { - const spans: SpannedNode[] = []; +const collectCallReceiverSpans = (program: EsTreeNode): CallReceiverSpan[] => { + const spans: CallReceiverSpan[] = []; walkAst(program, (node: EsTreeNode) => { if (spans.length >= MAX_VERDICT_VARIANT_ANCHORS) return false; if (!isNodeOfType(node, "CallExpression")) return; @@ -75,7 +83,15 @@ const collectCallReceiverSpans = (program: EsTreeNode): SpannedNode[] => { const receiver = callee.object; if (!hasSpan(receiver)) return; if (isNodeOfType(receiver, "Super")) return; - spans.push({ start: receiver.start, end: receiver.end }); + const expressionRoot = findTransparentExpressionRoot(node); + const expressionStatement = expressionRoot.parent; + const statementParent = expressionStatement?.parent; + const needsLeadingSemicolon = + isNodeOfType(expressionStatement, "ExpressionStatement") && + (isNodeOfType(statementParent, "Program") || + isNodeOfType(statementParent, "BlockStatement") || + isNodeOfType(statementParent, "SwitchCase")); + spans.push({ start: receiver.start, end: receiver.end, needsLeadingSemicolon }); }); return spans; }; @@ -181,7 +197,7 @@ const buildComputedMemberVariantCode = ( const buildReceiverWrapVariant = ( code: string, - spans: ReadonlyArray, + spans: ReadonlyArray, label: string, open: string, close: string, @@ -190,7 +206,10 @@ const buildReceiverWrapVariant = ( if (spans.length === 0) return null; const edits: SpanEdit[] = []; for (const span of spans) { - edits.push({ position: span.start, insertText: open }); + edits.push({ + position: span.start, + insertText: span.needsLeadingSemicolon ? `;${open}` : open, + }); edits.push({ position: span.end, insertText: close }); } return { label, code: applyEdits(code, edits), mustPreserveVerdict }; diff --git a/packages/fuzz/tests/fuzz-harness-smoke.test.ts b/packages/fuzz/tests/fuzz-harness-smoke.test.ts index 262da30bde..aa64ae0cb6 100644 --- a/packages/fuzz/tests/fuzz-harness-smoke.test.ts +++ b/packages/fuzz/tests/fuzz-harness-smoke.test.ts @@ -192,6 +192,21 @@ describe("fuzz harness oracles", () => { expect(castReceiver?.mustPreserveVerdict).toBe(true); }); + it("keeps receiver wrappers from joining semicolonless statements", () => { + const variants = buildVerdictPreservingVariants( + `const state = React.useState(false) +React.useEffect(() => { + setTimeout(() => {}, 100) +}, [])`, + "fixture.tsx", + ); + for (const variant of variants.filter((candidate) => + candidate.label.endsWith("call receivers"), + )) { + expect(variant.code).toContain("\n;(React"); + } + }); + it("catches a rule that keys off incidental source shape", () => { const commentSensitiveRule: Rule = { id: "fuzz-smoke-invariant", diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.issue-1558.test.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.issue-1558.test.ts new file mode 100644 index 0000000000..682e642607 --- /dev/null +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.issue-1558.test.ts @@ -0,0 +1,849 @@ +import { describe, expect, it } from "vite-plus/test"; +import { runRule } from "../../../test-utils/run-rule.js"; +import { effectNeedsCleanup } from "./effect-needs-cleanup.js"; + +const validCases: ReadonlyArray = [ + [ + "returns an imported addEventListener disposer", + `import NetInfo from "@react-native-community/netinfo"; +import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const unsubscribe = NetInfo.addEventListener(() => {}); + return unsubscribe; + }, []); + return null; +};`, + ], + [ + "calls an imported addEventListener disposer from a cleanup", + `import NetInfo from "@react-native-community/netinfo"; +import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const unsubscribe = NetInfo.addEventListener(() => {}); + return () => unsubscribe(); + }, []); + return null; +};`, + ], + [ + "calls every mapped addListener disposer with forEach", + `import { useEffect } from "react"; +export const Component = ({ tabs }) => { + useEffect(() => { + const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {})); + return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); + }, [tabs]); + return null; +};`, + ], + [ + "calls every mapped addListener disposer with for-of", + `import { useEffect } from "react"; +export const Component = ({ tabs }) => { + useEffect(() => { + const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {})); + return () => { + for (const unsubscribe of unsubscribers) unsubscribe(); + }; + }, [tabs]); + return null; +};`, + ], + [ + "removes every mapped React Native subscription", + `import { useEffect } from "react"; +import { AppState } from "react-native"; +export const Component = ({ events }) => { + useEffect(() => { + const subscriptions = events.map((event) => AppState.addEventListener(event, () => {})); + return () => subscriptions.forEach((subscription) => subscription.remove()); + }, [events]); + return null; +};`, + ], + [ + "owns a guarded timer helper invoked directly and by an owned listener", + `import { useEffect } from "react"; +import { AppState } from "react-native"; +export const Component = () => { + useEffect(() => { + let timer = null; + const disarm = () => { + if (timer != null) { + clearTimeout(timer); + timer = null; + } + }; + const arm = () => { + if (timer != null) return; + timer = setTimeout(() => {}, 30000); + }; + const handleChange = (state) => state === "active" ? arm() : disarm(); + const subscription = AppState.addEventListener("change", handleChange); + arm(); + return () => { + disarm(); + subscription.remove(); + }; + }, []); + return null; +};`, + ], + [ + "cleans an effect-local object member timer through dispose", + `import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const resource = { + timer: null, + arm() { + resource.timer = setTimeout(() => {}, 30000); + }, + dispose() { + clearTimeout(resource.timer); + }, + }; + resource.arm(); + return () => resource.dispose(); + }, []); + return null; +};`, + ], + [ + "cleans a listener timer retained in a React ref", + `import { useEffect, useRef } from "react"; +export const Component = () => { + const timerRef = useRef(null); + useEffect(() => { + const handleResize = () => { + if (timerRef.current) clearTimeout(timerRef.current); + timerRef.current = setTimeout(() => {}, 30000); + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + if (timerRef.current) clearTimeout(timerRef.current); + }; + }, []); + return null; +};`, + ], + [ + "cleans a listener timer through an exact local helper", + `import { useEffect, useRef } from "react"; +export const Component = () => { + const timerRef = useRef(null); + useEffect(() => { + const clearTimer = () => { + if (timerRef.current) clearTimeout(timerRef.current); + }; + const handleResize = () => { + clearTimer(); + timerRef.current = setTimeout(() => {}, 30000); + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + clearTimer(); + }; + }, []); + return null; +};`, + ], + [ + "cleans every timer retained by an owned listener", + `import { useEffect } from "react"; +export const Component = () => { + 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; +};`, + ], + [ + "cleans every listener timer retained from a loop", + `import { useEffect } from "react"; +export const Component = ({ delays }) => { + useEffect(() => { + const timers = []; + const handleResize = () => { + for (const delay of delays) { + timers.push(setTimeout(() => {}, delay)); + } + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + timers.forEach(clearTimeout); + }; + }, [delays]); + return null; +};`, + ], + [ + "cleans the latest handle in a recursive one-shot timer", + `import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + let timer = null; + const schedule = () => { + timer = setTimeout(schedule, 30000); + }; + schedule(); + return () => clearTimeout(timer); + }, []); + return null; +};`, + ], +]; + +const invalidCases: ReadonlyArray = [ + [ + "does not treat a DOM addEventListener return value as callable cleanup", + `import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const cleanup = window.addEventListener("resize", () => {}); + return cleanup; + }, []); + return null; +};`, + ], + [ + "does not treat a typed EventTarget return value as callable cleanup", + `import { useEffect } from "react"; +export const Component = ({ target }: { target: EventTarget }) => { + useEffect(() => { + const cleanup = target.addEventListener("change", () => {}); + return () => cleanup(); + }, [target]); + return null; +};`, + ], + [ + "does not treat a React Native subscription object as callable cleanup", + `import { useEffect } from "react"; +import { AppState } from "react-native"; +export const Component = () => { + useEffect(() => { + const subscription = AppState.addEventListener("change", () => {}); + return subscription; + }, []); + return null; +};`, + ], + [ + "rejects a reassigned listener disposer handle", + `import NetInfo from "@react-native-community/netinfo"; +import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + let unsubscribe = NetInfo.addEventListener(() => {}); + unsubscribe = () => {}; + return unsubscribe; + }, []); + return null; +};`, + ], + [ + "does not call mapped DOM listener return values cleanup", + `import { useEffect } from "react"; +export const Component = ({ targets }: { targets: EventTarget[] }) => { + useEffect(() => { + const cleanups = targets.map((target) => target.addEventListener("change", () => {})); + return () => cleanups.forEach((cleanup) => cleanup()); + }, [targets]); + return null; +};`, + ], + [ + "rejects cleanup of a different disposer collection", + `import { useEffect } from "react"; +export const Component = ({ tabs, previousUnsubscribers }) => { + useEffect(() => { + tabs.map((tab) => tab.addListener("tabPress", () => {})); + return () => previousUnsubscribers.forEach((unsubscribe) => unsubscribe()); + }, [tabs, previousUnsubscribers]); + return null; +};`, + ], + [ + "rejects conditional cleanup within forEach", + `import { useEffect } from "react"; +export const Component = ({ tabs, enabled }) => { + useEffect(() => { + const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {})); + return () => unsubscribers.forEach((unsubscribe) => { + if (enabled) unsubscribe(); + }); + }, [tabs, enabled]); + return null; +};`, + ], + [ + "rejects non-exhaustive collection iteration", + `import { useEffect } from "react"; +export const Component = ({ tabs }) => { + useEffect(() => { + const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {})); + return () => unsubscribers.some((unsubscribe) => unsubscribe()); + }, [tabs]); + return null; +};`, + ], + [ + "rejects a disposer collection that drops entries", + `import { useEffect } from "react"; +export const Component = ({ tabs }) => { + useEffect(() => { + const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {})); + unsubscribers.pop(); + return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); + }, [tabs]); + return null; +};`, + ], + [ + "rejects a disposer collection mutated through an effect-local helper", + `import { useEffect } from "react"; +export const Component = ({ tabs }) => { + useEffect(() => { + const unsubscribers = tabs.map((tab) => tab.addListener("tabPress", () => {})); + const dropLast = () => unsubscribers.pop(); + dropLast(); + return () => unsubscribers.forEach((unsubscribe) => unsubscribe()); + }, [tabs]); + return null; +};`, + ], + [ + "rejects a timer helper that can overwrite a live handle", + `import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + let timer = null; + const arm = () => { + timer = setTimeout(() => {}, 30000); + }; + arm(); + arm(); + return () => clearTimeout(timer); + }, []); + return null; +};`, + ], + [ + "rejects a timer helper invoked by a listener that is not removed", + `import { useEffect } from "react"; +import { AppState } from "react-native"; +export const Component = () => { + useEffect(() => { + let timer = null; + const arm = () => { + if (timer != null) return; + timer = setTimeout(() => {}, 30000); + }; + AppState.addEventListener("change", arm); + return () => clearTimeout(timer); + }, []); + return null; +};`, + ], + [ + "rejects a timer helper whose reference escapes", + `import { useEffect } from "react"; +export const Component = ({ register }) => { + useEffect(() => { + let timer = null; + const arm = () => { + if (timer != null) return; + timer = setTimeout(() => {}, 30000); + }; + register(arm); + arm(); + return () => clearTimeout(timer); + }, [register]); + return null; +};`, + ], + [ + "keeps reporting a timer owned by a sibling effect", + `import { useEffect, useLayoutEffect, useRef } from "react"; +export const Component = ({ videoId }) => { + const timerRef = useRef(null); + useLayoutEffect(() => { + timerRef.current = setTimeout(() => {}, 4000); + }, [videoId]); + useEffect(() => () => clearTimeout(timerRef.current), []); + return null; +};`, + ], + [ + "does not treat an imported DOM wrapper as a callable disposer", + `import { document as importedDocument } from "global-jsdom"; +import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const dispose = importedDocument.addEventListener("change", () => {}); + return () => dispose(); + }, []); + return null; +};`, + ], + [ + "does not treat a Node EventEmitter instance as a callable disposer", + `import { EventEmitter } from "node:events"; +import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const emitter = new EventEmitter(); + const dispose = emitter.addListener("change", () => {}); + return () => dispose(); + }, []); + return null; +};`, + ], + [ + "clears a different listener timer ref", + `import { useEffect, useRef } from "react"; +export const Component = () => { + const timerRef = useRef(null); + const previousTimerRef = useRef(null); + useEffect(() => { + const handleResize = () => { + if (timerRef.current) clearTimeout(timerRef.current); + timerRef.current = setTimeout(() => {}, 30000); + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + clearTimeout(previousTimerRef.current); + }; + }, []); + return null; +};`, + ], + [ + "drops a listener timer before collection cleanup", + `import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + const timers = []; + const handleResize = () => { + timers.push(setTimeout(() => {}, 30000)); + timers.pop(); + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + timers.forEach(clearTimeout); + }; + }, []); + return null; +};`, + ], + [ + "leaves the listener that owns a retained timer active", + `import { useEffect, useRef } from "react"; +export const Component = () => { + const timerRef = useRef(null); + useEffect(() => { + const handleResize = () => { + if (timerRef.current) clearTimeout(timerRef.current); + timerRef.current = setTimeout(() => {}, 30000); + }; + window.addEventListener("resize", handleResize); + return () => clearTimeout(timerRef.current); + }, []); + return null; +};`, + ], + [ + "allows a listener timer ref to overwrite a live handle", + `import { useEffect, useRef } from "react"; +export const Component = () => { + const timerRef = useRef(null); + useEffect(() => { + const handleResize = () => { + timerRef.current = setTimeout(() => {}, 30000); + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + clearTimeout(timerRef.current); + }; + }, []); + return null; +};`, + ], + [ + "conditionally calls a listener timer cleanup helper", + `import { useEffect, useRef } from "react"; +export const Component = ({ shouldRelease }) => { + const timerRef = useRef(null); + useEffect(() => { + const clearTimer = () => { + if (shouldRelease) clearTimeout(timerRef.current); + }; + const handleResize = () => { + clearTimer(); + timerRef.current = setTimeout(() => {}, 30000); + }; + window.addEventListener("resize", handleResize); + return () => { + window.removeEventListener("resize", handleResize); + clearTimer(); + }; + }, [shouldRelease]); + return null; +};`, + ], + [ + "starts more than one recursive timer chain", + `import { useEffect } from "react"; +export const Component = () => { + useEffect(() => { + let timer = null; + const schedule = () => { + timer = setTimeout(schedule, 30000); + timer = setTimeout(schedule, 30000); + }; + schedule(); + return () => clearTimeout(timer); + }, []); + return null; +};`, + ], +]; + +const timerAllocationVariants: ReadonlyArray = [ + [ + "early-return guard", + `if (timer != null) return; + timer = setTimeout(() => {}, 30000);`, + true, + ], + [ + "release-before-replace guard", + `if (timer != null) clearTimeout(timer); + timer = setTimeout(() => {}, 30000);`, + true, + ], + [ + "unconditional release before replace", + `clearTimeout(timer); + timer = setTimeout(() => {}, 30000);`, + true, + ], + [ + "conditional release before replace", + `if (condition) clearTimeout(timer); + timer = setTimeout(() => {}, 30000);`, + false, + ], + ["unprotected replacement", `timer = setTimeout(() => {}, 30000);`, false], +]; + +const timerInvocationVariants: ReadonlyArray = [ + ["direct calls", `arm(); arm();`, `clearTimeout(timer);`, true], + [ + "owned listener calls", + `const unsubscribe = NetInfo.addEventListener(arm);`, + `clearTimeout(timer); unsubscribe();`, + true, + ], + ["deferred promise call", `Promise.resolve().then(arm);`, `clearTimeout(timer);`, false], +]; + +const generatedTimerCases = timerAllocationVariants.flatMap( + ([allocationName, allocationBody, doesProtectLiveHandle]) => + timerInvocationVariants.map( + ([invocationName, invocationBody, cleanupBody, doesEffectOwnInvocation]): readonly [ + string, + string, + boolean, + ] => [ + `${allocationName} with ${invocationName}`, + `import NetInfo from "@react-native-community/netinfo"; +import { useEffect } from "react"; +export const Component = ({ condition }) => { + useEffect(() => { + let timer = null; + const arm = () => { + ${allocationBody} + }; + ${invocationBody} + return () => { ${cleanupBody} }; + }, []); + return null; +};`, + doesProtectLiveHandle && doesEffectOwnInvocation, + ], + ), +); + +const collectionStorageVariants: ReadonlyArray = [ + [ + "direct collection", + `const ownedUnsubscribers = sources.map((source) => source.addListener("change", () => {}));`, + true, + ], + [ + "stable alias", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers;`, + true, + ], + [ + "full slice copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice();`, + true, + ], + [ + "full spread copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = [...unsubscribers];`, + true, + ], + [ + "Array.from copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = Array.from(unsubscribers);`, + true, + ], + [ + "empty concat copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.concat();`, + true, + ], + [ + "reversed copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.toReversed();`, + true, + ], + [ + "sorted copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.toSorted();`, + true, + ], + [ + "source mutation after snapshot", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + unsubscribers.length = 0;`, + true, + ], + [ + "source mutation through helper after snapshot", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + const clearSource = () => { unsubscribers.length = 0; }; + clearSource();`, + true, + ], + [ + "source mutation through iterator after snapshot", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + unsubscribers.forEach(() => unsubscribers.pop());`, + true, + ], + [ + "source escape after snapshot", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + register(unsubscribers);`, + true, + ], + [ + "partial slice copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(1);`, + false, + ], + [ + "source mutation before snapshot", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + unsubscribers.length = 0; + const ownedUnsubscribers = unsubscribers.slice();`, + false, + ], + [ + "snapshot mutation after copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + ownedUnsubscribers.length = 0;`, + false, + ], + [ + "snapshot mutation through helper after copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + const clearSnapshot = () => { ownedUnsubscribers.length = 0; }; + clearSnapshot();`, + false, + ], + [ + "snapshot mutation through iterator after copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + ownedUnsubscribers.forEach(() => ownedUnsubscribers.pop());`, + false, + ], + [ + "uninvoked snapshot mutation helper", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.slice(); + const clearSnapshot = () => { ownedUnsubscribers.length = 0; }; + void clearSnapshot;`, + true, + ], + [ + "source escape before snapshot", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + register(unsubscribers); + const ownedUnsubscribers = unsubscribers.slice();`, + false, + ], + [ + "filtered copy", + `const unsubscribers = sources.map((source) => source.addListener("change", () => {})); + const ownedUnsubscribers = unsubscribers.filter(Boolean);`, + false, + ], + [ + "escaped collection", + `const ownedUnsubscribers = sources.map((source) => source.addListener("change", () => {})); + register(ownedUnsubscribers);`, + false, + ], +]; + +const collectionCleanupVariants: ReadonlyArray = [ + ["forEach", `ownedUnsubscribers.forEach((unsubscribe) => unsubscribe());`, true], + [ + "for-of with continue after release", + `for (const unsubscribe of ownedUnsubscribers) { + unsubscribe(); + if (condition) continue; + }`, + true, + ], + [ + "for-of with continue before release", + `for (const unsubscribe of ownedUnsubscribers) { + if (condition) continue; + unsubscribe(); + }`, + false, + ], + [ + "for-of with break after release", + `for (const unsubscribe of ownedUnsubscribers) { + unsubscribe(); + if (condition) break; + }`, + false, + ], + [ + "for-of with nested break before release", + `for (const unsubscribe of ownedUnsubscribers) { + for (const item of []) { + if (item) break; + } + unsubscribe(); + }`, + true, + ], + [ + "for-of with nested continue before release", + `for (const unsubscribe of ownedUnsubscribers) { + for (const item of []) { + if (item) continue; + } + unsubscribe(); + }`, + true, + ], + [ + "for-of with caught throw before release", + `for (const unsubscribe of ownedUnsubscribers) { + try { + if (condition) throw new Error("retry"); + } catch {} + unsubscribe(); + }`, + true, + ], +]; + +const generatedCollectionCases = collectionStorageVariants.flatMap( + ([storageName, storageBody, doesStorageRetainEveryEntry]) => + collectionCleanupVariants.map( + ([cleanupName, cleanupBody, doesCleanupVisitEveryEntry]): readonly [ + string, + string, + boolean, + ] => [ + `${storageName} with ${cleanupName}`, + `import { useEffect } from "react"; +export const Component = ({ sources, condition, register }) => { + useEffect(() => { + ${storageBody} + return () => { + ${cleanupBody} + }; + }, [sources, condition, register]); + return null; +};`, + doesStorageRetainEveryEntry && doesCleanupVisitEveryEntry, + ], + ), +); + +describe("effect-needs-cleanup issue #1558", () => { + it.each(validCases)("accepts %s", (_name, source) => { + const result = runRule(effectNeedsCleanup, source); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics).toHaveLength(0); + }); + + it.each(invalidCases)("reports when cleanup %s", (_name, source) => { + const result = runRule(effectNeedsCleanup, source); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length).toBeGreaterThan(0); + }); + + it.each(generatedTimerCases)( + "proves generated timer ownership for %s", + (_name, source, isSafe) => { + const result = runRule(effectNeedsCleanup, source); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length > 0).toBe(!isSafe); + }, + ); + + it.each(generatedCollectionCases)( + "proves generated collection ownership for %s", + (_name, source, isSafe) => { + const result = runRule(effectNeedsCleanup, source); + expect(result.parseErrors).toEqual([]); + expect(result.diagnostics.length > 0).toBe(!isSafe); + }, + ); +}); diff --git a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts index 9207a9caa2..bda62c3fc9 100644 --- a/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts +++ b/packages/oxlint-plugin-react-doctor/src/plugin/rules/state-and-effects/effect-needs-cleanup.ts @@ -33,8 +33,10 @@ import { getFinalSequenceExpressionValue } from "../../utils/get-final-sequence- import { doNodesCoverEveryPathAfterNode } from "../../utils/do-nodes-cover-every-path-after-node.js"; import { doNodesCoverEveryPathFromFunctionEntry } from "../../utils/do-nodes-cover-every-path-from-function-entry.js"; import { getFunctionBindingIdentifier } from "../../utils/get-function-binding-name.js"; +import { getImportDeclarationForSymbol } from "../../utils/get-import-declaration-for-symbol.js"; import { getRangeStart } from "../../utils/get-range-start.js"; import { getStaticPropertyKeyName } from "../../utils/get-static-property-key-name.js"; +import { getSymbolTypeAnnotation } from "../../utils/get-symbol-type-annotation.js"; import { isEventHandlerAttribute } from "../../utils/is-event-handler-attribute.js"; import { isEarlyExitStatement } from "../../utils/is-early-exit-statement.js"; import { isAstNode } from "../../utils/is-ast-node.js"; @@ -44,6 +46,7 @@ import { isReactHookName } from "../../utils/is-react-hook-name.js"; import { isReactHookCall } from "../../utils/is-react-hook-call.js"; import { isReactApiCall } from "../../utils/is-react-api-call.js"; import { readStaticBoolean } from "../../utils/read-static-boolean.js"; +import { resolveExactLocalFunction } from "../../utils/resolve-exact-local-function.js"; import { resolveReactRefCurrentOriginSymbol, resolveReactRefSymbol, @@ -74,6 +77,14 @@ import type { EsTreeNodeOfType } from "../../utils/es-tree-node-of-type.js"; import type { SymbolDescriptor } from "../../semantic/scope-analysis.js"; const CLEANUP_EFFECT_HOOK_NAMES = new Set([...EFFECT_HOOK_NAMES, "useInsertionEffect"]); +const CALLABLE_ADD_EVENT_LISTENER_MODULE_NAMES: ReadonlySet = new Set([ + "@react-native-community/netinfo", +]); +const NON_CALLABLE_ADD_LISTENER_CONSTRUCTOR_MODULE_NAMES: ReadonlySet = new Set([ + "events", + "node:events", + "react-native", +]); const REPLAYABLE_ITERATOR_COLLECTION_CACHE = new WeakMap>(); const REPLAY_ENTRY_DROPPING_ARRAY_METHOD_NAMES: ReadonlySet = new Set([ "pop", @@ -581,6 +592,170 @@ const doesResourceKeyMatchUsageHandle = ( resourceKey !== null && (resourceKey === usage.handleKey || resourceKey === findAssignedResourceKey(usage.node, context)); +const getImportedReceiverSource = (expression: EsTreeNode, context: RuleContext): string | null => { + const unwrappedExpression = stripParenExpression(expression); + if (isNodeOfType(unwrappedExpression, "MemberExpression")) { + return getImportedReceiverSource(unwrappedExpression.object, context); + } + if (!isNodeOfType(unwrappedExpression, "Identifier")) return null; + const symbol = context.scopes.symbolFor(unwrappedExpression); + const importDeclaration = symbol ? getImportDeclarationForSymbol(symbol) : null; + return typeof importDeclaration?.source.value === "string" + ? importDeclaration.source.value + : null; +}; + +const canListenerRegistrationReturnCallableDisposer = ( + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + if (usage.kind !== "subscribe" || !isNodeOfType(usage.node, "CallExpression")) return false; + if (isCleanupReturningSubscribeLikeCallExpression(usage.node)) return true; + if ( + usage.registrationVerbName !== "addEventListener" && + usage.registrationVerbName !== "addListener" + ) { + return false; + } + if (isProvenLegacyMediaQueryListMethodCall(usage.node, "addListener", context)) return false; + const callee = stripParenExpression(usage.node.callee); + if (!isNodeOfType(callee, "MemberExpression")) return false; + const receiverSource = getImportedReceiverSource(callee.object, context); + if (receiverSource !== null) { + return usage.registrationVerbName === "addEventListener" + ? CALLABLE_ADD_EVENT_LISTENER_MODULE_NAMES.has(receiverSource) + : receiverSource !== "react-native"; + } + const receiver = stripParenExpression(callee.object); + const stableReceiver = resolveStableValue(receiver, context); + const constructedReceiverSource = + stableReceiver && isNodeOfType(stableReceiver, "NewExpression") + ? getImportedReceiverSource(stableReceiver.callee, context) + : null; + if ( + usage.registrationVerbName === "addListener" && + constructedReceiverSource !== null && + NON_CALLABLE_ADD_LISTENER_CONSTRUCTOR_MODULE_NAMES.has(constructedReceiverSource) + ) { + return false; + } + const receiverSymbol = isNodeOfType(receiver, "Identifier") + ? context.scopes.symbolFor(receiver) + : null; + const isKnownGlobalDomReceiver = + isNodeOfType(receiver, "Identifier") && + (receiver.name === "window" || receiver.name === "document") && + context.scopes.isGlobalReference(receiver); + const hasProvableReceiverOrigin = + !isNodeOfType(receiver, "Identifier") || + isKnownGlobalDomReceiver || + (receiverSymbol !== null && + (getSymbolTypeAnnotation(receiverSymbol) !== null || + getDirectUnreassignedInitializer(receiverSymbol) !== null)); + if (!hasProvableReceiverOrigin) return false; + return getProvenDomEventTargetPrototypeOwnerNames(receiver, context.scopes).length === 0; +}; + +const isKnownNetInfoReceiver = (expression: EsTreeNode, context: RuleContext): boolean => { + const receiver = stripParenExpression(expression); + const importedReceiver = resolveImportedApiReference(receiver, context.scopes); + return ( + (isNodeOfType(receiver, "Identifier") && + receiver.name === "NetInfo" && + context.scopes.symbolFor(receiver) !== null) || + importedReceiver?.source === "@react-native-community/netinfo" + ); +}; + +const isKnownReactNavigationReceiver = ( + expression: EsTreeNode, + context: RuleContext, + visitedSymbolIds: Set = new Set(), +): boolean => { + const receiver = stripParenExpression(expression); + if (isNodeOfType(receiver, "Identifier")) { + const receiverSymbol = context.scopes.symbolFor(receiver); + if (!receiverSymbol || visitedSymbolIds.has(receiverSymbol.id)) return false; + if (receiver.name === "navigation") return true; + visitedSymbolIds.add(receiverSymbol.id); + const receiverInitializer = receiverSymbol.initializer + ? stripParenExpression(receiverSymbol.initializer) + : null; + const navigationHook = isNodeOfType(receiverInitializer, "CallExpression") + ? resolveImportedApiReference(receiverInitializer.callee, context.scopes) + : null; + if ( + navigationHook?.importedName === "useNavigation" && + navigationHook.source.startsWith("@react-navigation/") + ) { + return true; + } + return Boolean( + receiverInitializer && + isKnownReactNavigationReceiver(receiverInitializer, context, visitedSymbolIds), + ); + } + if (!isNodeOfType(receiver, "CallExpression")) return false; + const receiverCallee = stripParenExpression(receiver.callee); + return Boolean( + isNodeOfType(receiverCallee, "MemberExpression") && + !receiverCallee.computed && + isNodeOfType(receiverCallee.property, "Identifier") && + receiverCallee.property.name === "getParent" && + isKnownReactNavigationReceiver(receiverCallee.object, context, visitedSymbolIds), + ); +}; + +const isKnownCallableSubscriptionResult = ( + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + if (canListenerRegistrationReturnCallableDisposer(usage, context)) return true; + if (!isNodeOfType(usage.node, "CallExpression")) return false; + const callee = stripParenExpression(usage.node.callee); + if ( + !isNodeOfType(callee, "MemberExpression") || + callee.computed || + !isNodeOfType(callee.property, "Identifier") + ) { + return false; + } + if (callee.property.name === "addEventListener") { + return usage.node.arguments.length === 1 && isKnownNetInfoReceiver(callee.object, context); + } + return ( + callee.property.name === "addListener" && + usage.node.arguments.length >= 2 && + isKnownReactNavigationReceiver(callee.object, context) + ); +}; + +const doesStableIdentifierMatchUsageHandle = ( + expression: EsTreeNode, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + const identifier = stripParenExpression(expression); + if (!isNodeOfType(identifier, "Identifier") || usage.handleKey === null) return false; + const symbol = context.scopes.symbolFor(identifier); + return Boolean( + symbol && + resolveExpressionKey(identifier, context) === usage.handleKey && + !symbol.references.some((reference) => isWithinAssignmentTarget(reference.identifier)), + ); +}; + +const doesStableIdentifierCallUsageDisposer = ( + expression: EsTreeNode, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + return ( + doesStableIdentifierMatchUsageHandle(expression, usage, context) && + isKnownCallableSubscriptionResult(usage, context) + ); +}; + const doesSocketOwnerReleaseListenerUsage = ( releaseReceiverKey: string | null, releaseVerbName: string, @@ -748,78 +923,13 @@ const getCallRegistrationDetails = ( }; }; -const isKnownNetInfoReceiver = (expression: EsTreeNode, context: RuleContext): boolean => { - const receiver = stripParenExpression(expression); - const importedReceiver = resolveImportedApiReference(receiver, context.scopes); - return ( - (isNodeOfType(receiver, "Identifier") && - receiver.name === "NetInfo" && - context.scopes.symbolFor(receiver) !== null) || - importedReceiver?.source === "@react-native-community/netinfo" - ); -}; - -const isKnownReactNavigationReceiver = ( - expression: EsTreeNode, - context: RuleContext, - visitedSymbolIds: Set = new Set(), -): boolean => { - const receiver = stripParenExpression(expression); - if (isNodeOfType(receiver, "Identifier")) { - const receiverSymbol = context.scopes.symbolFor(receiver); - if (!receiverSymbol || visitedSymbolIds.has(receiverSymbol.id)) return false; - if (receiver.name === "navigation") return true; - visitedSymbolIds.add(receiverSymbol.id); - const receiverInitializer = receiverSymbol.initializer - ? stripParenExpression(receiverSymbol.initializer) - : null; - const navigationHook = isNodeOfType(receiverInitializer, "CallExpression") - ? resolveImportedApiReference(receiverInitializer.callee, context.scopes) - : null; - if ( - navigationHook?.importedName === "useNavigation" && - navigationHook.source.startsWith("@react-navigation/") - ) { - return true; - } - return Boolean( - receiverInitializer && - isKnownReactNavigationReceiver(receiverInitializer, context, visitedSymbolIds), - ); - } - if (!isNodeOfType(receiver, "CallExpression")) return false; - const receiverCallee = stripParenExpression(receiver.callee); - return Boolean( - isNodeOfType(receiverCallee, "MemberExpression") && - !receiverCallee.computed && - isNodeOfType(receiverCallee.property, "Identifier") && - receiverCallee.property.name === "getParent" && - isKnownReactNavigationReceiver(receiverCallee.object, context, visitedSymbolIds), - ); -}; - -const isKnownCallableSubscriptionResult = ( - usage: SubscribeLikeUsage, - context: RuleContext, -): boolean => { - if (!isNodeOfType(usage.node, "CallExpression")) return false; - if (isCleanupReturningSubscribeLikeCallExpression(usage.node)) return true; - const callee = stripParenExpression(usage.node.callee); - if ( - !isNodeOfType(callee, "MemberExpression") || - callee.computed || - !isNodeOfType(callee.property, "Identifier") - ) { - return false; - } - if (callee.property.name === "addEventListener") { - return usage.node.arguments.length === 1 && isKnownNetInfoReceiver(callee.object, context); - } - return ( - callee.property.name === "addListener" && - usage.node.arguments.length >= 2 && - isKnownReactNavigationReceiver(callee.object, context) - ); +const getSubscribeUsageCallbackArgument = (usage: SubscribeLikeUsage): EsTreeNode | null => { + if (usage.kind !== "subscribe" || !isNodeOfType(usage.node, "CallExpression")) return null; + const callbackArgument = + usage.node.arguments?.length === UNARY_LISTENER_ARGUMENT_COUNT + ? usage.node.arguments[UNARY_LISTENER_HANDLER_ARGUMENT_INDEX] + : usage.node.arguments?.[EVENT_LISTENER_HANDLER_ARGUMENT_INDEX]; + return callbackArgument && isAstNode(callbackArgument) ? callbackArgument : null; }; const resolveChannelClientKey = ( @@ -872,6 +982,55 @@ const findFluentChannelSubscriptionHandleKey = ( return findAssignedResourceKey(terminalCall, context, true); }; +const collectEffectOwnedResourceCallbackFunctions = ( + callback: EsTreeNode, + context: RuleContext, +): Set => { + const ownedFunctions = collectEffectInvokedFunctions(callback, context.scopes); + const pendingFunctions = [...ownedFunctions]; + while (pendingFunctions.length > 0) { + const ownerFunction = pendingFunctions.pop(); + if (!ownerFunction || !isFunctionLike(ownerFunction)) continue; + walkAst(ownerFunction.body, (child: EsTreeNode) => { + if (child !== ownerFunction.body && isFunctionLike(child)) return false; + if (!isNodeOfType(child, "CallExpression")) return; + let callbackArgument: EsTreeNode | null = null; + if ( + isNodeOfType(child.callee, "Identifier") && + TIMER_CALLEE_NAMES_REQUIRING_CLEANUP.has(child.callee.name) + ) { + const timerCallback = child.arguments?.[0]; + callbackArgument = timerCallback && isAstNode(timerCallback) ? timerCallback : null; + } else { + const promiseCallback = child.arguments?.find( + (argument) => isAstNode(argument) && getPromiseChainCallForCallback(argument) === child, + ); + if (promiseCallback && isAstNode(promiseCallback)) { + callbackArgument = promiseCallback; + } + const registrationVerbName = getSubscribeOrObserveMethodName(child); + if (registrationVerbName !== null) { + const registrationDetails = getCallRegistrationDetails(child, context); + callbackArgument = getSubscribeUsageCallbackArgument({ + kind: "subscribe", + node: child, + resourceName: registrationVerbName, + handleKey: null, + ...registrationDetails, + }); + } + } + const callbackFunction = callbackArgument + ? resolveExactLocalFunction(callbackArgument, context.scopes) + : null; + if (!callbackFunction || ownedFunctions.has(callbackFunction)) return; + ownedFunctions.add(callbackFunction); + pendingFunctions.push(callbackFunction); + }); + } + return ownedFunctions; +}; + const findSubscribeLikeUsages = ( callback: EsTreeNode, context: RuleContext, @@ -891,7 +1050,7 @@ const findSubscribeLikeUsages = ( cleanupArgument = lastCallbackStatement.argument; } } - const effectInvokedFunctions = collectEffectInvokedFunctions(callback); + const effectInvokedFunctions = collectEffectOwnedResourceCallbackFunctions(callback, context); walkAst(callback, (child: EsTreeNode) => { if (child !== callback && isFunctionLike(child)) { @@ -1231,70 +1390,255 @@ const resolveIteratorCollectionKey = ( return null; }; -const resolveReceiverIteratorCollectionKey = ( +const resolveCleanupIteratorCollectionKey = ( expression: EsTreeNode | null | undefined, context: RuleContext, ): string | null => { - if (!expression) return null; - const unwrappedExpression = stripParenExpression(expression); - if (!isNodeOfType(unwrappedExpression, "Identifier")) return null; - const forOfStatement = findForOfStatementForIteratorExpression(unwrappedExpression, context); - const collectionExpression = forOfStatement?.right; - if (!collectionExpression) return null; - const collectionIdentifier = stripParenExpression(collectionExpression); - if ( - !isNodeOfType(collectionIdentifier, "Identifier") || - !isPrivatePlainConstIdentifier(collectionIdentifier, context) - ) { - return null; - } - const collectionSymbol = context.scopes.symbolFor(collectionIdentifier); - const initializer = collectionSymbol?.initializer - ? stripParenExpression(collectionSymbol.initializer) - : null; - return collectionSymbol && - isNodeOfType(initializer, "ArrayExpression") && - hasOnlyReplayableCollectionReferences(collectionIdentifier, context, new Set()) - ? `symbol:${collectionSymbol.id}` - : null; + const forOfStatement = findForOfStatementForIteratorExpression(expression, context); + return forOfStatement + ? resolveExpressionKey(forOfStatement.right, context) + : resolveIteratorCollectionKey(expression, context); }; -const isStableLoopReceiver = ( - expression: EsTreeNode | null | undefined, - context: RuleContext, -): boolean => { - if (!expression) return false; - const unwrappedExpression = stripParenExpression(expression); - return ( - isNodeOfType(unwrappedExpression, "Identifier") && - unwrappedExpression.name === "document" && - context.scopes.isGlobalReference(unwrappedExpression) - ); +const setCollectionMutationLimit = ( + mutationLimits: Map, + collectionKey: string | null, + maximumRelevantStart: number, +): void => { + if (collectionKey === null) return; + const existingMutationLimit = mutationLimits.get(collectionKey); + if (existingMutationLimit === undefined || existingMutationLimit < maximumRelevantStart) { + mutationLimits.set(collectionKey, maximumRelevantStart); + } }; -const resolveStableLoopHandlerSymbolId = ( +const resolveExhaustiveCollectionReplayMutationLimits = ( expression: EsTreeNode | null | undefined, context: RuleContext, -): number | null => { - if (!expression) return null; + maximumRelevantStart: number = Number.POSITIVE_INFINITY, + visitedSymbolIds: Set = new Set(), +): ReadonlyMap => { + const mutationLimits = new Map(); + if (!expression) return mutationLimits; const unwrappedExpression = stripParenExpression(expression); - if (!isNodeOfType(unwrappedExpression, "Identifier")) return null; + const expressionKey = resolveExpressionKey(unwrappedExpression, context); + setCollectionMutationLimit(mutationLimits, expressionKey, maximumRelevantStart); + if (!isNodeOfType(unwrappedExpression, "Identifier")) { + return mutationLimits; + } const symbol = context.scopes.symbolFor(unwrappedExpression); if ( !symbol || - (symbol.kind !== "const" && symbol.kind !== "function" && symbol.kind !== "parameter") || - !symbol.references.every( - (reference) => reference.flag === "read" && !isWithinAssignmentTarget(reference.identifier), - ) + visitedSymbolIds.has(symbol.id) || + !isPrivatePlainConstIdentifier(unwrappedExpression, context) ) { - return null; + return mutationLimits; } - return symbol.id; -}; - + const nextVisitedSymbolIds = new Set(visitedSymbolIds); + nextVisitedSymbolIds.add(symbol.id); + const initializer = symbol.initializer ? stripParenExpression(symbol.initializer) : null; + let copiedCollection: EsTreeNode | null = null; + let copiedCollectionMaximumRelevantStart = maximumRelevantStart; + if (isNodeOfType(initializer, "Identifier")) { + copiedCollection = initializer; + } else if (isNodeOfType(initializer, "ArrayExpression")) { + const elements = initializer.elements ?? []; + const onlyElement = elements[0]; + if (elements.length === 1 && onlyElement && isNodeOfType(onlyElement, "SpreadElement")) { + copiedCollection = onlyElement.argument; + copiedCollectionMaximumRelevantStart = Math.min( + maximumRelevantStart, + getRangeStart(initializer) ?? Number.POSITIVE_INFINITY, + ); + } + } else if (isNodeOfType(initializer, "CallExpression")) { + const copyCallee = stripParenExpression(initializer.callee); + if ( + isNodeOfType(copyCallee, "MemberExpression") && + !copyCallee.computed && + isNodeOfType(copyCallee.property, "Identifier") + ) { + const copyMethodName = copyCallee.property.name; + const isArrayFrom = + isNodeOfType(copyCallee.object, "Identifier") && + copyCallee.object.name === "Array" && + context.scopes.isGlobalReference(copyCallee.object) && + copyMethodName === "from" && + initializer.arguments.length === 1; + const isReceiverCopy = + ((copyMethodName === "slice" || copyMethodName === "concat") && + initializer.arguments.length === 0) || + copyMethodName === "toReversed" || + copyMethodName === "toSorted"; + if (isArrayFrom) { + const sourceArgument = initializer.arguments[0]; + copiedCollection = isAstNode(sourceArgument) ? sourceArgument : null; + } else if (isReceiverCopy) { + copiedCollection = copyCallee.object; + } + if (copiedCollection) { + copiedCollectionMaximumRelevantStart = Math.min( + maximumRelevantStart, + getRangeStart(initializer) ?? Number.POSITIVE_INFINITY, + ); + } + } + } + if (!copiedCollection) return mutationLimits; + for (const [replayKey, mutationLimit] of resolveExhaustiveCollectionReplayMutationLimits( + copiedCollection, + context, + copiedCollectionMaximumRelevantStart, + nextVisitedSymbolIds, + )) { + setCollectionMutationLimit(mutationLimits, replayKey, mutationLimit); + } + return mutationLimits; +}; + +const resolveCleanupIteratorCollectionMutationLimits = ( + expression: EsTreeNode | null | undefined, + context: RuleContext, +): ReadonlyMap => { + const forOfStatement = findForOfStatementForIteratorExpression(expression, context); + if (forOfStatement) { + return resolveExhaustiveCollectionReplayMutationLimits(forOfStatement.right, context); + } + const unwrappedExpression = expression ? stripParenExpression(expression) : null; + if (!isNodeOfType(unwrappedExpression, "Identifier")) return new Map(); + const symbol = context.scopes.symbolFor(unwrappedExpression); + if (!symbol || symbol.kind !== "parameter") return new Map(); + let callbackNode: EsTreeNode | null | undefined = symbol.bindingIdentifier.parent; + while (callbackNode && !isFunctionLike(callbackNode)) callbackNode = callbackNode.parent; + const callNode = callbackNode?.parent; + const callee = isNodeOfType(callNode, "CallExpression") + ? stripParenExpression(callNode.callee) + : null; + return isNodeOfType(callee, "MemberExpression") + ? resolveExhaustiveCollectionReplayMutationLimits(callee.object, context) + : new Map(); +}; + +const doesCleanupIteratorMatchUsageCollection = ( + expression: EsTreeNode | null | undefined, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + const usageCollectionKey = findContainingCollectionKey(usage.node, context); + return ( + usageCollectionKey !== null && + resolveCleanupIteratorCollectionMutationLimits(expression, context).has(usageCollectionKey) + ); +}; + +const resolveReceiverIteratorCollectionKey = ( + expression: EsTreeNode | null | undefined, + context: RuleContext, +): string | null => { + if (!expression) return null; + const unwrappedExpression = stripParenExpression(expression); + if (!isNodeOfType(unwrappedExpression, "Identifier")) return null; + const forOfStatement = findForOfStatementForIteratorExpression(unwrappedExpression, context); + const collectionExpression = forOfStatement?.right; + if (!collectionExpression) return null; + const collectionIdentifier = stripParenExpression(collectionExpression); + if ( + !isNodeOfType(collectionIdentifier, "Identifier") || + !isPrivatePlainConstIdentifier(collectionIdentifier, context) + ) { + return null; + } + const collectionSymbol = context.scopes.symbolFor(collectionIdentifier); + const initializer = collectionSymbol?.initializer + ? stripParenExpression(collectionSymbol.initializer) + : null; + return collectionSymbol && + isNodeOfType(initializer, "ArrayExpression") && + hasOnlyReplayableCollectionReferences(collectionIdentifier, context, new Set()) + ? `symbol:${collectionSymbol.id}` + : null; +}; + +const isStableLoopReceiver = ( + expression: EsTreeNode | null | undefined, + context: RuleContext, +): boolean => { + if (!expression) return false; + const unwrappedExpression = stripParenExpression(expression); + return ( + isNodeOfType(unwrappedExpression, "Identifier") && + unwrappedExpression.name === "document" && + context.scopes.isGlobalReference(unwrappedExpression) + ); +}; + +const resolveStableLoopHandlerSymbolId = ( + expression: EsTreeNode | null | undefined, + context: RuleContext, +): number | null => { + if (!expression) return null; + const unwrappedExpression = stripParenExpression(expression); + if (!isNodeOfType(unwrappedExpression, "Identifier")) return null; + const symbol = context.scopes.symbolFor(unwrappedExpression); + if ( + !symbol || + (symbol.kind !== "const" && symbol.kind !== "function" && symbol.kind !== "parameter") || + !symbol.references.every( + (reference) => reference.flag === "read" && !isWithinAssignmentTarget(reference.identifier), + ) + ) { + return null; + } + return symbol.id; +}; + +const doesLoopJumpExitForOfIteration = ( + jumpStatement: EsTreeNode, + forOfStatement: EsTreeNodeOfType<"ForOfStatement">, +): boolean => { + if ( + !isNodeOfType(jumpStatement, "BreakStatement") && + !isNodeOfType(jumpStatement, "ContinueStatement") + ) { + return false; + } + if (jumpStatement.label) { + let ancestor = jumpStatement.parent; + while (ancestor) { + if ( + isNodeOfType(ancestor, "LabeledStatement") && + ancestor.label.name === jumpStatement.label.name + ) { + return isAstDescendant(forOfStatement, ancestor.body); + } + ancestor = ancestor.parent; + } + return false; + } + let ancestor = jumpStatement.parent; + while (ancestor) { + const isLoop = + isNodeOfType(ancestor, "ForStatement") || + isNodeOfType(ancestor, "ForInStatement") || + isNodeOfType(ancestor, "ForOfStatement") || + isNodeOfType(ancestor, "WhileStatement") || + isNodeOfType(ancestor, "DoWhileStatement"); + if (isLoop) return ancestor === forOfStatement; + if ( + isNodeOfType(jumpStatement, "BreakStatement") && + isNodeOfType(ancestor, "SwitchStatement") + ) { + return false; + } + ancestor = ancestor.parent; + } + return false; +}; + const isDirectExhaustiveForOfRelease = ( releaseNode: EsTreeNode, forOfStatement: EsTreeNodeOfType<"ForOfStatement">, + context: RuleContext, ): boolean => { const releaseRoot = findTransparentExpressionRoot(releaseNode); const releaseStatement = releaseRoot.parent; @@ -1303,21 +1647,46 @@ const isDirectExhaustiveForOfRelease = ( ? releaseStatement.parent === forOfStatement.body : releaseStatement === forOfStatement.body; if (!isDirectLoopBodyStatement) return false; - let hasAbruptLoopExit = false; + const cleanupOwnerFunction = findEnclosingFunction(releaseStatement); + let hasTerminatingLoopExit = false; + let hasContinueBeforeRelease = false; walkAst(forOfStatement.body, (child: EsTreeNode) => { - if (hasAbruptLoopExit) return false; + if (hasTerminatingLoopExit || hasContinueBeforeRelease) return false; if (child !== forOfStatement.body && isFunctionLike(child)) return false; + if (isNodeOfType(child, "ReturnStatement")) { + hasTerminatingLoopExit = true; + return false; + } + if ( + isNodeOfType(child, "ThrowStatement") && + (!cleanupOwnerFunction || + !canNodeReachLaterNodeWithinFunction( + child, + releaseStatement, + cleanupOwnerFunction, + context, + )) + ) { + hasTerminatingLoopExit = true; + return false; + } + if ( + isNodeOfType(child, "BreakStatement") && + doesLoopJumpExitForOfIteration(child, forOfStatement) + ) { + hasTerminatingLoopExit = true; + return false; + } if ( - isNodeOfType(child, "BreakStatement") || - isNodeOfType(child, "ContinueStatement") || - isNodeOfType(child, "ReturnStatement") || - isNodeOfType(child, "ThrowStatement") + isNodeOfType(child, "ContinueStatement") && + doesLoopJumpExitForOfIteration(child, forOfStatement) && + (getRangeStart(child) ?? -1) < (getRangeStart(releaseStatement) ?? 0) ) { - hasAbruptLoopExit = true; + hasContinueBeforeRelease = true; return false; } }); - return !hasAbruptLoopExit; + return !hasTerminatingLoopExit && !hasContinueBeforeRelease; }; const findCollectionMappingCall = (callbackNode: EsTreeNode): EsTreeNode | null => { @@ -1416,10 +1785,44 @@ const findMappedResourceCollectionKey = ( : null; }; +const resolveDirectResourcePushCollectionSymbol = ( + resourceNode: EsTreeNode, + context: RuleContext, +): SymbolDescriptor | null => { + const resourceRoot = findTransparentExpressionRoot(resourceNode); + const pushCall = resourceRoot.parent; + const pushCallee = isNodeOfType(pushCall, "CallExpression") + ? stripParenExpression(pushCall.callee) + : null; + if ( + !isNodeOfType(pushCall, "CallExpression") || + !pushCall.arguments.some((argument) => argument === resourceRoot) || + !isNodeOfType(pushCallee, "MemberExpression") || + pushCallee.computed || + !isNodeOfType(pushCallee.object, "Identifier") || + !isNodeOfType(pushCallee.property, "Identifier") || + pushCallee.property.name !== "push" || + !isPrivatePlainConstIdentifier(pushCallee.object, context) + ) { + return null; + } + const collectionSymbol = context.scopes.symbolFor(pushCallee.object); + const initializer = collectionSymbol?.initializer + ? stripParenExpression(collectionSymbol.initializer) + : null; + return collectionSymbol && + isNodeOfType(initializer, "ArrayExpression") && + (initializer.elements?.length ?? 0) === 0 + ? collectionSymbol + : null; +}; + const findContainingCollectionKey = ( resourceNode: EsTreeNode, context: RuleContext, ): string | null => { + const pushedCollectionSymbol = resolveDirectResourcePushCollectionSymbol(resourceNode, context); + if (pushedCollectionSymbol) return `symbol:${pushedCollectionSymbol.id}`; const mappedCollectionKey = findMappedResourceCollectionKey(resourceNode, context); if (mappedCollectionKey !== null) return mappedCollectionKey; let currentNode = resourceNode; @@ -1807,16 +2210,17 @@ const findReconnectHelperInvocation = ( }; const resolveCleanupPathAnchor = ( - usage: SubscribeLikeUsage, + usageNode: EsTreeNode, effectCallback: EsTreeNode, context: RuleContext, + usage?: SubscribeLikeUsage, ): EsTreeNode => { - const usageFunction = findEnclosingFunction(usage.node); - if (!usageFunction || usageFunction === effectCallback) return usage.node; + const usageFunction = findEnclosingFunction(usageNode); + if (!usageFunction || usageFunction === effectCallback) return usageNode; return ( findSingleDirectInvocation(usageFunction, effectCallback, context) ?? - findReconnectHelperInvocation(usageFunction, effectCallback, usage, context) ?? - usage.node + (usage ? findReconnectHelperInvocation(usageFunction, effectCallback, usage, context) : null) ?? + usageNode ); }; @@ -1895,13 +2299,55 @@ const resolveCleanupHelperParameterSubstitutions = ( return substitutions; }; +const isDirectExhaustiveTimerCollectionCleanup = ( + cleanupNode: EsTreeNode, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + if (usage.kind !== "timer") return false; + const cleanupCall = isNodeOfType(cleanupNode, "ChainExpression") + ? cleanupNode.expression + : cleanupNode; + const cleanupCallee = isNodeOfType(cleanupCall, "CallExpression") + ? stripParenExpression(cleanupCall.callee) + : null; + const cleanupCallback = isNodeOfType(cleanupCall, "CallExpression") + ? cleanupCall.arguments[0] + : null; + const expectedCleanupName = + usage.registrationVerbName === "setInterval" ? "clearInterval" : "clearTimeout"; + const retainedCollectionKey = findContainingCollectionKey(usage.node, context); + if ( + !isNodeOfType(cleanupCall, "CallExpression") || + !isNodeOfType(cleanupCallee, "MemberExpression") || + cleanupCallee.computed || + !isNodeOfType(cleanupCallee.property, "Identifier") || + cleanupCallee.property.name !== "forEach" || + !isNodeOfType(cleanupCallback, "Identifier") || + cleanupCallback.name !== expectedCleanupName || + !context.scopes.isGlobalReference(cleanupCallback) || + retainedCollectionKey === null || + retainedCollectionKey !== resolveExpressionKey(cleanupCallee.object, context) + ) { + return false; + } + const collectionMutationLimits = resolveExhaustiveCollectionReplayMutationLimits( + cleanupCallee.object, + context, + ); + return ( + collectionMutationLimits.has(retainedCollectionKey) && + !hasCollectionMutationBeforeRelease(usage.node, cleanupCall, collectionMutationLimits, context) + ); +}; + const doesCleanupFunctionReleaseUsage = ( cleanupFunction: EsTreeNode, usage: SubscribeLikeUsage, context: RuleContext, - requiresDirectReleasePathCoverage = false, visitedFunctions: Set = new Set(), parameterSubstitutions: ReadonlyMap = new Map(), + requireExhaustivePaths = false, ): boolean => { if (!isFunctionLike(cleanupFunction) || visitedFunctions.has(cleanupFunction)) return false; visitedFunctions.add(cleanupFunction); @@ -1919,34 +2365,59 @@ const doesCleanupFunctionReleaseUsage = ( const cleanupCall = isNodeOfType(cleanupChild, "ChainExpression") ? cleanupChild.expression : cleanupChild; + if (isDirectExhaustiveTimerCollectionCleanup(cleanupChild, usage, context)) { + if (requireExhaustivePaths) { + matchingLoopOrHelperAnchors.push(cleanupChild); + return; + } + didCleanupFunctionMatch = true; + return false; + } if (doesReleaseCallMatchUsage(cleanupChild, usage, context, parameterSubstitutions)) { const cleanupForEachCall = findEnclosingForEachCall(cleanupChild); const cleanupCallee = isNodeOfType(cleanupCall, "CallExpression") ? stripParenExpression(cleanupCall.callee) : null; - const mappedResourceCollectionKey = findMappedResourceCollectionKey(usage.node, context); - const cleanupIdentifierLoop = isNodeOfType(cleanupCallee, "Identifier") - ? (findForOfStatementForIteratorExpression(cleanupCallee, context) ?? cleanupForEachCall) - : null; - if (mappedResourceCollectionKey !== null && cleanupIdentifierLoop) { - matchingLoopOrHelperAnchors.push(cleanupIdentifierLoop); - return; - } - const cleanupReceiverForOfStatement = isNodeOfType(cleanupCallee, "MemberExpression") - ? findForOfStatementForIteratorExpression(cleanupCallee.object, context) - : null; - const cleanupReceiverCollectionKey = cleanupReceiverForOfStatement - ? resolveExpressionKey(cleanupReceiverForOfStatement.right, context) - : isNodeOfType(cleanupCallee, "MemberExpression") - ? resolveIteratorCollectionKey(cleanupCallee.object, context) - : null; + const cleanupIteratorExpression = isNodeOfType(cleanupCallee, "MemberExpression") + ? cleanupCallee.object + : cleanupCallee; + const cleanupReceiverForOfStatement = findForOfStatementForIteratorExpression( + cleanupIteratorExpression, + context, + ); + const cleanupReceiverCollectionKey = resolveCleanupIteratorCollectionKey( + cleanupIteratorExpression, + context, + ); + const cleanupCollectionMutationLimits = resolveCleanupIteratorCollectionMutationLimits( + cleanupIteratorExpression, + context, + ); + const retainedResourceCollectionKey = + findPushedResourceCollectionKey(usage, context) ?? + findContainingCollectionKey(usage.node, context); if ( cleanupReceiverCollectionKey !== null && findEnclosingFunction(cleanupChild) !== cleanupFunction ) { + const cleanupIteratorFunction = findEnclosingFunction(cleanupChild); if ( cleanupForEachCall && - findPushedResourceCollectionKey(usage, context) === cleanupReceiverCollectionKey + cleanupIteratorFunction && + isFunctionLike(cleanupIteratorFunction) && + retainedResourceCollectionKey !== null && + cleanupCollectionMutationLimits.has(retainedResourceCollectionKey) && + doNodesCoverEveryPathFromFunctionEntry( + cleanupIteratorFunction, + [cleanupChild], + context, + ) && + !hasCollectionMutationBeforeRelease( + usage.node, + cleanupChild, + cleanupCollectionMutationLimits, + context, + ) ) { matchingLoopOrHelperAnchors.push(cleanupForEachCall); } @@ -1959,11 +2430,14 @@ const doesCleanupFunctionReleaseUsage = ( findForOfStatementForIteratorExpression(cleanupEventArgument, context) ?? cleanupReceiverForOfStatement; if (!cleanupForOfStatement) { - if (requiresDirectReleasePathCoverage) { - matchingLoopOrHelperAnchors.push( - findDirectHandleGuardForRelease(cleanupChild, cleanupFunction, usage, context) ?? - cleanupChild, + if (requireExhaustivePaths) { + const handleGuard = findDirectHandleGuardForRelease( + cleanupChild, + cleanupFunction, + usage, + context, ); + matchingLoopOrHelperAnchors.push(handleGuard ?? cleanupChild); return; } didCleanupFunctionMatch = true; @@ -1972,7 +2446,16 @@ const doesCleanupFunctionReleaseUsage = ( if ( !cleanupFunction.async && !cleanupFunction.generator && - isDirectExhaustiveForOfRelease(cleanupChild, cleanupForOfStatement) + isDirectExhaustiveForOfRelease(cleanupChild, cleanupForOfStatement, context) && + (retainedResourceCollectionKey === null || + cleanupCollectionMutationLimits.size === 0 || + (cleanupCollectionMutationLimits.has(retainedResourceCollectionKey) && + !hasCollectionMutationBeforeRelease( + usage.node, + cleanupChild, + cleanupCollectionMutationLimits, + context, + ))) ) { matchingLoopOrHelperAnchors.push(cleanupForOfStatement); } @@ -2004,20 +2487,45 @@ const doesCleanupFunctionReleaseUsage = ( helperFunction, usage, context, - requiresDirectReleasePathCoverage, new Set(visitedFunctions), helperParameterSubstitutions, + requireExhaustivePaths, ) ) { matchingLoopOrHelperAnchors.push(cleanupCall); } }); + const cleanupBodyRoot = findTransparentExpressionRoot(cleanupFunction.body); + if ( + requireExhaustivePaths && + matchingLoopOrHelperAnchors.some( + (releaseAnchor) => findTransparentExpressionRoot(releaseAnchor) === cleanupBodyRoot, + ) + ) { + return true; + } return ( didCleanupFunctionMatch || doNodesCoverEveryPathFromFunctionEntry(cleanupFunction, matchingLoopOrHelperAnchors, context) ); }; +const cleanupReturnsExhaustivelyReleaseUsage = ( + cleanupReturns: ReadonlyArray, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => + cleanupReturns.length > 0 && + cleanupReturns.every((cleanupReturn) => { + if (!isNodeOfType(cleanupReturn, "ReturnStatement") || !cleanupReturn.argument) return false; + const cleanupFunction = resolveStableValue(cleanupReturn.argument, context); + return Boolean( + cleanupFunction && + isFunctionLike(cleanupFunction) && + doesCleanupFunctionReleaseUsage(cleanupFunction, usage, context, new Set(), new Map(), true), + ); + }); + const doesBoundCleanupReleaseUsage = ( expression: EsTreeNode, usage: SubscribeLikeUsage, @@ -2142,31 +2650,6 @@ const doesTestRequireLiveExpressionKey = ( ); }; -const findBlockingLiveHandleGuard = ( - callback: EsTreeNode, - usageNode: EsTreeNode, - handleKey: string, - context: RuleContext, -): EsTreeNodeOfType<"IfStatement"> | null => { - if (!isFunctionLike(callback)) return null; - let matchingGuard: EsTreeNodeOfType<"IfStatement"> | null = null; - walkAst(callback.body, (child: EsTreeNode) => { - if (matchingGuard) return false; - if (child !== callback.body && isFunctionLike(child)) return false; - if ( - isNodeOfType(child, "IfStatement") && - !child.alternate && - doesTestRequireLiveExpressionKey(child.test, handleKey, context) && - !canNodeReachLaterNodeWithinFunction(child.consequent, usageNode, callback, context) && - doMatchingNodesCoverEveryPathBeforeUsage(usageNode, [child], callback, context) - ) { - matchingGuard = child; - return false; - } - }); - return matchingGuard; -}; - const findLiveExpressionGuardForRelease = ( releaseCall: EsTreeNode, owner: EsTreeNode, @@ -2286,7 +2769,6 @@ const hasRerunReleaseBeforeUsage = ( helperFunction, usage, context, - false, new Set(), helperParameterSubstitutions, ) @@ -2724,128 +3206,372 @@ const cleanupReturnsReleaseUsage = ( ); }); -const findSubscriptionRegistrationsForHandler = ( - handlerFunction: EsTreeNode, - synchronouslyInvokedFunctions: ReadonlySet, +const hasLiveHandleOverwriteProtection = ( + usageFunction: EsTreeNode, + usage: SubscribeLikeUsage, context: RuleContext, -): EsTreeNodeOfType<"CallExpression">[] | null => { - if (!isFunctionLike(handlerFunction) || handlerFunction.async || handlerFunction.generator) { - return null; - } - const handlerRoot = findTransparentExpressionRoot(handlerFunction); - const inlineRegistration = handlerRoot.parent; - if ( - isNodeOfType(inlineRegistration, "CallExpression") && - inlineRegistration.arguments.some((argument) => argument === handlerRoot) && - isSubscribeOrObserveCallExpression(inlineRegistration) - ) { - return [inlineRegistration]; - } - const bindingIdentifier = getFunctionBindingIdentifier(handlerFunction); - const handlerSymbol = bindingIdentifier ? context.scopes.symbolFor(bindingIdentifier) : null; - if (!handlerSymbol || handlerSymbol.references.length === 0) return null; - const registrations: EsTreeNodeOfType<"CallExpression">[] = []; - for (const reference of handlerSymbol.references) { - const directCall = findDirectCallForReference(reference.identifier); - if (directCall) { - const invocationOwner = findEnclosingFunction(directCall); - if ( - !invocationOwner || - invocationOwner === handlerFunction || - !synchronouslyInvokedFunctions.has(invocationOwner) - ) { - return null; - } - continue; +): boolean => { + const handleKey = usage.handleKey; + if (!isFunctionLike(usageFunction) || handleKey === null) return false; + const usageStart = getRangeStart(usage.node); + if (usageStart === null) return false; + let didFindEarlyReturnGuard = false; + const releaseBeforeReplacementAnchors: EsTreeNode[] = []; + walkAst(usageFunction.body, (child: EsTreeNode) => { + if (didFindEarlyReturnGuard) return false; + if (child !== usageFunction.body && isFunctionLike(child)) return false; + if ( + isNodeOfType(child, "IfStatement") && + !child.alternate && + doesTestRequireLiveExpressionKey(child.test, handleKey, context) && + !canNodeReachLaterNodeWithinFunction(child.consequent, usage.node, usageFunction, context) && + doMatchingNodesCoverEveryPathBeforeUsage(usage.node, [child], usageFunction, context) + ) { + didFindEarlyReturnGuard = true; + return false; } - const referenceRoot = findTransparentExpressionRoot(reference.identifier); - const registration = referenceRoot.parent; + const childStart = getRangeStart(child); if ( - !isNodeOfType(registration, "CallExpression") || - !registration.arguments.some((argument) => argument === referenceRoot) || - !isSubscribeOrObserveCallExpression(registration) + childStart === null || + childStart >= usageStart || + !doesNodeOrCalledHelperReleaseUsage(child, usage, context) ) { - return null; + return; } - registrations.push(registration); - } - return [...new Set(registrations)]; -}; - -const isEffectOwnedSubscriptionHandler = ( - handlerFunction: EsTreeNode, + const handleGuard = findDirectHandleGuardForRelease(child, usageFunction, usage, context); + releaseBeforeReplacementAnchors.push(handleGuard ?? child); + }); + return ( + didFindEarlyReturnGuard || + doMatchingNodesCoverEveryPathBeforeUsage( + usage.node, + releaseBeforeReplacementAnchors, + usageFunction, + context, + ) + ); +}; + +const getUsageCallbackKey = (usage: SubscribeLikeUsage, context: RuleContext): string | null => { + if (usage.kind === "subscribe") { + return ( + usage.handlerKey ?? resolveExpressionKey(getSubscribeUsageCallbackArgument(usage), context) + ); + } + if (usage.kind !== "timer" || !isNodeOfType(usage.node, "CallExpression")) return null; + return resolveExpressionKey(usage.node.arguments?.[0], context); +}; + +const getFunctionIdentityKeys = ( + functionNode: EsTreeNode, + context: RuleContext, +): ReadonlySet => { + const bindingIdentifier = getFunctionBindingIdentifier(functionNode); + return new Set( + [ + resolveExpressionKey(functionNode, context), + resolveExpressionKey(bindingIdentifier, context), + ].filter((identityKey): identityKey is string => identityKey !== null), + ); +}; + +const doesCleanupOwnUsageAfterRegistration = ( callback: EsTreeNode, + usage: SubscribeLikeUsage, cleanupReturns: ReadonlyArray, - synchronouslyInvokedFunctions: ReadonlySet, context: RuleContext, -): boolean => { - const registrations = findSubscriptionRegistrationsForHandler( - handlerFunction, - synchronouslyInvokedFunctions, +): boolean => + cleanupReturnsExhaustivelyReleaseUsage(cleanupReturns, usage, context) && + doMatchingNodesCoverEveryPathAfterUsage( + resolveCleanupPathAnchor(usage.node, callback, context), + cleanupReturns, context, ); - if ( - !registrations || - registrations.length === 0 || - !doNodesCoverEveryPathFromFunctionEntry(callback, cleanupReturns, context) + +const doesNodeOrCalledHelperReleaseUsage = ( + node: EsTreeNode, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + if (doesReleaseCallMatchUsage(node, usage, context)) return true; + const callNode = isNodeOfType(node, "ChainExpression") ? node.expression : node; + if (!isNodeOfType(callNode, "CallExpression")) return false; + const helperFunction = resolveStableValue(callNode.callee, context); + return Boolean( + helperFunction && + isFunctionLike(helperFunction) && + !helperFunction.async && + !helperFunction.generator && + doesCleanupFunctionReleaseUsage(helperFunction, usage, context, new Set(), new Map(), true), + ); +}; + +const resolveNestedTimerStorageSymbol = ( + assignmentTarget: EsTreeNode, + callback: EsTreeNode, + context: RuleContext, +): SymbolDescriptor | null => { + const unwrappedTarget = stripParenExpression(assignmentTarget); + if (isNodeOfType(unwrappedTarget, "Identifier")) { + const symbol = context.scopes.symbolFor(unwrappedTarget); + return symbol && + (symbol.kind === "let" || symbol.kind === "var") && + isNodeOfType(symbol.declarationNode, "VariableDeclarator") && + findEnclosingFunction(symbol.declarationNode) === callback + ? symbol + : null; + } + const refSymbol = resolveReactRefSymbol(unwrappedTarget, context.scopes, { + resolveNamedAliases: true, + }); + if (refSymbol) return refSymbol; + let storageObject: EsTreeNode = unwrappedTarget; + while (isNodeOfType(storageObject, "MemberExpression")) { + storageObject = stripParenExpression(storageObject.object); + } + if (!isNodeOfType(storageObject, "Identifier")) return null; + const storageSymbol = context.scopes.symbolFor(storageObject); + const initializer = storageSymbol?.initializer + ? stripParenExpression(storageSymbol.initializer) + : null; + return storageSymbol && + storageSymbol.kind === "const" && + isNodeOfType(storageSymbol.declarationNode, "VariableDeclarator") && + isNodeOfType(initializer, "ObjectExpression") && + findEnclosingFunction(storageSymbol.declarationNode) === callback + ? storageSymbol + : null; +}; + +const getOutermostMemberReference = (identifier: EsTreeNode): EsTreeNode => { + let expression: EsTreeNode = identifier; + while ( + isNodeOfType(expression.parent, "MemberExpression") && + expression.parent.object === expression ) { - return false; + expression = expression.parent; } - return registrations.every((registration) => { - const registrationOwner = findEnclosingFunction(registration); + return findTransparentExpressionRoot(expression); +}; + +const hasOnlySafeHandleStorageAssignments = ( + usage: SubscribeLikeUsage, + handleStorageSymbol: SymbolDescriptor, + usageAssignment: EsTreeNodeOfType<"AssignmentExpression">, + context: RuleContext, +): boolean => + handleStorageSymbol.references.every((reference) => { + const assignmentTarget = getOutermostMemberReference(reference.identifier); + if (resolveExpressionKey(assignmentTarget, context) !== usage.handleKey) { + return isNodeOfType(usageAssignment.left, "Identifier"); + } + if (!isWithinAssignmentTarget(reference.identifier)) return true; + const assignment = assignmentTarget.parent; + if (assignment === usageAssignment) return true; if ( - registrationOwner !== callback && - (!registrationOwner || !synchronouslyInvokedFunctions.has(registrationOwner)) + !isNodeOfType(assignment, "AssignmentExpression") || + assignment.operator !== "=" || + assignment.left !== assignmentTarget ) { return false; } - const registrationDetails = getCallRegistrationDetails(registration, context); - const registrationUsage: SubscribeLikeUsage = { - kind: "subscribe", - node: registration, - resourceName: registrationDetails.registrationVerbName ?? "subscribe", - handleKey: findAssignedResourceKey(registration, context, true), - ...registrationDetails, - }; - return cleanupReturnsReleaseUsage(cleanupReturns, registrationUsage, context); + const assignedValue = stripParenExpression(assignment.right); + const isNullishReset = + (isNodeOfType(assignedValue, "Literal") && assignedValue.value === null) || + (isNodeOfType(assignedValue, "Identifier") && + assignedValue.name === "undefined" && + context.scopes.isGlobalReference(assignedValue)); + const assignmentOwner = findEnclosingFunction(assignment); + if (!isNullishReset || !assignmentOwner || !isFunctionLike(assignmentOwner)) return false; + const matchingReleaseCalls: EsTreeNode[] = []; + walkAst(assignmentOwner.body, (child: EsTreeNode) => { + if (child !== assignmentOwner.body && isFunctionLike(child)) return false; + if (doesNodeOrCalledHelperReleaseUsage(child, usage, context)) { + matchingReleaseCalls.push(child); + } + }); + return doMatchingNodesCoverEveryPathBeforeUsage( + assignment, + matchingReleaseCalls, + assignmentOwner, + context, + ); }); + +const isEffectOwnedDirectTimerCollection = ( + callback: EsTreeNode, + usage: SubscribeLikeUsage, + context: RuleContext, +): boolean => { + const collectionSymbol = resolveDirectResourcePushCollectionSymbol(usage.node, context); + return Boolean( + collectionSymbol && findEnclosingFunction(collectionSymbol.declarationNode) === callback, + ); }; -const hasOnlyEffectOwnedFunctionInvocations = ( +const isSelfReschedulingOneShotTimer = ( + usage: SubscribeLikeUsage, usageFunction: EsTreeNode, + allUsages: ReadonlyArray, + context: RuleContext, +): boolean => { + if (usage.registrationVerbName !== "setTimeout") return false; + const functionIdentityKeys = getFunctionIdentityKeys(usageFunction, context); + if (!functionIdentityKeys.has(getUsageCallbackKey(usage, context) ?? "")) return false; + const selfSchedulingUsages = allUsages.filter( + (candidateUsage) => + candidateUsage.kind === "timer" && + findEnclosingFunction(candidateUsage.node) === usageFunction && + functionIdentityKeys.has(getUsageCallbackKey(candidateUsage, context) ?? ""), + ); + return selfSchedulingUsages.length === 1 && selfSchedulingUsages[0] === usage; +}; + +const hasEffectOwnedNestedTimerCleanup = ( callback: EsTreeNode, + usage: SubscribeLikeUsage, + allUsages: ReadonlyArray, cleanupReturns: ReadonlyArray, context: RuleContext, ): boolean => { - const bindingIdentifier = getFunctionBindingIdentifier(usageFunction); - const functionSymbol = bindingIdentifier ? context.scopes.symbolFor(bindingIdentifier) : null; + const usageFunction = findEnclosingFunction(usage.node); + const usageExpression = findTransparentExpressionRoot(usage.node); + const usageAssignment = usageExpression.parent; + const isAssignedHandle = + usage.handleKey !== null && + isNodeOfType(usageAssignment, "AssignmentExpression") && + usageAssignment.operator === "=" && + usageAssignment.right === usageExpression; + const handleStorageSymbol = isAssignedHandle + ? resolveNestedTimerStorageSymbol(usageAssignment.left, callback, context) + : null; + const isOwnedCollection = isEffectOwnedDirectTimerCollection(callback, usage, context); + if ( + usage.kind !== "timer" || + !usageFunction || + !isFunctionLike(usageFunction) || + usageFunction === callback || + usageFunction.async || + usageFunction.generator || + (!handleStorageSymbol && !isOwnedCollection) || + !cleanupReturnsExhaustivelyReleaseUsage(cleanupReturns, usage, context) + ) { + return false; + } + if ( + handleStorageSymbol && + isNodeOfType(usageAssignment, "AssignmentExpression") && + !hasOnlySafeHandleStorageAssignments(usage, handleStorageSymbol, usageAssignment, context) + ) { + return false; + } + const isSelfRescheduling = isSelfReschedulingOneShotTimer( + usage, + usageFunction, + allUsages, + context, + ); + if ( + handleStorageSymbol && + !isSelfRescheduling && + !hasLiveHandleOverwriteProtection(usageFunction, usage, context) + ) { + return false; + } + let usageAncestor: EsTreeNode | null | undefined = usage.node.parent; + while (usageAncestor && usageAncestor !== usageFunction) { + if ( + handleStorageSymbol && + (isNodeOfType(usageAncestor, "ForStatement") || + isNodeOfType(usageAncestor, "ForInStatement") || + isNodeOfType(usageAncestor, "ForOfStatement") || + isNodeOfType(usageAncestor, "WhileStatement") || + isNodeOfType(usageAncestor, "DoWhileStatement")) + ) { + return false; + } + usageAncestor = usageAncestor.parent; + } + const functionBindingIdentifier = getFunctionBindingIdentifier(usageFunction); + const functionSymbol = functionBindingIdentifier + ? context.scopes.symbolFor(functionBindingIdentifier) + : null; if (!functionSymbol || functionSymbol.references.length === 0) return false; + const selfSchedulingReferences = functionSymbol.references.filter( + (reference) => + isSelfRescheduling && + isAstDescendant(reference.identifier, usage.node) && + getUsageCallbackKey(usage, context) === resolveExpressionKey(reference.identifier, context), + ); + if ( + isSelfRescheduling && + (selfSchedulingReferences.length !== 1 || functionSymbol.references.length !== 2) + ) { + return false; + } const synchronouslyInvokedFunctions = collectSynchronouslyEffectInvokedFunctions( callback, context.scopes, ); return functionSymbol.references.every((reference) => { - const directCall = findDirectCallForReference(reference.identifier); - if (!directCall) return false; - const invocationOwner = findEnclosingFunction(directCall); - if (!invocationOwner) return false; - if (invocationOwner === callback) return true; - const registrations = findSubscriptionRegistrationsForHandler( - invocationOwner, - synchronouslyInvokedFunctions, - context, + const referenceKey = resolveExpressionKey(reference.identifier, context); + if (selfSchedulingReferences.some((candidate) => candidate === reference)) return true; + const callbackOwnerUsage = allUsages.find( + (candidateUsage) => + candidateUsage !== usage && + referenceKey !== null && + getUsageCallbackKey(candidateUsage, context) === referenceKey, ); - if (registrations === null) return false; + const callbackOwnerArgument = callbackOwnerUsage + ? getSubscribeUsageCallbackArgument(callbackOwnerUsage) + : null; + if ( + callbackOwnerUsage && + !( + callbackOwnerArgument && + isFunctionLike(callbackOwnerArgument) && + callbackOwnerArgument.async + ) && + doesCleanupOwnUsageAfterRegistration(callback, callbackOwnerUsage, cleanupReturns, context) + ) { + return true; + } + const invocationCall = findDirectCallForReference(reference.identifier); + if (!invocationCall) return false; + const invocationOwner = findEnclosingFunction(invocationCall); + if (!invocationOwner || !isFunctionLike(invocationOwner) || invocationOwner === usageFunction) { + return false; + } + if (invocationOwner.async || invocationOwner.generator) return false; + if (invocationOwner === callback) { + return doMatchingNodesCoverEveryPathAfterUsage( + resolveCleanupPathAnchor(invocationCall, callback, context), + cleanupReturns, + context, + ); + } + const invocationOwnerKeys = getFunctionIdentityKeys(invocationOwner, context); + const ownerUsage = allUsages.find((candidateUsage) => { + if (candidateUsage === usage) return false; + const callbackArgument = getSubscribeUsageCallbackArgument(candidateUsage); + const resolvedCallback = callbackArgument + ? resolveStableValue(callbackArgument, context) + : null; + return ( + invocationOwnerKeys.has(getUsageCallbackKey(candidateUsage, context) ?? "") || + resolvedCallback === invocationOwner + ); + }); + if (ownerUsage) { + return doesCleanupOwnUsageAfterRegistration(callback, ownerUsage, cleanupReturns, context); + } return ( - (registrations.length === 0 && synchronouslyInvokedFunctions.has(invocationOwner)) || - (registrations.length > 0 && - isEffectOwnedSubscriptionHandler( - invocationOwner, - callback, - cleanupReturns, - synchronouslyInvokedFunctions, - context, - )) + synchronouslyInvokedFunctions.has(invocationOwner) && + doMatchingNodesCoverEveryPathAfterUsage( + resolveCleanupPathAnchor(invocationCall, callback, context), + cleanupReturns, + context, + ) ); }); }; @@ -2962,32 +3688,6 @@ const hasGuardedRefOwnedNestedCleanup = ( ); }; -const collectGlobalReleaseProofs = ( - cleanupFunction: EsTreeNode, - usage: SubscribeLikeUsage, - context: RuleContext, -): GlobalReleaseProof[] => { - if (!isFunctionLike(cleanupFunction)) return []; - const globalReleaseProofs: GlobalReleaseProof[] = []; - walkAst(cleanupFunction.body, (child: EsTreeNode) => { - if (child !== cleanupFunction.body && isFunctionLike(child)) return false; - if ( - isNodeOfType(child, "CallExpression") && - isNodeOfType(child.callee, "Identifier") && - context.scopes.isGlobalReference(child.callee) && - doesReleaseCallMatchUsage(child, usage, context) - ) { - const handleGuard = findDirectHandleGuardForRelease(child, cleanupFunction, usage, context); - globalReleaseProofs.push({ - anchor: handleGuard ?? child, - call: child, - handleGuard, - }); - } - }); - return globalReleaseProofs; -}; - const hasGuardedDeferredCleanup = ( callback: EsTreeNode, usage: SubscribeLikeUsage, @@ -2999,14 +3699,6 @@ const hasGuardedDeferredCleanup = ( } const usageFunction = findEnclosingFunction(usage.node); const promiseChainCall = usageFunction ? getPromiseChainCallForCallback(usageFunction) : null; - const hasEffectOwnedUsageInvocations = Boolean( - usageFunction && - isFunctionLike(usageFunction) && - hasOnlyEffectOwnedFunctionInvocations(usageFunction, callback, cleanupReturns, context), - ); - const hasEffectOwnedCleanupPath = - hasEffectOwnedUsageInvocations && - doNodesCoverEveryPathFromFunctionEntry(callback, cleanupReturns, context); if ( usage.kind !== "timer" || usage.handleKey === null || @@ -3018,10 +3710,9 @@ const hasGuardedDeferredCleanup = ( !isNodeOfType(usage.node, "CallExpression") || !isNodeOfType(usage.node.callee, "Identifier") || !context.scopes.isGlobalReference(usage.node.callee) || + !promiseChainCall || !collectEffectInvokedFunctions(callback, context.scopes).has(usageFunction) || - (!promiseChainCall && !hasEffectOwnedCleanupPath) || - (promiseChainCall && - !doMatchingNodesCoverEveryPathAfterUsage(promiseChainCall, cleanupReturns, context)) + !doMatchingNodesCoverEveryPathAfterUsage(promiseChainCall, cleanupReturns, context) ) { return false; } @@ -3053,15 +3744,30 @@ const hasGuardedDeferredCleanup = ( const globalReleaseProofsByCleanup = new Map(); for (const cleanupFunction of cleanupFunctions) { if (!isFunctionLike(cleanupFunction)) return false; - const globalReleaseProofs = collectGlobalReleaseProofs(cleanupFunction, usage, context); - const hasRequiredRelease = promiseChainCall - ? doNodesCoverEveryPathFromFunctionEntry( - cleanupFunction, - globalReleaseProofs.map((releaseProof) => releaseProof.anchor), - context, - ) - : doesCleanupFunctionReleaseUsage(cleanupFunction, usage, context, true); - if (!hasRequiredRelease) { + const globalReleaseProofs: GlobalReleaseProof[] = []; + walkAst(cleanupFunction.body, (child: EsTreeNode) => { + if (child !== cleanupFunction.body && isFunctionLike(child)) return false; + if ( + isNodeOfType(child, "CallExpression") && + isNodeOfType(child.callee, "Identifier") && + context.scopes.isGlobalReference(child.callee) && + doesReleaseCallMatchUsage(child, usage, context) + ) { + const handleGuard = findDirectHandleGuardForRelease(child, cleanupFunction, usage, context); + globalReleaseProofs.push({ + anchor: handleGuard ?? child, + call: child, + handleGuard, + }); + } + }); + if ( + !doNodesCoverEveryPathFromFunctionEntry( + cleanupFunction, + globalReleaseProofs.map((releaseProof) => releaseProof.anchor), + context, + ) + ) { return false; } globalReleaseProofsByCleanup.set(cleanupFunction, globalReleaseProofs); @@ -3093,12 +3799,9 @@ const hasGuardedDeferredCleanup = ( if (!isNullishReset) return true; const cleanupFunction = findEnclosingFunction(assignment); const globalReleaseProofs = cleanupFunction - ? (globalReleaseProofsByCleanup.get(cleanupFunction) ?? - (hasEffectOwnedCleanupPath - ? collectGlobalReleaseProofs(cleanupFunction, usage, context) - : undefined)) + ? globalReleaseProofsByCleanup.get(cleanupFunction) : undefined; - const isSafeReset = Boolean( + return !( cleanupFunction && globalReleaseProofs && doMatchingNodesCoverEveryPathBeforeUsage( @@ -3111,9 +3814,8 @@ const hasGuardedDeferredCleanup = ( ), cleanupFunction, context, - ), + ) ); - return !isSafeReset; }); if (!hasUsageAssignment || hasUnsafeHandleAssignment) { return false; @@ -3148,42 +3850,22 @@ const hasGuardedDeferredCleanup = ( }); } if (hasPotentialInterruption) return false; - const guardStates = collectDeferredUsageGuardStates(usageFunction, usage.node, context); - const blockingLiveHandleGuard = findBlockingLiveHandleGuard( - usageFunction, - usage.node, - usage.handleKey, - context, - ); - if (blockingLiveHandleGuard) { - guardStates.push({ - bindingIdentifier: handleSymbol.bindingIdentifier, - guardNode: blockingLiveHandleGuard, - key: usage.handleKey, - value: false, - }); - } - return guardStates.some((guardState) => { - if (hasPotentialInterruptionAfterGuard(usageFunction, guardState, usage.node, context)) { - return false; - } - if (hasEffectOwnedCleanupPath && guardState.key === usage.handleKey) return true; - if (deferredUsageWritesGuardBeforeUsage(usageFunction, usage.node, guardState, context)) { - return false; - } - return ( + return collectDeferredUsageGuardStates(usageFunction, usage.node, context).some( + (guardState) => isEffectLocalLifecycleGuard(callback, guardState, cleanupFunctions, context) && + !hasPotentialInterruptionAfterGuard(usageFunction, guardState, usage.node, context) && + !deferredUsageWritesGuardBeforeUsage(usageFunction, usage.node, guardState, context) && cleanupReturns.every((cleanupReturn) => cleanupReturnInvalidatesGuard(cleanupReturn, guardState, context), - ) - ); - }); + ), + ); }; const effectHasCleanupForUsage = ( callback: EsTreeNode, usage: SubscribeLikeUsage, context: RuleContext, + allUsages: ReadonlyArray = [usage], ): boolean => { if ( !isNodeOfType(callback, "ArrowFunctionExpression") && @@ -3238,17 +3920,14 @@ const effectHasCleanupForUsage = ( (returnedValue === usage.node || (getRangeStart(returnedValue) !== null && getRangeStart(returnedValue) === getRangeStart(usage.node))) && - isCleanupReturningSubscribeLikeCallExpression(returnedValue) + isKnownCallableSubscriptionResult(usage, context) ) { matchingCleanupReturns.push(child); return; } if ( usage.kind === "subscribe" && - isNodeOfType(returnedValue, "Identifier") && - usage.handleKey !== null && - resolveExpressionKey(returnedValue, context) === usage.handleKey && - isKnownCallableSubscriptionResult(usage, context) + doesStableIdentifierCallUsageDisposer(returnedValue, usage, context) ) { matchingCleanupReturns.push(child); return; @@ -3257,8 +3936,6 @@ const effectHasCleanupForUsage = ( if (returnedValue.name === "undefined" && context.scopes.isGlobalReference(returnedValue)) { return; } - const returnedKey = resolveExpressionKey(returnedValue, context); - if (usage.handleKey !== null && returnedKey === usage.handleKey) return; const returnedSymbol = context.scopes.symbolFor(returnedValue); if (!returnedSymbol?.initializer) return; } @@ -3273,6 +3950,8 @@ const effectHasCleanupForUsage = ( cleanupFunction, usage, context, + new Set(), + new Map(), requiresDirectReleasePathCoverage, ) ) { @@ -3282,8 +3961,13 @@ const effectHasCleanupForUsage = ( if (hasGuardedDeferredCleanup(callback, usage, matchingCleanupReturns, context)) { return true; } + if ( + hasEffectOwnedNestedTimerCleanup(callback, usage, allUsages, matchingCleanupReturns, context) + ) { + return true; + } return doMatchingNodesCoverEveryPathAfterUsage( - resolveCleanupPathAnchor(usage, callback, context), + resolveCleanupPathAnchor(usage.node, callback, context, usage), matchingCleanupReturns, context, ); @@ -3296,7 +3980,7 @@ const findFirstUsageWithoutCleanup = ( ): SubscribeLikeUsage | null => { for (const usage of usages) { if ( - !effectHasCleanupForUsage(callback, usage, context) && + !effectHasCleanupForUsage(callback, usage, context, usages) && !hasSplitLifecycleCleanup(callback, usage, context) ) { return usage; @@ -3756,7 +4440,7 @@ const findDirectExhaustiveForEachCleanupFunction = ( } }; -const collectReplayOwnerFunctions = (usageNode: EsTreeNode): Set => { +const collectEnclosingOwnerFunctions = (usageNode: EsTreeNode): Set => { const ownerFunctions = new Set(); let currentNode = usageNode; while (true) { @@ -3764,9 +4448,7 @@ const collectReplayOwnerFunctions = (usageNode: EsTreeNode): Set => if (!ownerFunction || !isFunctionLike(ownerFunction) || ownerFunctions.has(ownerFunction)) break; ownerFunctions.add(ownerFunction); - const forEachCall = findEnclosingForEachCall(ownerFunction); - if (!forEachCall) break; - currentNode = forEachCall; + currentNode = ownerFunction; } return ownerFunctions; }; @@ -3774,14 +4456,137 @@ const collectReplayOwnerFunctions = (usageNode: EsTreeNode): Set => const hasCollectionMutationBeforeRelease = ( usageNode: EsTreeNode, releaseNode: EsTreeNode, - collectionKeys: ReadonlySet, + collectionMutationLimits: ReadonlyMap, context: RuleContext, ): boolean => { const usageStart = getRangeStart(usageNode); const releaseStart = getRangeStart(releaseNode); if (usageStart === null || releaseStart === null) return true; - const setupOwnerFunctions = collectReplayOwnerFunctions(usageNode); - const cleanupOwnerFunctions = collectReplayOwnerFunctions(releaseNode); + const setupOwnerFunctions = collectEnclosingOwnerFunctions(usageNode); + const cleanupOwnerFunctions = collectEnclosingOwnerFunctions(releaseNode); + const isCollectionKeyRelevantAt = (collectionKey: string | null, sourceStart: number): boolean => + collectionKey !== null && + sourceStart <= (collectionMutationLimits.get(collectionKey) ?? Number.NEGATIVE_INFINITY); + const doesNodeMutateCollection = ( + node: EsTreeNode, + executionStart: number, + visitedFunctions: ReadonlySet, + doesReturnEscape: boolean, + ): boolean => { + if ( + doesReturnEscape && + isNodeOfType(node, "ReturnStatement") && + isCollectionKeyRelevantAt(resolveExpressionKey(node.argument, context), executionStart) + ) { + return true; + } + if (isNodeOfType(node, "AssignmentExpression")) { + const assignmentKey = resolveExpressionKey(node.left, context); + const assignmentTarget = stripParenExpression(node.left); + const assignedValueKey = resolveExpressionKey(node.right, context); + return ( + (isCollectionKeyRelevantAt(assignedValueKey, executionStart) && + !isCollectionKeyRelevantAt(assignmentKey, executionStart)) || + Boolean( + assignmentKey && + [...collectionMutationLimits].some( + ([collectionKey, mutationLimit]) => + executionStart <= mutationLimit && + (assignmentKey === collectionKey || assignmentKey === `${collectionKey}.length`), + ), + ) || + (isNodeOfType(assignmentTarget, "MemberExpression") && + assignmentTarget.computed && + isCollectionKeyRelevantAt( + resolveExpressionKey(assignmentTarget.object, context), + executionStart, + )) + ); + } + if (isNodeOfType(node, "UnaryExpression") && node.operator === "delete") { + const deletedMember = stripParenExpression(node.argument); + return ( + isNodeOfType(deletedMember, "MemberExpression") && + isCollectionKeyRelevantAt( + resolveExpressionKey(deletedMember.object, context), + executionStart, + ) + ); + } + if (isNodeOfType(node, "UpdateExpression")) { + const updatedKey = resolveExpressionKey(node.argument, context); + return Boolean( + updatedKey && + [...collectionMutationLimits].some( + ([collectionKey, mutationLimit]) => + executionStart <= mutationLimit && updatedKey === `${collectionKey}.length`, + ), + ); + } + if (!isNodeOfType(node, "CallExpression") && !isNodeOfType(node, "NewExpression")) { + return false; + } + const doesReceiveCollection = node.arguments.some((argument) => { + if (!isAstNode(argument)) return false; + const argumentKey = resolveExpressionKey(argument, context); + return ( + isCollectionKeyRelevantAt(argumentKey, executionStart) && + resolveIteratorCollectionKey(argument, context) === null + ); + }); + const callee = stripParenExpression(node.callee); + const isArrayFromCopy = + isNodeOfType(node, "CallExpression") && + isNodeOfType(callee, "MemberExpression") && + !callee.computed && + isNodeOfType(callee.object, "Identifier") && + callee.object.name === "Array" && + context.scopes.isGlobalReference(callee.object) && + isNodeOfType(callee.property, "Identifier") && + callee.property.name === "from"; + if (doesReceiveCollection && !isArrayFromCopy) return true; + if ( + isNodeOfType(callee, "MemberExpression") && + !callee.computed && + isNodeOfType(callee.property, "Identifier") && + (REPLAY_ENTRY_DROPPING_ARRAY_METHOD_NAMES.has(callee.property.name) || + REPLAY_ENTRY_DROPPING_COLLECTION_METHOD_NAMES.has(callee.property.name)) && + isCollectionKeyRelevantAt(resolveExpressionKey(callee.object, context), executionStart) + ) { + return true; + } + if (!isNodeOfType(node, "CallExpression")) return false; + const executedFunctions = [ + resolveExactLocalFunction(node.callee, context.scopes), + ...node.arguments.flatMap((argument) => + isAstNode(argument) && isSynchronousIteratorCallbackCall(node, argument) + ? [resolveExactLocalFunction(argument, context.scopes)] + : [], + ), + ]; + return executedFunctions.some((executedFunction) => { + if ( + !executedFunction || + !isFunctionLike(executedFunction) || + executedFunction.generator || + visitedFunctions.has(executedFunction) + ) { + return false; + } + const nextVisitedFunctions = new Set(visitedFunctions); + nextVisitedFunctions.add(executedFunction); + let didExecutedFunctionMutateCollection = false; + walkAst(executedFunction.body, (executedNode: EsTreeNode) => { + if (didExecutedFunctionMutateCollection) return false; + if (executedNode !== executedFunction.body && isFunctionLike(executedNode)) return false; + if (doesNodeMutateCollection(executedNode, executionStart, nextVisitedFunctions, false)) { + didExecutedFunctionMutateCollection = true; + return false; + } + }); + return didExecutedFunctionMutateCollection; + }); + }; let programNode = usageNode; while (programNode.parent) programNode = programNode.parent; let didFindMutation = false; @@ -3794,56 +4599,7 @@ const hasCollectionMutationBeforeRelease = ( const isAfterRegistration = setupOwnerFunctions.has(ownerFunction) && childStart > usageStart; const isBeforeRelease = cleanupOwnerFunctions.has(ownerFunction) && childStart < releaseStart; if (!isAfterRegistration && !isBeforeRelease) return; - if (isNodeOfType(child, "AssignmentExpression")) { - const assignmentKey = resolveExpressionKey(child.left, context); - const assignmentTarget = stripParenExpression(child.left); - if ( - (assignmentKey && - [...collectionKeys].some( - (collectionKey) => - assignmentKey === collectionKey || assignmentKey === `${collectionKey}.length`, - )) || - (isNodeOfType(assignmentTarget, "MemberExpression") && - assignmentTarget.computed && - collectionKeys.has(resolveExpressionKey(assignmentTarget.object, context) ?? "")) - ) { - didFindMutation = true; - return false; - } - return; - } - if (isNodeOfType(child, "UnaryExpression") && child.operator === "delete") { - const deletedMember = stripParenExpression(child.argument); - if (!isNodeOfType(deletedMember, "MemberExpression")) return; - if (collectionKeys.has(resolveExpressionKey(deletedMember.object, context) ?? "")) { - didFindMutation = true; - return false; - } - return; - } - if (isNodeOfType(child, "UpdateExpression")) { - const updatedKey = resolveExpressionKey(child.argument, context); - if ( - updatedKey && - [...collectionKeys].some((collectionKey) => updatedKey === `${collectionKey}.length`) - ) { - didFindMutation = true; - return false; - } - return; - } - if (!isNodeOfType(child, "CallExpression")) return; - const callee = stripParenExpression(child.callee); - if ( - !isNodeOfType(callee, "MemberExpression") || - callee.computed || - !isNodeOfType(callee.property, "Identifier") || - (!REPLAY_ENTRY_DROPPING_ARRAY_METHOD_NAMES.has(callee.property.name) && - !REPLAY_ENTRY_DROPPING_COLLECTION_METHOD_NAMES.has(callee.property.name)) || - !collectionKeys.has(resolveExpressionKey(callee.object, context) ?? "") - ) { - return; - } + if (!doesNodeMutateCollection(child, childStart, new Set(), true)) return; didFindMutation = true; return false; }); @@ -3929,7 +4685,7 @@ const hasSafeForEachProjectionCleanup = ( return !hasCollectionMutationBeforeRelease( registrationCall, releaseCall, - collectionKeys, + new Map([...collectionKeys].map((collectionKey) => [collectionKey, Number.POSITIVE_INFINITY])), context, ); }; @@ -3972,28 +4728,18 @@ const doesReleaseCallMatchUsage = ( ); } - if (isNodeOfType(callee, "Identifier") && usage.kind === "subscribe") { - if (doesResourceKeyMatchUsageHandle(resolveExpressionKey(callee, context), usage, context)) { - return true; - } - const mappedCollectionKey = findMappedResourceCollectionKey(usage.node, context); - if (mappedCollectionKey !== null) { - const collectionKeys = new Set([mappedCollectionKey]); - const releaseForOfStatement = findForOfStatementForIteratorExpression(callee, context); - const releaseCollectionKey = releaseForOfStatement - ? resolveExpressionKey(releaseForOfStatement.right, context) - : resolveIteratorCollectionKey(callee, context); - const hasExhaustiveRelease = releaseForOfStatement - ? isDirectExhaustiveForOfRelease(callNode, releaseForOfStatement) - : findDirectExhaustiveForEachCleanupFunction(callNode, collectionKeys, context) !== null; - if ( - mappedCollectionKey === releaseCollectionKey && - hasExhaustiveRelease && - !hasCollectionMutationBeforeRelease(usage.node, callNode, collectionKeys, context) - ) { - return true; - } - } + if ( + isNodeOfType(callee, "Identifier") && + usage.kind === "subscribe" && + ((doesStableIdentifierMatchUsageHandle(callee, usage, context) && + (usage.registrationVerbName !== "addEventListener" && + usage.registrationVerbName !== "addListener" + ? true + : isKnownCallableSubscriptionResult(usage, context))) || + (usage.registrationVerbName === "addListener" && + doesCleanupIteratorMatchUsageCollection(callee, usage, context))) + ) { + return true; } const releaseVerbName = getReleaseVerbName(callNode); @@ -4060,7 +4806,9 @@ const doesReleaseCallMatchUsage = ( } if ( - doesResourceKeyMatchUsageHandle(releaseReceiverKey, usage, context) && + (doesResourceKeyMatchUsageHandle(releaseReceiverKey, usage, context) || + (usage.kind === "subscribe" && + doesCleanupIteratorMatchUsageCollection(callee.object, usage, context))) && (releaseVerbName === "unsubscribe" || releaseVerbName === "unsub" || releaseVerbName === "close" || @@ -4225,7 +4973,7 @@ const doesReleaseCallMatchUsage = ( ) { return false; } - if (!isDirectExhaustiveForOfRelease(callNode, releaseForOfStatement)) return false; + if (!isDirectExhaustiveForOfRelease(callNode, releaseForOfStatement, context)) return false; } } if (releaseVerbName === "on") {