fix(ownts): не принимать options-объект за блок cleanup (false-positive OWN001)#148
Conversation
…deRabbit) Follow-up to the merged #145. For an EXPRESSION-bodied effect cleanup whose removeEventListener passes an options object — return () => el.removeEventListener("x", h, {capture: true}) _cleanup_span did `find("{")`, which landed on the `{` of `{capture: true}`, so _match_block ended the span at the options `}` and truncated the trailing `)`. The resulting cleanup no longer parsed via _LISTENER, so _listener_call returned None and a correctly-released listener was reported as a false OWN001 leak. Now a cleanup is treated as block-bodied only when the first non-whitespace character after `=>` is `{`; otherwise it is an expression body ending at the line break (which keeps the whole call, options object and closing `)` included). New fixture EffectExprCleanup.tsx (expect zero findings), pinned in test_ownts.py and a CI step. Full suite green (effects 31/31, ownir 194/194); ruff + mypy --strict clean; all existing fixtures unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR updates OwnTS cleanup-span parsing for expression-bodied ChangesExpression-bodied cleanup parsing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1093-1099: The silent-case check is masking failures by forcing
`ownlang ownir` to succeed even when it crashes, so the zero-findings assertion
can pass on an empty result. Update the regression test in the CI workflow to
call `ownlang ownir` directly and let it fail normally, then assert zero
findings only on a successful run using the `expr` capture and grep check. Keep
the existing `expr`/grep-based check, but remove the `|| true` fallback so
parser/core regressions are not hidden.
In `@frontend/ownts/ownts.py`:
- Around line 398-403: The expression-bodied cleanup parsing in the cleanup
extractor is stopping too early by using the first newline after the arrow,
which truncates formatted multi-line cleanups and breaks _listener_call
detection. Update the cleanup span logic in the expression-bodied branch of the
parser in ownts.py so it keeps consuming the full cleanup expression across line
breaks, including cases like return () => followed by an indented call or
multiline options objects, and only stops when the actual expression ends.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 242c3764-66c0-410c-bc1f-1c00a0f33683
📒 Files selected for processing (4)
.github/workflows/ci.ymlfrontend/ownts/examples/EffectExprCleanup.tsxfrontend/ownts/ownts.pyfrontend/ownts/test_ownts.py
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23c89f2ff8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run: | | ||
| python frontend/ownts/ownts.py frontend/ownts/examples/EffectExprCleanup.tsx \ | ||
| -o "$RUNNER_TEMP/expr.facts.json" | ||
| expr=$(python -m ownlang ownir "$RUNNER_TEMP/expr.facts.json" || true) |
There was a problem hiding this comment.
In the new Expression-bodied cleanup with an options object is silent workflow section, || true hides every non-zero status from python -m ownlang ownir. I checked ownlang/__main__.py: cmd_ownir returns 2 for malformed/drifted OwnIR facts and returns 0 when there are no leaks, so for this expected-silent fixture an OwnIR load/schema error or crash that emits no [OWN...] line would still leave the grep -cE count at 0 and let the regression step pass without validating the generated facts. Preserve/check the exit code instead of unconditionally swallowing it.
Useful? React with 👍 / 👎.
Two follow-ups from review (CodeRabbit + Codex) on the cleanup-span fix:
- Expression-bodied cleanups were cut at the FIRST newline after `=>`, so a
formatted cleanup like
return () =>
el.removeEventListener("resize", onResize);
left an empty cleanup and reintroduced the false OWN001. _cleanup_span now
consumes the whole expression across line breaks, stopping only at a top-level
`;` or the effect body's own closing brace (depth-aware over the masked body).
- The silent-case CI step used `|| true`, which would turn an OwnIR load/schema
crash (rc 2, no findings printed) into a false-green zero-count. Dropped it so a
parser/core regression fails the step; the other steps keep `|| true` because
they legitimately expect findings (rc 1).
EffectExprCleanup.tsx gains a multi-line cleanup case. Full suite green
(effects 31/31, ownir 194/194); ruff + mypy --strict clean; all fixtures
unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PVeuchch67CtjSbizYm8gi
2 participants
Что и зачем
Follow-up к смерженному #145 (#145 уехал в main ровно когда я дорабатывал последнее замечание CodeRabbit). Для expression-bodied cleanup, чей
removeEventListenerпередаёт options-объект ——
_cleanup_spanделалfind("{")и попадал на{объекта{capture: true}:_match_blockобрывал диапазон на}опций и срезал хвостовую). Получившийся cleanup переставал парситься через_LISTENER,_listener_callвозвращалNone, и корректно снятый listener報ался как ложная утечкаOWN001.Теперь cleanup считается block-bodied только если первый непробельный символ после
=>— это{; иначе это выражение, заканчивающееся на переводе строки (целиком сохраняя вызов с options-объектом и закрывающей)).Тип изменения
Как проверено
python tests/run_tests.py— зелёный (effects 31/31,ownir 194/194)ruff check .иmypy(--strict поownlang) — чистоpython frontend/ownts/test_ownts.py(новая фикстураEffectExprCleanup.tsx→ 0 находок)python frontend/ownts/ownts.py frontend/ownts/examples/EffectExprCleanup.tsx --checkСвязанные issue
Follow-up к #145 (OwnTS parser hardening). Closes — нет.
Чеклист
EffectExprCleanup.tsx+ CI-шаг)fix:)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
useEffectcleanups that pass listener options (e.g.,{ capture: true }) are correctly recognized and no longer trigger false leak/instability diagnostics.Tests