docs: add LOGGING.md describing the logging philosophy

docs/LOGGING.md

@@ -0,0 +1,276 @@
+# Logging philosophy
+
+This document explains how `go-polyscript` handles diagnostic logging — what
+the library guarantees, what hosts can rely on, and why those decisions were
+made. It also lays the choices alongside what other modern Go libraries do,
+so you can see they're not idiosyncratic.
+
+The short version: **a host that does nothing gets diagnostic logs through
+`slog.Default()`. A host that wires its own `slog.Handler` gets that handler
+honored at every layer. The library never writes to `os.Stdout`/`os.Stderr`
+on its own.**
+
+---
+
+## Principles
+
+### 1. Logging is opt-in, never required
+
+No constructor demands a `slog.Handler`. Every public option that takes one
+also accepts nil:
+
+- `polyscript.WithLogHandler[E](nil)` — top-level, generic
+- `engines/{risor,starlark,extism}.WithLogHandler(nil)` — per-engine
+- `engines/{risor,starlark,extism}/compiler.WithLogHandler(nil)` — compiler-level
+- `engines/{risor,starlark,extism}/compiler.WithLogger(nil)` — compiler-level
+
+A host that supplies no handler is treated identically to a host that
+supplies `nil` — no error, no panic, the engine inherits from
+`slog.Default()`.
+
+Two layers behave slightly differently when the option is called more than
+once with mixed values:
+
+- The **compiler-level** options (`compiler.WithLogHandler`,
+  `compiler.WithLogger`) are **true no-ops** on nil: they skip the set
+  *and* the clear-of-the-other-field, so `WithLogHandler(real)` followed by
+  `WithLogHandler(nil)` leaves the real handler in place. This was
+  established in #111 / PR #113.
+- The **top-level and per-engine** `WithLogHandler` options assign the
+  handler field directly (`c.handler = h`). They accept nil, but if a
+  caller passes nil after a non-nil value in the same option list, the
+  earlier value is replaced. In practice this isn't a concern — these
+  options are rarely passed twice — and the field's zero value (nil) is
+  the same shape, so nothing observable changes for the common case.
+
+### 2. nil means "inherit from `slog.Default()`"
+
+Every internal construction site routes through one canonical helper:
+
+```go
+// internal/helpers/logger.go
+func SetupLogger(handler slog.Handler, engineName, groupName string) (slog.Handler, *slog.Logger)
+```
+
+When `handler` is nil, `SetupLogger` calls `slog.Default().Handler()` and
+wraps it with `engineName` as a `slog.Group`. The returned logger is then
+wrapped with `groupName` as a nested group. So a record from the Risor
+evaluator carries the group prefix `risor.Evaluator.<…>` — the engine name
+is added because the host supplied no handler.
+
+When `handler` is non-nil, `SetupLogger` returns it unchanged and only
+wraps the *logger* with `groupName`. The host's handler keeps whatever
+grouping the host configured; the library doesn't prepend its engine name.
+A record from the Risor evaluator under a host-supplied handler carries
+just the `Evaluator.<…>` group — the host already knows it's their handler
+and presumably has their own taxonomy.
+
+The asymmetry is deliberate: when the library is using its own (default)
+sink, prefixing with the engine name disambiguates "where did this record
+come from?" When the host owns the handler, the host owns the layout.
+
+A host can call `slog.SetDefault(...)` once at startup and have every
+engine inherit it — no per-engine wiring, no per-call argument passing.
+
+### 3. There is exactly one fallback path
+
+Every optional-handler path in the package routes through `helpers.SetupLogger`.
+There are no `slog.NewTextHandler(os.Stdout, …)` fallbacks. There are no
+`if handler == nil { handler = … }` branches scattered through constructors.
+There is no "if you forgot to wire a handler we'll print to stderr to be
+helpful" behavior.
+
+Two consequences:
+
+- A misconfigured host never has the library writing to stdout/stderr behind
+  its back, polluting structured-log pipelines.
+- Changing the default-handler behavior is a one-line change in `SetupLogger`,
+  not a migration across every engine constructor.
+
+The four loader-internal `slog.Default().Debug(...)` cleanup-error logs in
+`platform/script/loader/{fromHTTP,fromDisk}.go` are the one apparent
+exception — they live in code paths that don't carry a logger. They still
+inherit from `slog.Default()` (same fallback target as `SetupLogger`), so
+the principle holds.
+
+### 4. Functional options are silent on nil
+
+This is a Go idiom: passing `nil` to an optional functional option is
+equivalent to not calling it at all. Erroring on nil violates the contract
+and forces every call site into a defensive `if x != nil { … opt(x) … }`
+wrap.
+
+We held the wrong contract for a while — the per-engine
+`compiler.WithLogHandler(nil)` and `compiler.WithLogger(nil)` returned
+`fmt.Errorf("log handler cannot be nil")`. That decision propagated outward:
+six call sites in `polyscript.go` and `engines/{risor,starlark,extism}/new.go`
+gained a `cfg.handler != nil` guard solely to avoid tripping the error
+during normal construction. PR #113 (issue #111) fixed the option layer; PR
+#114 (issue #112) removed the now-redundant guards.
+
+### 5. Group names follow `(engine, struct)` shape
+
+Every `SetupLogger` call uses `(engineName, structName)` form:
+
+```go
+helpers.SetupLogger(handler, "extism", "Compiler")
+helpers.SetupLogger(handler, "extism", "Evaluator")
+helpers.SetupLogger(handler, "extism", "execResult")
+helpers.SetupLogger(handler, "script", "ExecutableUnit")
+```
+
+`slog.Handler.WithGroup` prepends a group qualifier to every attribute key
+that follows. With a `slog.JSONHandler`, the records nest under that group
+key; with a `slog.TextHandler`, attribute keys carry the group prefix
+(e.g. `extism.Evaluator.scriptID=…`).
+
+The exact prefix depends on whether the host supplied a handler (see
+principle 2):
+
+- **Nil handler**: prefix is `<engine>.<struct>.` — `extism.Evaluator.`,
+  `script.ExecutableUnit.`. The engine name is the library's own
+  contribution; the struct name disambiguates between the compiler's logs
+  and the evaluator's.
+- **Host-supplied handler**: prefix is `<struct>.` only — `Evaluator.`,
+  `ExecutableUnit.`. The host owns whatever outer grouping is in place.
+
+In either case the `<struct>` group is always present, so a host filtering
+by group `Evaluator` reaches the per-evaluator records regardless of which
+mode is active.
+
+---
+
+## How this aligns with prior art
+
+The choices above aren't novel — they line up with conventions that are by
+now mainstream in modern Go.
+
+### Standard library `log/slog`
+
+`slog.NewTextHandler(w, nil)` accepts a nil `*HandlerOptions` and falls back
+to defaults. The pattern is: **constructors accept nil with sensible
+defaults; only fully-built logger objects panic on nil** (e.g. `slog.New(nil)`
+panics, but that's an already-built thing — different layer).
+
+`go-polyscript` mirrors this at the option layer: passing nil never panics,
+never errors, falls back to a sensible default.
+
+`slog.Default()` is the canonical hook for "let the host configure logging
+once, library code inherits." We use it as our nil-handler target for
+exactly that reason.
+
+### `log/slog` discard handler
+
+Go 1.24 added `slog.DiscardHandler` (issue golang/go#62005) so libraries
+have a clean "log nothing" sink without instantiating a handler against
+`io.Discard`. We don't use it as a default — `slog.Default()` is more
+useful, since the host can swap it. But the same philosophy applies:
+**nil-checks in business logic are a smell; have a sentinel sink instead.**
+
+### OpenTelemetry `otelslog`
+
+`otelslog.WithLoggerProvider` defaults to the global `LoggerProvider` when
+the option is omitted. Absence == default. Same shape as our
+`WithLogHandler`: omit it, and the global default takes over.
+
+### `go-logr/logr`
+
+The `logr` ecosystem documents `logr.Discard()` so callers never need to
+nil-check a logger field. The principle: **nil logger should not be a
+foot-gun.** Pre-#85 this library had nil-handlers writing to `os.Stdout`;
+post-#85/#99 nil routes through `slog.Default()`. Different mechanism, same
+intent.
+
+### Functional options pattern (Cheney/Pike lineage)
+
+Dave Cheney's foundational write-ups on functional options consistently
+treat the option as **optional**: calling `Server(WithTimeout(...))` opts
+into a non-default value; calling `Server()` accepts the default. Erroring
+on `WithTimeout(0)` would be a contract violation. Same logic applies to
+our `WithLogHandler(nil)` — it's the option layer's job to accept any value
+the type allows; the constructor's defaults take over when the value isn't
+informative.
+
+### Stdlib `database/sql`
+
+`sql.Open(driver, dsn)` doesn't take a logger. Drivers that want to log
+configure it via their own setters or rely on `log.Default()` /
+`slog.Default()`. The pattern is: **the host owns the logging surface; the
+library doesn't fight it.**
+
+We follow the same pattern with `slog.Default()` as the inheritance point.
+
+---
+
+## What we explicitly don't do
+
+These are choices that look reasonable on paper but have specific failure
+modes we avoided:
+
+- **No `os.Stdout` / `os.Stderr` fallback writes.** Pre-#99 several
+  evaluator constructors did `slog.NewTextHandler(os.Stdout, nil)` when
+  given nil. That's hostile to a host running under systemd-journal,
+  Docker, or anything that captures stdout for output rather than logs. The
+  library has no business deciding where logs go.
+
+- **No "Handler is nil, using default" warning at construction.** Same
+  evaluators emitted a one-time `Warn` log when falling back. That's noise
+  the host didn't ask for, especially during tests where nil is the
+  intentional choice.
+
+- **No panic on nil at the option layer.** `slog.New(nil)` panics, and
+  that's correct for *constructors*, but functional options aren't
+  constructors — they're configuration knobs, and a nil knob is "use the
+  default."
+
+- **No mandatory handler argument on `polyscript.New`.** The host's
+  ergonomic baseline is `polyscript.New[Risor](polyscript.FromString(src))`
+  — nothing else. Adding a required handler argument would force every
+  caller to either write `slog.Default()` explicitly (boilerplate) or pass
+  nil (which we'd then have to handle anyway).
+
+- **No "library defines its own logger interface."** `logr` predates
+  `slog`; we don't. There's no value in re-abstracting `slog.Handler`
+  behind a `Logger` interface for our use case.
+
+---
+
+## Architectural map
+
+If you're reading the source, the chain is:
+
+```
+host code
+  └─ polyscript.New[E](src, polyscript.WithLogHandler[E](h))
+       └─ engines/<engine>.FromXxxLoader(ldr, <engine>.WithLogHandler(h))
+            ├─ engines/<engine>/compiler.New(compiler.WithLogHandler(h))
+            │    └─ helpers.SetupLogger(h, "<engine>", "Compiler")
+            └─ engines/<engine>/evaluator.New(h, execUnit)
+                 └─ helpers.SetupLogger(h, "<engine>", "Evaluator")
+                      and execResult uses "<engine>.execResult"
+
+platform/script.NewExecutableUnit(h, ...) — separately:
+  └─ helpers.SetupLogger(h, "script", "ExecutableUnit")
+```
+
+Every leaf is `helpers.SetupLogger`. Every layer accepts nil. Every layer's
+nil-path inherits from `slog.Default()`.
+
+---
+
+## How we got here (history)
+
+Four PRs (filed against issues #85, #99, #111, #112) converged the package
+on the philosophy above:
+
+| Issue | PR | What changed |
+|---|---|---|
+| #85 | #105 | Made `slog.Handler` optional in engine subpackages; introduced `helpers.SetupLogger` as the canonical fallback. |
+| #99 | #110 | Replaced the last three `slog.NewTextHandler(os.Stdout, nil)` fallbacks (in `engines/*/evaluator/response.go`) with `SetupLogger`. Dropped three stale logging-related TODOs. |
+| #111 | #113 | Relaxed `WithLogHandler(nil)` and `WithLogger(nil)` across the three engines from "errors on nil" to true no-op (skip set, skip clear-of-the-other-field). |
+| #112 | #114 | Dropped the six `cfg.handler != nil` guards in `polyscript.go` and `engines/*/new.go` that existed solely to work around the pre-#111 nil-rejection. Tightened the top-level `WithLogHandler` godoc. |
+
+If you're considering adding a new construction site that takes an
+`slog.Handler`, the rule is simple: **call `helpers.SetupLogger` with
+`(engineName, structName)`. Don't write a nil-check. Don't construct
+fallback handlers. Don't panic.**