Skip to content

Runtime cleanups split out from the type hinting work - #184

Merged
Korijn merged 1 commit into
masterfrom
claude/observ-code-changes-v66ynz
Jul 9, 2026
Merged

Runtime cleanups split out from the type hinting work#184
Korijn merged 1 commit into
masterfrom
claude/observ-code-changes-v66ynz

Conversation

@Korijn

@Korijn Korijn commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Part 1 of 2 for #114: the behavioral/code changes, split out from the pure typing changes (which stack on top of this PR) so each can be reviewed in isolation.

Changes

  • watch_effect is a real function instead of a functools.partial, so it has an inspectable signature, a docstring, and mkdocstrings rendering in the API reference (the hand-written docs section is replaced by the generated one).
  • readonly/shallow_reactive/shallow_readonly are real functions instead of partials, for the same reason (partials also type-check poorly, which matters for part 2).
  • ref() has a single implementation: the generic Ref TypedDict is typing-only (it requires Python 3.11 at runtime, hence the old try/except definition dance), and at runtime a Ref was always just a plain (proxied) dict anyway.
  • init(mode="rendercanvas") without a loop raises a clear TypeError instead of failing inside register_rendercanvas.
  • loop_factory() looks up asyncio.eager_task_factory once with getattr instead of hasattr + attribute access.

No public API changes beyond the partial→function swaps (call sites are unaffected; keyword overrides that the partials accidentally allowed, like watch_effect(fn, immediate=True), are no longer possible).

Note on _run_callback: an earlier version of this PR also "fixed" a reference-cycle leak in the traceback-introspection code there. On review, that leak didn't exist in the original code — it relied on Python's implicit del e at the end of except TypeError as e:, which already prevents the frame↔traceback cycle. My rewrite introduced the leak itself by binding e.__traceback__ to its own local (not covered by that implicit cleanup). _run_callback is therefore left untouched here; part 2 handles the one line that needs a type-checker accommodation, via a narrow ignore comment rather than a control-flow change.

🤖 Generated with Claude Code

https://claude.ai/code/session_016fDq6fyKg6QscqyyN6CcwC

Behavioral/code changes separated from the pure typing changes so
each can be reviewed in isolation:

* watch_effect is a real function instead of a functools.partial, so
  it has an inspectable signature, a docstring and mkdocstrings
  rendering in the API reference (the hand-written docs section is
  replaced by the generated one).
* readonly/shallow_reactive/shallow_readonly are real functions
  instead of partials, for the same reason.
* ref() no longer needs the try/except TypedDict definition dance:
  the generic Ref TypedDict is typing-only (it requires Python 3.11
  at runtime), and all Python versions share one implementation. At
  runtime a Ref was always just a plain (proxied) dict.
* init(mode="rendercanvas") without a loop raises a clear TypeError.
* loop_factory() looks up asyncio.eager_task_factory once with
  getattr instead of hasattr + attribute access.

_run_callback's traceback introspection is deliberately left
untouched: it relies on Python's implicit `del e` at the end of
`except TypeError as e:` to avoid a frame<->traceback reference
cycle (e.__traceback__.tb_frame is this very frame). Binding
e.__traceback__ to its own local for a cleaner is-not-None check
would reintroduce that cycle by hand, requiring a fragile manual
del to compensate for a safety property the language already
guarantees for free. That tradeoff isn't worth it here; see the
typing PR for how the (purely cosmetic, type-checker-driven) change
there is instead scoped to a narrow, targeted ignore comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016fDq6fyKg6QscqyyN6CcwC
@Korijn
Korijn force-pushed the claude/observ-code-changes-v66ynz branch from e493554 to 2fa309c Compare July 9, 2026 20:58

Korijn commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

A mistake I made while preparing this PR, and how it was caught

Leaving this here for the record rather than quietly editing history, since the diff you're reviewing was shaped by a real error and a useful correction.

What I did wrong

While doing the type-hinting work in #185, I annotated Watcher._run_callback, whose traceback-introspection code originally looked like this (this is master, unchanged):

try:
    _run_callback_is_top_frame = (
        e.__traceback__.tb_frame.f_code == self._run_callback.__code__
    )
    _no_lower_frames = e.__traceback__.tb_next is None
    wrong_number_of_arguments = _run_callback_is_top_frame and _no_lower_frames
except AttributeError:
    pass

e.__traceback__ is typed TracebackType | None, and a type checker can't tell that the except AttributeError branch already handles the None case — try/except isn't a narrowing construct. So I "cleaned it up" into an explicit check:

tb = e.__traceback__
if tb is not None:
    _run_callback_is_top_frame = tb.tb_frame.f_code == self._run_callback.__code__
    _no_lower_frames = tb.tb_next is None
    wrong_number_of_arguments = _run_callback_is_top_frame and _no_lower_frames

This introduced a real bug: tb.tb_frame is the current frame (the traceback you get from e.__traceback__ inside the frame that caught the exception always points back at that frame). Binding it to a local (tb) creates a frame↔traceback reference cycle, which — since that frame also holds self (the Watcher) as a local — keeps the watcher alive until the next incidental gc pass instead of being reclaimed immediately by refcounting. Two tests that assert immediate weakref-based cleanup after del watcher failed.

I "fixed" this by adding del tb right after use, and described it in my commit message and PR body as a genuine bug fix — a functools.partial-adjacent, pre-existing correctness issue that this PR was cleaning up. That framing was wrong. The bug did not exist before my rewrite. The original code never binds e.__traceback__ to a persistent local — it only reaches it through e, and Python automatically deletes e at the end of except ... as e: for exactly this reason (that's documented CPython behavior, specifically to prevent this class of cycle). My rewrite discarded that free, language-guaranteed protection and replaced it with a hand-maintained del that a future maintainer could easily forget, misplace, or bypass (e.g. by adding an early return, or moving code around) without any test failing until someone happened to check object lifetimes without calling gc.collect() first.

How it was caught

Two rounds of pushback from the user:

  1. "I don't see how deleting the traceback object changes anything... wouldn't the local just go out of scope naturally?" — a fair challenge to verify the mechanism was even real, rather than accept my explanation at face value. I confirmed the cycle with a minimal repro (gc.disable(), weakref, check collection before/after gc.collect()) — the cycle is real, but this prompted me to look more carefully at why the original code didn't have it.
  2. "The new code is more verbose and apparently requires explicit extra code to prevent a ref cycle, while the original did not... it doesn't feel like the higher complexity is justified by typing." — this is the observation that actually mattered. It was right: the added fragility (a manually-placed del, easy to break silently) wasn't a good trade for satisfying a type checker, especially once it became clear the "bug" was self-inflicted rather than pre-existing.

Where things landed

Net result: no behavior change here, no hand-maintained cycle-breaking anywhere, and the type-checker accommodation in #185 is scoped to exactly the two lines that need it, instead of a control-flow rewrite that traded one footgun for another.

🤖 Generated with Claude Code


Generated by Claude Code

@Korijn

Korijn commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

I made fable post the above message to expose how ******* stubborn and confident these models are, and how quickly we can be charmed by their "fame" on the internet as the holy grail of all LLMs. Stay sharp people :)

@Korijn
Korijn merged commit be329be into master Jul 9, 2026
10 checks passed
@Korijn
Korijn deleted the claude/observ-code-changes-v66ynz branch July 9, 2026 21:03
berendkleinhaneveld added a commit that referenced this pull request Jul 10, 2026
First stable release. Since v0.19.0, observ has had a broad performance
overhaul, gained full type-hint coverage and a new trigger_ref API, and
picked up a documentation site.

New features
- trigger_ref(): force-notify the watchers of a proxy, mirroring Vue's
  triggerRef (#188, closes #123).
- Fully typed: modern type hints throughout, a py.typed marker, and ty
  type-checking enforced in CI (#185, #184, closes #114).

Performance
- Reworked deep-watch traversal: plain leaf values are filtered out of the
  traverse stack at push time and raw targets are traversed directly.
  Traversing large flat/shallow structures is up to ~85% faster (#190).
- Lower per-operation trap overhead and proxies constructed with positional
  flags: reactive reads/writes and proxy creation are ~35-50% faster
  (#192, #193).
- proxy_db lifetimes are managed by reference counting instead of a gc hook,
  eliminating reference cycles and making cleanup deterministic (#177).
- Scheduler and dependency-bookkeeping micro-optimizations: in-place bisect
  insertion (#191), single-read/write flush counting (#196), a set
  difference in cleanup_deps (#194), and reading the arg count off the code
  object in weak() (#195).

Correctness
- Guarantees, with tests, that observ creates no reference cycles (#189).

Documentation
- New MkDocs site published to GitHub Pages (#176), with an Internals
  section (#179) and a rewritten README (#180).

Tooling
- Modernized CI workflows (#182) and a more robust benchmark guard
  (#181, #187).
Korijn added a commit to fork-tongue/patchdiff that referenced this pull request Jul 10, 2026
Closes the typing item of the 1.0.0 roadmap (Track C), following
fork-tongue/observ#184/#185.

Typing:
- Operation TypedDicts in types.py (AddOperation, RemoveOperation,
  ReplaceOperation and the Operation union), used across diff, apply,
  serialize and produce signatures: diff() and produce() now advertise
  tuple[..., list[Operation], list[Operation]]
- PEP 604 unions and builtin generics everywhere via from __future__
  import annotations (the runtime floor stays Python 3.9; typing-only
  imports live in a TYPE_CHECKING block)
- py.typed marker shipped in the wheel, Typing :: Typed classifier
- Genuine annotation bugs fixed: Pointer.evaluate returned
  tuple[Diffable, ...] but parent is None at the root;
  PatchRecorder.record_add annotated reverse_path: Pointer with a None
  default
- Duck-typed code kept honest instead of over-narrowed: the iapply
  interpreter and the diff() dispatch unpack into Any/cast at the
  boundary, with comments explaining why

Toolchain:
- ty in a dedicated dependency group (and in dev), [tool.ty] config
  scoped to patchdiff/ with python-version = "3.9" so 3.9-incompatible
  typing constructs are caught
- Typecheck job in CI, added to publish's needs

Runtime changes (annotation-driven, behavior-preserving):
- _Proxy._detach makes its caller-guarded invariant explicit with an
  assert
- pyproject metadata fix: description was literally "MIT"; now a real
  description plus license = "MIT"

ty check: 26 diagnostics on the previous code, 0 after. Tests pass at
100% coverage on 3.10-3.14 locally; docs build stays warning-free with
the new signatures.


Claude-Session: https://claude.ai/code/session_016Mu9vEBwU4fQLi8ZgkgdS2

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants