Skip to content

Commit 4516e29

Browse files
authored
Merge pull request #149 from PhysShell/claude/ownts-real-world-fp
feat(ownts): real-world FP benchmark + срезано 17/28 ложных срабатываний
2 parents f2285eb + c7a7127 commit 4516e29

6 files changed

Lines changed: 412 additions & 47 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1122,6 +1122,26 @@ jobs:
11221122
# the listener is released, so no false-positive leak
11231123
[ "$(echo "$expr" | grep -cE '\[(OWN|EFF|DI)[0-9]{3}\]')" -eq 0 ] \
11241124
|| { echo "FAIL: a properly-released listener must not be reported"; exit 1; }
1125+
- name: Real-world cleanup patterns (OSS-benchmark FP fixes) are silent
1126+
run: |
1127+
python frontend/ownts/ownts.py frontend/ownts/examples/EffectRealWorld.tsx \
1128+
-o "$RUNNER_TEMP/rw.facts.json"
1129+
rw=$(python -m ownlang ownir "$RUNNER_TEMP/rw.facts.json")
1130+
echo "$rw"
1131+
# AbortController signal, ref/pre-declared timer handles, nested-block
1132+
# cleanup, observer.subscribe/unsubscribe — all released, zero findings.
1133+
[ "$(echo "$rw" | grep -cE '\[(OWN|EFF|DI)[0-9]{3}\]')" -eq 0 ] \
1134+
|| { echo "FAIL: real-world cleanup patterns must not be reported"; exit 1; }
1135+
- name: False-negative controls (wrong controller / args / conditional cleanup) still leak
1136+
run: |
1137+
python frontend/ownts/ownts.py frontend/ownts/examples/EffectLeakControl.tsx \
1138+
-o "$RUNNER_TEMP/leak.facts.json"
1139+
leak=$(python -m ownlang ownir "$RUNNER_TEMP/leak.facts.json" || true)
1140+
echo "$leak"
1141+
# a release-shaped cleanup that does not release THIS resource must not be
1142+
# over-suppressed — all three controls stay OWN001
1143+
[ "$(echo "$leak" | grep -c 'OWN001')" -eq 3 ] \
1144+
|| { echo "FAIL: broadened matchers must not over-suppress real leaks"; exit 1; }
11251145
- name: The clean fixture (cleanups + useMemo'd dep) is silent
11261146
run: |
11271147
python frontend/ownts/ownts.py frontend/ownts/examples/DashboardClean.tsx \

‎docs/notes/ownts-oss-benchmark.md‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
# OwnTS real-world benchmark — the false-positive reckoning
2+
3+
A field note answering the gating question P-020 set for the OwnTS spike: *does it
4+
find real `useEffect` leaks in real OSS with low false positives?* Short answer for
5+
the first run: **no — 28/28 findings were false positives.** This note records the
6+
corpus, the result, and the prioritized false-positive (FP) catalog the run
7+
produced, plus what was done about it.
8+
9+
## Method
10+
11+
- **Corpus.** Real, published React hook libraries pulled from **npm** (the
12+
session's egress policy blocks `github.com`, so a raw-source clone was not
13+
possible; npm registry is allowed). Limited to packages whose published build is
14+
**modern arrow-function ESM**, which the source-tuned spike parses as-is:
15+
- `@mantine/hooks` 9.4.0
16+
- `@react-hookz/web` 25.2.0
17+
- `@uidotdev/usehooks` 2.4.1
18+
- `usehooks-ts` 3.1.1
19+
- **Caveat.** `ahooks` and `react-use` were pulled too but ship **ES5
20+
(`function(){}`) output**, which the spike (tuned for arrow `() =>` callbacks and
21+
`return () =>` cleanups) does not parse — a separate parser-coverage gap, excluded
22+
from the FP analysis rather than counted as analyzer FPs.
23+
- **Scale.** ~108 `useEffect` calls across 81 files.
24+
- **Run.** Each file → `ownts.extract` + `ownts.extract_effects` → `check_facts`;
25+
findings collected and then **triaged by reading the actual source** around each.
26+
27+
## Result (baseline — before this PR's fixes)
28+
29+
| Code | Findings | True positive | False positive |
30+
|------|---------:|--------------:|---------------:|
31+
| `OWN001` | 28 | 0 | **28** |
32+
| `EFF001` | 0 | — | — |
33+
| **Total** | **28** | **0** | **28 (100%)** |
34+
35+
Zero real leaks, zero effect storms, 28 false positives. These are well-maintained
36+
libraries that *do* clean up — just with patterns the synthetic-fixture-tuned spike
37+
did not model.
38+
39+
## The FP catalog (what real code does that the spike missed)
40+
41+
Ordered by frequency; each is a frontend (not core) limitation.
42+
43+
1. **AbortController `{ signal }`** — listeners added with a `signal` option and torn
44+
down by `controller.abort()` in cleanup (abort removes every signal-bound
45+
listener at once). The spike had no model for it. *(mantine `use-floating-window`,
46+
7 findings.)*
47+
2. **Handle stored in a ref / pre-declared `let`** — `timeoutRef.current =
48+
setTimeout(...)`, `let i; … i = setInterval(...)`. `_lhs_token` only recognized a
49+
same-statement `const/let/var NAME =`, so the handle was "uncapturable" and the
50+
matching `clearTimeout(ref.current)` never matched. *(debounced-value, use-idle,
51+
uidotdev, useVibrate.)*
52+
3. **Cleanup `return` from a nested block** — the `return () => …` lives inside an
53+
`if` / `try` / `.then()` within the effect. `_cleanup_span` only accepted a return
54+
at the effect body's own brace depth (1), so a depth-2 cleanup was invisible and
55+
every resource read as un-released. *(use-media-query, use-scroller, useVibrate.)*
56+
4. **Aliased receiver** — `b.addEventListener(...)` in a `.then(b => …)` callback,
57+
torn down by `battery.removeEventListener(...)` (same object, different
58+
identifier). The full-key match compares the receiver string, so `b ≠ battery`.
59+
*(uidotdev `useBattery`, 4 findings.)*
60+
5. **Cleanup via a named function or a method** — `return cancel`, `ro.unsubscribe`,
61+
`resizeObserver.disconnect()`. The matcher could not follow a function reference
62+
or a bare-statement subscribe. *(react-hookz `useResizeObserver`, debounced-value.)*
63+
6. **One-shot `setTimeout`** with no captured handle — fires once and is gone, not a
64+
retained resource, but flagged like an un-cleared `setInterval`. *(use-focus-trap.)*
65+
66+
## Verdict
67+
68+
The spike was **not alpha-ready** on idiomatic modern hook code: ~100% FP. This is
69+
the honest "value, not form" check the consolidation note asked for. The catalog is
70+
the payoff — a concrete, prioritized list of what to model next, every item a small
71+
frontend extension.
72+
73+
## What this PR changed (after)
74+
75+
This PR teaches the **frontend** (not the core) to recognize the *provable* cleanup
76+
patterns — those where the teardown is visible in the effect — and leaves the
77+
genuinely-hard ones documented rather than guessed. Implemented:
78+
79+
- **AbortController `{ signal }`** (#1): a signal-bound listener is released by a
80+
`controller.abort()` in cleanup.
81+
- **Ref / pre-declared handle** (#2): `_lhs_token` now also recognizes
82+
`ref.current = setTimeout(...)` and `let i; i = setInterval(...)`, so the matching
83+
`clearTimeout(ref.current)` / `clearInterval(i)` lands.
84+
- **Nested-block cleanup** (#3): `_cleanup_span` switched from brace-depth to
85+
**function-depth**, so a `return () => …` inside an `if`/`try`/`.then()` is the
86+
effect's cleanup, while a `return` inside a nested callback still is not.
87+
- **Observer subscribe/unsubscribe** (part of #5): a bare `recv.subscribe(...)` is
88+
released by `recv.unsubscribe(...)` / `recv.disconnect()` on the same receiver.
89+
90+
Pinned by `frontend/ownts/examples/EffectRealWorld.tsx` (distilled from the OSS
91+
cases) + a CI step; `Dashboard`/`EffectEdges` still flag real leaks, proving the
92+
fixes do not over-suppress.
93+
94+
### Result (after)
95+
96+
| | Findings | Confirmed leaks | False positives |
97+
|---|---:|---:|---:|
98+
| **Baseline** | 28 | 0 | 28 |
99+
| **After** | **11** | 0 | 9 FP + 2 borderline |
100+
101+
**17 false positives eliminated** (61%); correctly-silent effects rose from
102+
**80/108 → 97/108** specificity. Still zero confirmed leaks — these are
103+
well-maintained libraries, so that is the *expected* honest outcome, not a miss.
104+
105+
### Residual (deliberately not fixed — they would require guessing)
106+
107+
- **Aliased receiver** (#4): `b.addEventListener(...)` torn down by
108+
`battery.removeEventListener(...)` (same object, different identifier). Needs alias
109+
analysis. *(uidotdev `useBattery`, 4.)*
110+
- **Cross-effect / named-helper cleanup** (#5): the teardown is a `cancel()` /
111+
`removeEventListeners()` helper, often called from a *different* effect. Following
112+
it is interprocedural — out of scope for a narrow frontend. *(mantine
113+
`use-debounced-value` 2, uidotdev `useScript` 2.)*
114+
- **One-shot `setTimeout`** (#6): an uncaptured `setTimeout` fires once and frees
115+
itself; flagging it like a repeating `setInterval` is the conservative-but-noisy
116+
call. Left as-is rather than silently suppressed (which could hide a real
117+
re-render pileup). *(mantine `use-focus-trap`, 1.)*
118+
- **Borderline**: a listener intentionally left on a *cached* shared `<script>` node
119+
(`usehooks-ts` `useScript`, 2) — defensible either way; the spike correctly
120+
observes "never removed."
121+
122+
The honest takeaway: OwnTS is now quiet on idiomatic cleanup, with its remaining
123+
false alarms confined to four named, understood patterns — a real step toward the
124+
"low false positives" bar P-020 set, without a single guessed release.
125+
126+
### Reproduce
127+
128+
```bash
129+
# (egress to npm only; github.com is blocked in this environment)
130+
# versions pinned to the exact builds benchmarked above, so reruns are reproducible
131+
npm pack @mantine/hooks@9.4.0 @react-hookz/web@25.2.0 \
132+
@uidotdev/usehooks@2.4.1 usehooks-ts@3.1.1
133+
# extract each tarball, then run ownts.extract + extract_effects + check_facts
134+
# over every *.js/*.mjs (skip *.d.ts); triage each finding against the source.
135+
```
136+
137+
### Soundness of the new matchers
138+
139+
The broadened release matchers stay **fail-closed** — a release-shaped cleanup that
140+
does not actually release *this* resource still reports the leak, pinned by
141+
`EffectLeakControl.tsx`: a listener bound to `c1.signal` torn down by `c2.abort()`,
142+
a `ro.subscribe(a, h)` "released" by `ro.unsubscribe(b, h)`, and a cleanup returned
143+
only under `if (enabled)` over an unconditional `setInterval` — all three remain
144+
`OWN001`. The signal is tied to its controller, the observer to its argument list,
145+
and a conditional cleanup only releases acquires it dominates (co-guarded in the
146+
same block).
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// False-negative controls for the broadened release matchers (Codex/CodeRabbit):
2+
// each uses a release-shaped cleanup that does NOT actually release THIS resource,
3+
// so it must STILL report a leak. Run:
4+
// python frontend/ownts/ownts.py frontend/ownts/examples/EffectLeakControl.tsx --check
5+
// Expect exactly three OWN001 findings.
6+
import { useEffect } from "react";
7+
8+
export function LeakControl({ enabled }: { enabled: boolean }) {
9+
// (1) wrong controller: the listener is bound to c1's signal, but cleanup aborts
10+
// c2 — the original listener stays live.
11+
useEffect(() => {
12+
const c1 = new AbortController();
13+
const c2 = new AbortController();
14+
window.addEventListener("scroll", onScroll, { signal: c1.signal });
15+
return () => c2.abort();
16+
}, []);
17+
18+
// (2) mismatched unsubscribe args: subscribed with (a, h), torn down with (b, h)
19+
// — the (a, h) subscription leaks.
20+
useEffect(() => {
21+
ro.subscribe(a, h);
22+
return () => ro.unsubscribe(b, h);
23+
}, []);
24+
25+
// (3) conditional cleanup: the timer is created unconditionally, but the cleanup
26+
// is only returned when `enabled` — the !enabled path leaks.
27+
useEffect(() => {
28+
const id = setInterval(tick, 1000);
29+
if (enabled) return () => clearInterval(id);
30+
}, [enabled]);
31+
32+
return <div>leak control</div>;
33+
}
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// Real-world cleanup patterns the OSS benchmark (docs/notes/ownts-oss-benchmark.md)
2+
// found the spike was missing — each distilled to a minimal release that the
3+
// frontend must now recognise. Run:
4+
// python frontend/ownts/ownts.py frontend/ownts/examples/EffectRealWorld.tsx --check
5+
// Expect ZERO findings — every resource here is properly released.
6+
// (The "still leaks" control lives in Dashboard.tsx / EffectEdges.tsx, so this
7+
// fixture also proves the fixes did not over-suppress real leaks.)
8+
import { useEffect, useRef } from "react";
9+
10+
export function RealWorld({ src }: { src: string }) {
11+
// AbortController: listeners bound to a signal, torn down by controller.abort().
12+
useEffect(() => {
13+
const controller = new AbortController();
14+
const { signal } = controller;
15+
window.addEventListener("mousemove", onMove, { signal });
16+
window.addEventListener("mouseup", onEnd, { signal });
17+
return () => controller.abort();
18+
}, []);
19+
20+
// Timer handle stored in a ref, cleared inline in cleanup.
21+
const timeoutRef = useRef(-1);
22+
useEffect(() => {
23+
timeoutRef.current = window.setTimeout(() => onReady(), 200);
24+
return () => window.clearTimeout(timeoutRef.current);
25+
}, []);
26+
27+
// Pre-declared `let` handle, assigned then cleared.
28+
useEffect(() => {
29+
let interval;
30+
interval = setInterval(tick, 1000);
31+
return () => clearInterval(interval);
32+
}, []);
33+
34+
// Cleanup returned from a NESTED block (inside `if`) — still the effect's cleanup.
35+
useEffect(() => {
36+
if ("matchMedia" in window) {
37+
const mq = window.matchMedia(src);
38+
const cb = (e) => onMatch(e.matches);
39+
mq.addEventListener("change", cb);
40+
return () => mq.removeEventListener("change", cb);
41+
}
42+
}, [src]);
43+
44+
// Bare observer.subscribe(...) released by observer.unsubscribe(...) on the same receiver.
45+
useEffect(() => {
46+
ro.subscribe(target, handler);
47+
return () => ro.unsubscribe(target, handler);
48+
}, []);
49+
50+
return <div>real world</div>;
51+
}

0 commit comments

Comments
 (0)