Skip to content

feat: add OTP logger handler bindings + fix latent bare-ok Result bug - #108

Merged
dbrattli merged 4 commits into
mainfrom
feat/logger-handler-bindings
Jun 7, 2026
Merged

feat: add OTP logger handler bindings + fix latent bare-ok Result bug#108
dbrattli merged 4 commits into
mainfrom
feat/logger-handler-bindings

Conversation

@dbrattli

@dbrattli dbrattli commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

What

Started as: add the missing OTP logger handler-management bindings so downstream apps don't fall back to raw emitErlExpr. While doing it we found — and fixed — a latent runtime bug in the existing Supervisor bindings, and documented the underlying gotcha in the bindings guide.

1. New logger handler bindings (src/otp/Logger.fs)

Faithful 1:1 bindings for the handler-management functions (OTP docs):

  • add_handler/3, remove_handler/1, get_handler_config/1, set_handler_config/2, set_handler_config/3

Design:

  • Typed returns, not obj. ok | {error, term()}Result<unit, Dynamic>; get_handler_configResult<BeamMap<Atom, obj>, Dynamic>. Matches the house convention (File/Supervisor).
  • Typed config, not obj. Config params are BeamMap<Atom, obj> (the same typed map already used for log metadata in this file). value stays obj since handler-config values are genuinely heterogeneous.
  • Fixed update_handler_config's latent bug: it returned unit, silently swallowing {error, _} (e.g. when OTP rejects changing a logger_std_h handler's type at runtime). Now Result<unit, Dynamic>.
  • Thin and unopinionated — no logging-policy helpers.

2. Fix: latent bare-ok bug in Supervisor (src/otp/Supervisor.fs)

supervisor:terminate_child/2 and delete_child/2 were typed Result<unit, Atom> but bind to OTP functions that return a bare ok atom. Fable encodes Ok () as the tuple {ok, ok}, so a successful call matched neither Result branch — every success was silently wrong at runtime. (No test covered the ok path; the existing test only hit which_children on a missing supervisor.) Both now use an [<Emit>] case wrapper to bridge ok -> {ok, ok}, the same pattern File/Logger use.

3. Verification

  • New test/test_basic_sup.erl — a one_for_one supervisor with a single transient child, wired into the just test pipeline (justfile).
  • test/TestSupervisor.fs — 4 new tests covering both the Ok () and Error not_found paths of terminate_child and delete_child (the success path is the one that exposes a missing wrapper).
  • test/TestLogger.fs — round-trips a handler through add_handler/3 + remove_handler/1, asserting Ok () (only passes if the bare-ok bridge is correct on the BEAM).

Confirmed against OTP directly that a temporary child's spec is auto-removed on termination — hence the transient child so delete_child can succeed after terminate_child.

just build clean (0 warnings) · just test356/356 passing.

Downstream apps can now write, with no emitErlExpr:

logger.remove_handler (Erlang.binaryToAtom "default") |> ignore
logger.add_handler (Erlang.binaryToAtom "default", Erlang.binaryToAtom "logger_std_h", someConfigMap) |> ignore

4. Docs (BINDINGS-GUIDE.md)

Documented the bare-ok trap as a new anti-pattern: a plain Result<unit, _> binding over a bare-ok OTP function compiles cleanly but is silently wrong at runtime, because Fable encodes Ok () as {ok, ok} and the error path masks the bug. Includes the fix, the carve-out (no wrapper needed when OTP already returns {ok, V} | {error, R}), and the rule: always test the success path of a bare-ok binding.

Commits

  1. feat: add OTP logger handler-management bindings
  2. refactor: type logger handler bindings with Result and BeamMap
  3. fix: bridge bare ok in supervisor terminate_child/delete_child
  4. docs: warn about Result<unit,_> over bare-ok OTP functions

🤖 Generated with Claude Code

dbrattli and others added 2 commits June 7, 2026 10:03
Add faithful 1:1 bindings for logger:add_handler/3, remove_handler/1,
get_handler_config/1, and set_handler_config/2,3 to Fable.Beam.Logger,
so downstream apps no longer need to fall back to raw emitErlExpr for
handler management.

All return the raw `ok | {error, term()}` Erlang term rather than
swallowing it. Also fix update_handler_config to return obj instead of
unit for the same reason (no in-repo callers, so non-breaking here).

Add a TestLogger test that round-trips a handler through add_handler/3
and remove_handler/1 on the BEAM.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the raw obj returns/params from the first pass with the house
conventions:

- Returns: ok | {error, term()} now maps to Result<unit, Dynamic>
  (matching Supervisor/File). add/remove/set/update_handler_config use
  an [<Emit>] case-wrapper to bridge OTP's bare `ok` into Fable's
  {ok, ok} Result representation, the same pattern Ets.fs/File.fs use.
- get_handler_config returns Result<BeamMap<Atom, obj>, Dynamic>; OTP
  already returns {ok, Config} | {error, _}, which is Fable's Result
  shape, so no wrapper is needed.
- config params are now BeamMap<Atom, obj> (the typed map already used
  for log metadata in this file) instead of obj. value stays obj since
  handler-config values are heterogeneous.

Update the round-trip test to assert Ok (), which only passes if the
bare-ok bridge is correct on the BEAM.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dbrattli

dbrattli commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator Author

Updated to address the obj-heavy typing feedback — replaced raw obj returns/params with the existing house conventions:

  • Returns are now Result<_, Dynamic>. ok | {error, term()}Result<unit, Dynamic> (matching Supervisor/File). add_handler/remove_handler/set_handler_config/update_handler_config use an [<Emit>] case wrapper to bridge OTP's bare ok into Fable's {ok, ok} Result representation — the same pattern Ets.fs/File.fs already use. (A plain binding can't, because Fable encodes Ok () as {ok, ok}, not bare ok.)
  • get_handler_configResult<BeamMap<Atom, obj>, Dynamic>. OTP already returns {ok, Config} | {error, _}, which is Fable's Result shape, so no wrapper is needed.
  • config params are now BeamMap<Atom, obj> (the typed map already used for log metadata in this file) instead of obj. value stays obj since handler-config values are heterogeneous.

The round-trip test now asserts Ok () rather than comparing a boxed atom — which only passes if the bare-ok bridge is correct on the BEAM. Transpiled output confirms it:

case logger:add_handler(HandlerId, Modle, Config) of
    ok -> {ok, ok};
    {error, LoggerAddHandlerReason__} -> {error, LoggerAddHandlerReason__}
end

just build clean, just test 352/352.

supervisor:terminate_child/2 and delete_child/2 return a bare `ok` atom,
but the bindings typed them as Result<unit, Atom> — which Fable encodes
as {ok, ok}. A successful call therefore never matched Ok () on the F#
side (a latent bug; no test exercised the ok path). Wrap both in the
same [<Emit>] case-converter used for File/Logger so OTP's `ok` surfaces
as Ok () and {error, Reason} as Error Reason.

Add a test_basic_sup supervisor callback (one transient counter child)
and four tests covering both the Ok () and Error not_found paths of
terminate_child and delete_child.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dbrattli

dbrattli commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed supervisor:terminate_child/2 and delete_child/2 the same way (commit 9f58118).

Both return a bare ok atom in OTP, but were typed Result<unit, Atom> — which Fable encodes as {ok, ok}. So a successful terminate_child/delete_child never matched Ok () on the F# side. This was a latent bug: no test exercised the ok path (the only existing test hit which_children on a missing supervisor). Both now use the same [<Emit>] case-converter as File/Logger.

Added verification:

  • test_basic_sup — a one_for_one supervisor callback module with a single transient counter child (transient, not temporary, so the spec survives terminate_child and can then be delete_child'd).
  • 4 tests covering both branches of each binding:
    • terminate_childOk () for a running child; Error not_found for an unknown id
    • delete_childOk () after the child is terminated; Error not_found for an unknown id

While writing these I confirmed against OTP directly that a temporary child's spec is auto-removed on termination (so delete_child would return not_found) — hence the transient child. The binding was correct; only the test's restart-type assumption needed fixing.

just test356/356.

Document the latent bug fixed in this PR: a plain Result<unit, _> binding
over an OTP function that returns a bare `ok` atom compiles cleanly but is
silently wrong at runtime, because Fable encodes Ok () as {ok, ok}. The
error path masks it (it's already in Result shape), so only a success-path
test reveals the missing [<Emit>] wrapper.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@dbrattli dbrattli changed the title feat: add OTP logger handler-management bindings feat: add OTP logger handler bindings + fix latent bare-ok Result bug Jun 7, 2026
@dbrattli
dbrattli merged commit e850ad5 into main Jun 7, 2026
10 checks passed
@dbrattli
dbrattli deleted the feat/logger-handler-bindings branch June 7, 2026 08:31
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.

1 participant