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

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions .github/workflows/ci.yml
Original file line numberDiff line numberDiff line change
Expand Up@@ -1132,16 +1132,30 @@ jobs:
# cleanup, observer.subscribe/unsubscribe — all released, zero findings.
[ "$(echo "$rw" | grep -cE '\[(OWN|EFF|DI)[0-9]{3}\]')" -eq 0 ] \
|| { echo "FAIL: real-world cleanup patterns must not be reported"; exit 1; }
- name: False-negative controls (wrong controller / args / conditional cleanup) still leak
- name: False-negative controls (wrong controller / args / conditional / async arrow+ES5) still leak
run: |
python frontend/ownts/ownts.py frontend/ownts/examples/EffectLeakControl.tsx \
-o "$RUNNER_TEMP/leak.facts.json"
leak=$(python -m ownlang ownir "$RUNNER_TEMP/leak.facts.json" || true)
echo "$leak"
# a release-shaped cleanup that does not release THIS resource must not be
# over-suppressed — all three controls stay OWN001
[ "$(echo "$leak" | grep -c 'OWN001')" -eq 3 ] \
# a release-shaped cleanup that does not release THIS resource (wrong
# controller / mismatched args / conditional return / async arrow effect /
# async ES5 `function` effect) must not be over-suppressed — all five
# controls stay OWN001
[ "$(echo "$leak" | grep -c 'OWN001')" -eq 5 ] \
|| { echo "FAIL: broadened matchers must not over-suppress real leaks"; exit 1; }
- name: ES5 `function` callbacks parse; capture-mismatch leak caught (real bug shape)
run: |
python frontend/ownts/ownts.py frontend/ownts/examples/EffectFunctionCallback.tsx \
-o "$RUNNER_TEMP/fn.facts.json"
fn=$(python -m ownlang ownir "$RUNNER_TEMP/fn.facts.json" || true)
echo "$fn"
# the matched ES5 cleanup is silent; the capture:true-vs-default mismatch
# (react-scroll-to-bottom@4.2.0 shape) is the one OWN001
[ "$(echo "$fn" | grep -c 'OWN001')" -eq 1 ] \
|| { echo "FAIL: expected exactly the capture-mismatch leak"; exit 1; }
echo "$fn" | grep -q "focus" \
|| { echo "FAIL: the leak must be the capture-mismatched focus listener"; exit 1; }
- name: The clean fixture (cleanups + useMemo'd dep) is silent
run: |
python frontend/ownts/ownts.py frontend/ownts/examples/DashboardClean.tsx \
Expand Down
38 changes: 38 additions & 0 deletions docs/notes/ownts-oss-benchmark.md
Original file line numberDiff line numberDiff line change
Expand Up@@ -123,6 +123,44 @@ The honest takeaway: OwnTS is now quiet on idiomatic cleanup, with its remaining
false alarms confined to four named, understood patterns — a real step toward the
"low false positives" bar P-020 set, without a single guessed release.

## First confirmed TRUE positive — a real hanging effect

Hook *libraries* clean up by design, so the honest hunt for a real leak moved to
*application-shaped* component code. In **`react-scroll-to-bottom@4.2.0`**
(`ScrollToBottom/Composer.js:574`):

```js
// as published — transpiled ES5: a `function () {}` cleanup, not an arrow
target.addEventListener('focus', handleFocus, { capture: true, passive: true });
return function () {
return target.removeEventListener('focus', handleFocus); // default capture (false)
};
```

`removeEventListener` must match the capture flag; `capture: true` is added but the
removal uses the default `false`, so the listener is **never removed** — a new one
piles up every time `target` changes. (The cleanup is an ES5 `function () {}`, so it
is only reachable after the parser change below; the bug is then the capture-mismatch
class the listener-key matching (P-148/#145) models, not a missing cleanup.) A second
real one:
**`@reactuses/core@6.4.0`** passes `onPressed('mouse')` (a freshly *returned*
function) to both add and remove, so the drag/touch listeners can never be removed.

## ES5 (`function () {}`) build coverage

The benchmark above is arrow-ESM only. To reach the capture-mismatch bug for the
*right* reason — and to widen the corpus to transpiled output (`ahooks`,
`react-use`, and the Composer file above ship ES5 `function(){}`) — the frontend now
parses `function` callbacks and `return function () {}` cleanups
Comment thread
coderabbitai[bot] marked this conversation as resolved.
(`EffectFunctionCallback.tsx` pins it: a matched ES5 cleanup is silent, the
capture-mismatch is the one finding). The ES5 corpus (`ahooks` + `react-use`, ~93
`useEffect`) then yields 11 findings, again triaged as FPs from the known patterns
plus one new transpilation artifact: **optional-chaining desugaring**
(`ref.current?.removeEventListener` → `(_a = ref.current) ? … : _a.removeEventListener`)
makes the cleanup receiver a temp `_a ≠ ref.current`, which the exact-receiver match
rejects. Documented as residual; alias-resolving `_a` back to `ref.current` is the
next increment.

### Reproduce

```bash
Expand Down
28 changes: 28 additions & 0 deletions frontend/ownts/examples/EffectFunctionCallback.tsx
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
// Transpiled-ES5 shape: `useEffect(function () { … return function () { … } }, …)`.
// Proves the parser handles `function` callbacks AND `return function` cleanups —
// a correctly-matched ES5 cleanup stays silent, while a real capture-flag mismatch
// (the react-scroll-to-bottom@4.2.0 bug) is caught for the right reason. Run:
// python frontend/ownts/ownts.py frontend/ownts/examples/EffectFunctionCallback.tsx --check
// Expect exactly ONE OWN001 (the capture-mismatched focus listener).
import { useEffect } from "react";

export function Composer({ target }: { target: HTMLElement }) {
// Correctly cleaned ES5 effect: same handler, same (default) capture -> silent.
useEffect(function () {
window.addEventListener("resize", onResize);
return function () {
window.removeEventListener("resize", onResize);
};
}, []);

// Real leak: added with { capture: true } but removed with the default (false)
// capture, so the listener is never actually removed (react-scroll-to-bottom bug).
useEffect(function () {
target.addEventListener("focus", handleFocus, { capture: true, passive: true });
return function () {
return target.removeEventListener("focus", handleFocus);
};
}, [target]);

return <div>composer</div>;
}
19 changes: 18 additions & 1 deletion frontend/ownts/examples/EffectLeakControl.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,7 +2,7 @@
// each uses a release-shaped cleanup that does NOT actually release THIS resource,
// so it must STILL report a leak. Run:
// python frontend/ownts/ownts.py frontend/ownts/examples/EffectLeakControl.tsx --check
// Expect exactly three OWN001 findings.
// Expect exactly four OWN001 findings.
import { useEffect } from "react";

export function LeakControl({ enabled }: { enabled: boolean }) {
Expand All@@ -29,5 +29,22 @@ export function LeakControl({ enabled }: { enabled: boolean }) {
if (enabled) return () => clearInterval(id);
}, [enabled]);

// (4) async effect: React receives a Promise and never runs the returned
// function as cleanup, so the listener leaks despite the release-shaped return.
useEffect(async () => {
window.addEventListener("message", onMessage);
return () => window.removeEventListener("message", onMessage);
}, []);

// (5) async ES5 effect: same async suppression, but the transpiled
// `async function () { … }` shape — React still ignores the returned
// `function () {}` cleanup, so the listener leaks.
useEffect(async function () {
window.addEventListener("online", onOnline);
return function () {
window.removeEventListener("online", onOnline);
};
}, []);

return <div>leak control</div>;
}
46 changes: 34 additions & 12 deletions frontend/ownts/ownts.py
Original file line numberDiff line numberDiff line change
Expand Up@@ -412,19 +412,30 @@ def _expr_end(text: str, i: int) -> int:
def _effect_callback(masked: str, after_open: int) -> tuple[str, int, list[str] | None, int]:
"""Parse `useEffect(<cb>, [deps])` from just past the `(`, over the STRING-MASKED
source (so literals never truncate a body or split a dep). Returns
(body, body_start, deps, end). Handles BOTH a block `() => { ... }` and an
expression `() => fetch(url)` callback — calling `_match_block` blindly on the
latter would jump to an unrelated `{` or run off the end. `deps` is None when no
(body, body_start, deps, end). Handles a block arrow `() => { ... }`, an
expression arrow `() => fetch(url)`, AND a `function () { ... }` expression
(transpiled ES5 output) — calling `_match_block` blindly, or keying on `=>`,
would jump to an unrelated `{`/arrow or run off the end. `deps` is None when no
dependency array is present; the array is matched by BALANCED brackets so a dep
like `items[i]` survives. `body` includes the braces for a block callback."""
arrow = masked.find("=>", after_open)
i = (arrow + 2) if arrow != -1 else after_open
while i < len(masked) and masked[i] in " \t\r\n":
i += 1
if i < len(masked) and masked[i] == "{":
end = _match_block(masked, i)
j0 = after_open
while j0 < len(masked) and masked[j0] in " \t\r\n":
j0 += 1
if re.match(r"(?:async\s+)?function\b", masked[j0:]):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not credit cleanups from async effect callbacks

When the first argument is an async function expression, React receives a Promise from the effect and will not run the inner returned function as the cleanup. With this branch, useEffect(async function () { window.addEventListener(...); return function () { window.removeEventListener(...); }; }, []) is parsed like a normal synchronous effect, so _cleanup_span marks the listener released and the leak is missed in JS/transpiled inputs that are not type-checked.

Useful? React with 👍 / 👎.

# function-expression callback: the body `{` follows the parameter list.
i = _body_brace(masked, after_open)
end = _match_block(masked, i) if i != -1 else len(masked)
if i == -1:
i = after_open
else:
end = _expr_end(masked, i)
arrow = masked.find("=>", after_open)
i = (arrow + 2) if arrow != -1 else after_open
while i < len(masked) and masked[i] in " \t\r\n":
i += 1
if i < len(masked) and masked[i] == "{":
end = _match_block(masked, i)
else:
end = _expr_end(masked, i)
body = masked[i:end]
# the dependency array is the balanced `[ ... ]` after an optional `, `
deps: list[str] | None = None
Expand DownExpand Up@@ -462,7 +473,8 @@ def _cleanup_span(mbody: str) -> tuple[int, int, int] | None:
fn_braces.add(fm.end() - 1)
for fm in re.finditer(r"\bfunction\b[^{;]*?\)\s*\{", mbody):
fn_braces.add(fm.end() - 1)
ret_re = re.compile(r"return\s*(?:\(\s*\)|\w+)\s*=>")
# a cleanup is `return <arrow>` or `return <function expression>` (ES5 output).
ret_re = re.compile(r"return\s*(?:(?:\(\s*\)|\w+)\s*=>|(?:async\s+)?function\b)")
stack: list[tuple[bool, int]] = [] # (is_function_body, open_index) per open brace
fdepth = 0
i, n = 0, len(mbody)
Expand All@@ -487,6 +499,12 @@ def _cleanup_span(mbody: str) -> tuple[int, int, int] | None:
mbody.rfind("}", 0, i))
if re.search(r"\b(?:if|else|for|while)\b", mbody[b + 1:i]):
cov_start = m.start() # braceless conditional guard -> covers nothing
if "function" in m.group(): # `return function () { ... }` (ES5 cleanup)
brace = _body_brace(mbody, m.end()) # body brace after the params
if brace != -1:
return (m.start(), _match_block(mbody, brace), cov_start)
i += 1
continue
rest = mbody[m.end():]
stripped = rest.lstrip()
if stripped.startswith("{"): # block-bodied cleanup: `=> { ... }`
Expand DownExpand Up@@ -518,7 +536,11 @@ def extract(path: str) -> list[Component]:
for eff in _USE_EFFECT.finditer(masked):
body_m, body_start, _deps, end = _effect_callback(masked, eff.end())
body_o = text[body_start:end] # original (unmasked) body — same positions
span = _cleanup_span(body_m)
# An ASYNC effect callback returns a Promise, so React never runs its
# returned function as cleanup — credit no cleanup for it (a `return () => …`
# inside `useEffect(async () => …)` is dead, so the resource still leaks).
is_async = bool(re.match(r"\s*async\b", masked[eff.end():]))
span = None if is_async else _cleanup_span(body_m)
if span:
cs, ce, cov_start = span
setup_m = body_m[:cs] + body_m[ce:]
Expand Down
14 changes: 11 additions & 3 deletions frontend/ownts/test_ownts.py
Original file line numberDiff line numberDiff line change
Expand Up@@ -65,10 +65,18 @@ def main() -> int:

# False-negative controls: a release-shaped cleanup that does NOT release THIS
# resource (wrong AbortController, mismatched unsubscribe args, a conditionally
# returned cleanup over an unconditional acquire) must STILL report the leak —
# the broadened matchers must not over-suppress.
# returned cleanup over an unconditional acquire, an async arrow effect, and an
# async ES5 `function` effect) must STILL report the leak — the broadened
# matchers and async suppression must not over-suppress.
leaks = codes("EffectLeakControl.tsx")
assert leaks == ["OWN001", "OWN001", "OWN001"], f"EffectLeakControl -> {leaks}"
assert leaks == ["OWN001"] * 5, f"EffectLeakControl -> {leaks}"

# Transpiled-ES5 shape: `function () { … return function () { … } }`. The parser
# handles `function` callbacks + `return function` cleanups — a matched cleanup is
# silent, and a real capture-flag mismatch (the react-scroll-to-bottom@4.2.0 bug)
# is caught precisely via the listener key.
fn_cb = codes("EffectFunctionCallback.tsx")
assert fn_cb == ["OWN001"], f"EffectFunctionCallback -> {fn_cb}"

# an expression-bodied cleanup whose removeEventListener carries an options
# object must parse (the `{` belongs to the call, not the cleanup block) — the
Expand Down
Loading