fix: relax WithLogHandler(nil)/WithLogger(nil) to no-op across engines#113
Merged
Conversation
Closes #111 Before this change, the compiler-level functional options across all three engines rejected nil with an error: - engines/risor/compiler/options.go WithLogHandler/WithLogger - engines/extism/compiler/options.go WithLogHandler/WithLogger - engines/starlark/compiler/options.go WithLogHandler/WithLogger Meanwhile every other nil-handler path post-#85/#99 (`evaluator.New`, `newEvalResult`, `script.NewExecutableUnit`, the compiler-level `setupLogger`) accepted nil and routed through `helpers.SetupLogger`, which inherits from `slog.Default()`. The asymmetry forced six call sites in `polyscript.go` and `engines/*/new.go` to wrap the option in `if cfg.handler != nil { … }` solely to keep nil from triggering an error during normal construction. Idiomatic precedent argues for nil-tolerance: stdlib `slog.NewTextHandler(w, nil)` accepts nil and falls back to defaults; OpenTelemetry's `otelslog.WithLoggerProvider` defaults to the global provider when the option is omitted; the broader functional-options pattern treats `Option(nil)` as semantically equivalent to not calling the option at all. This change relaxes both `WithLogHandler(nil)` and `WithLogger(nil)` across all three engines to a true no-op (skip both the set AND the clear-of-the-other-field). Calling either with nil is now equivalent to not calling it at all — `setupLogger()` later falls back via `helpers.SetupLogger`. The "true no-op" detail (rather than "set field to nil") matters: `WithLogHandler(real)` followed by `WithLogHandler(nil)` now leaves the real handler in place, which matches caller intuition. The `fmt` import becomes unused in the risor and starlark variants and is dropped; extism keeps it for other options. Tests across 5 files flip 8 "errors on nil" assertions to "no-op on nil + prior state preserved." The "preserves prior set" assertion is the regression guard for the no-clear-of-the-other-field detail. Caller-side simplification of the now-redundant `if cfg.handler != nil` guards in `polyscript.go` and `engines/*/new.go` is tracked separately under #112, to be done after this lands.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes nil-logger/nil-handler behavior across the Extism, Risor, and Starlark compiler functional options so WithLogHandler(nil) / WithLogger(nil) become true no-ops (instead of returning errors), aligning compiler configuration with the rest of the logging setup that already falls back via helpers.SetupLogger.
Changes:
- Relax
WithLogHandler(nil)andWithLogger(nil)in all three engine compiler option packages to returnnil(no-op) rather than an error. - Update/replace unit tests to assert no-op semantics and “prior state preserved” behavior for nil options.
- Update higher-level compiler option handling tests to expect nil options to succeed.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| engines/starlark/compiler/options.go | Make WithLogHandler(nil) / WithLogger(nil) no-ops; update doc comments. |
| engines/starlark/compiler/options_test.go | Adjust option tests to assert no-op + state preservation. |
| engines/starlark/compiler/compiler_test.go | Update option-handling test to expect nil options to succeed. |
| engines/risor/compiler/options.go | Make WithLogHandler(nil) / WithLogger(nil) no-ops; update doc comments. |
| engines/risor/compiler/options_test.go | Adjust option tests to assert no-op + state preservation. |
| engines/risor/compiler/compiler_test.go | Update option-handling test to expect nil options to succeed. |
| engines/extism/compiler/options.go | Make WithLogHandler(nil) / WithLogger(nil) no-ops; update doc comments. |
| engines/extism/compiler/options_test.go | Adjust option tests to assert no-op + state preservation (multiple suites). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+38
to
+39
| // Passing a nil handler is a no-op equivalent to not calling the option: | ||
| // the compiler inherits from slog.Default() via [helpers.SetupLogger]. |
Comment on lines
+38
to
+39
| // Passing a nil handler is a no-op equivalent to not calling the option: | ||
| // the compiler inherits from slog.Default() via [helpers.SetupLogger]. |
Comment on lines
+32
to
+33
| // Passing a nil handler is a no-op equivalent to not calling the option: | ||
| // the compiler inherits from slog.Default() via [helpers.SetupLogger]. |
5 tasks
robbyt
added a commit
that referenced
this pull request
May 8, 2026
…114) Closes #112 Six call sites guarded `WithLogHandler(cfg.handler)` behind a `cfg.handler != nil` check that existed solely to work around the pre-#111 nil-rejection in `engines/*/compiler/options.go`: - polyscript.go:251-253 (newRisor) - polyscript.go:262-264 (newStarlark) - polyscript.go:273-275 (newExtism) - engines/risor/new.go:72-74 - engines/starlark/new.go:72-74 - engines/extism/new.go:85-87 PR #113 (issue #111) made `WithLogHandler(nil)` a true no-op across all three engines, so each guard now produces identical behavior to an unconditional append. Drop them. Also tightens the top-level `polyscript.WithLogHandler` godoc to match the per-engine wording: "A nil handler is permitted and means 'inherit from slog.Default()' — equivalent to omitting the option." The previous "If unset, the underlying engine picks a default" was vague about the nil case. No new tests required; existing construction tests cover both the nil-handler path (`polyscript_options_test.go:198-205` "WithLogHandler accepts nil") and the explicit-handler path. Full test suite + vet green. Co-authored-by: Claude <noreply@anthropic.com>
3 tasks
robbyt
pushed a commit
that referenced
this pull request
May 8, 2026
Three substantive corrections from PR #115 review (Copilot): 1. Principle 1 over-claimed "true no-op on nil" for all four option layers. Only the compiler-level WithLogHandler/WithLogger (relaxed in #111 / PR #113) skip both the set and the clear-of-the-other- field. The top-level and per-engine WithLogHandler do plain field assignment — they accept nil but don't preserve a prior non-nil value across the same option list. Doc now distinguishes the two layers and explains why the distinction is benign for the common single-call case. 2. Principle 2 said records carry the `<engine>.<struct>` prefix "regardless of whether the host configured a handler." That's not true: helpers.SetupLogger only adds the engineName group when the handler is nil. With a host-supplied handler the prefix is just `<struct>`. Doc now describes the asymmetry and explains why it's deliberate (library-owned sink vs. host-owned sink). 3. Principle 5 used `engine=extism` / `script=ExecutableUnit` filter examples that conflate slog groups with key/value attributes. Rephrased to describe slog.Handler.WithGroup behavior accurately (group qualifier on attribute keys; JSON nesting; text prefix) and to condition the `<engine>` prefix on the nil-handler case. The fourth Copilot comment (line 233 — claimed extra leading `|` in the markdown table) was a false positive; the table source has the correct single leading pipe per row and renders as three columns on GitHub. Skipped.
robbyt
added a commit
that referenced
this pull request
May 8, 2026
* docs: add LOGGING.md describing the logging philosophy Captures the design decisions arrived at across PRs #105, #110, #113, and #114 (issues #85, #99, #111, #112) — the four-PR convergence on: - Logging is opt-in; no constructor demands a slog.Handler. - nil handler means "inherit from slog.Default()" at every layer. - Exactly one fallback path: helpers.SetupLogger. - Functional options are silent on nil (Go idiom). - Group naming follows (engine, struct) shape. Includes a prior-art section showing how each choice tracks stdlib slog, slog.DiscardHandler, OpenTelemetry otelslog, go-logr/logr, the Cheney/Pike functional-options lineage, and database/sql. Also covers the explicitly-rejected alternatives (stdout fallback, nil-warning at construction, panic on nil at option layer, mandatory handler argument, library-defined logger interface) with the failure mode for each. Architectural map shows every nil-handler path funnels through helpers.SetupLogger; history table maps the four issues to the four PRs that shipped them. This is a contributor-facing engineering document. The README's "Capturing diagnostic logs" section is unchanged for now and remains the user-facing entry point; can be linked in a follow-up if desired. * docs(LOGGING): correct three Copilot review findings Three substantive corrections from PR #115 review (Copilot): 1. Principle 1 over-claimed "true no-op on nil" for all four option layers. Only the compiler-level WithLogHandler/WithLogger (relaxed in #111 / PR #113) skip both the set and the clear-of-the-other- field. The top-level and per-engine WithLogHandler do plain field assignment — they accept nil but don't preserve a prior non-nil value across the same option list. Doc now distinguishes the two layers and explains why the distinction is benign for the common single-call case. 2. Principle 2 said records carry the `<engine>.<struct>` prefix "regardless of whether the host configured a handler." That's not true: helpers.SetupLogger only adds the engineName group when the handler is nil. With a host-supplied handler the prefix is just `<struct>`. Doc now describes the asymmetry and explains why it's deliberate (library-owned sink vs. host-owned sink). 3. Principle 5 used `engine=extism` / `script=ExecutableUnit` filter examples that conflate slog groups with key/value attributes. Rephrased to describe slog.Handler.WithGroup behavior accurately (group qualifier on attribute keys; JSON nesting; text prefix) and to condition the `<engine>` prefix on the nil-handler case. The fourth Copilot comment (line 233 — claimed extra leading `|` in the markdown table) was a false positive; the table source has the correct single leading pipe per row and renders as three columns on GitHub. Skipped. --------- Co-authored-by: Claude <noreply@anthropic.com>
robbyt
added a commit
that referenced
this pull request
May 10, 2026
* docs: add CHANGELOG.md (Keep a Changelog 1.1.0) Closes #102 Repo had no CHANGELOG.md; users had to read GitHub release notes to find what changed between versions. With v1 ahead and a queue of breaking changes (#86, #87, #88, #89, #90, #91, #104), having a CHANGELOG before landing those gives downstream code reviewers and IDEs visibility into what's coming. Format follows Keep a Changelog 1.1.0 with the standard six headings (Added / Changed / Deprecated / Removed / Fixed / Security). [Unreleased] section captures the run of merged-but-not-released PRs since v0.7.0: - #103 polyscript.New[E] generic constructor (deprecates 12 FromXxx) - #105 slog.Handler optional in engine subpackages - #106 RequestToMap mutation fix - #107 HTTP loader MaxBodySize cap - #110 unify nil-handler on slog.Default(); drop stdout fallbacks - #113 WithLogHandler(nil)/WithLogger(nil) → no-op - #114 drop redundant nil-guards; tighten WithLogHandler doc - #115 docs/LOGGING.md - #117 fix bare top-level json.Number leak - #118 WithGlobals additive; drop dead URL check - #119 extism Eval test coverage Backfilled the three releases the issue called out (v0.5.0, v0.6.0, v0.7.0) from existing GitHub release notes, sorted into Keep a Changelog categories. Earlier releases (v0.0.x through v0.4.0) intentionally not backfilled per the issue scope; can be added in a follow-up if desired. Out of scope: the optional CI gate that fails PRs not touching CHANGELOG.md. Adds noise to small bug PRs and wants opt-out-label infrastructure to support it; better as a separate issue. * docs(CHANGELOG): clarify deprecated FromXxx still take positional handler Copilot review noted that "no constructor demands a handler" is misleading because the deprecated FromXxx constructors still take a positional `logHandler slog.Handler` argument (even though nil is accepted). Reword to distinguish the new generic constructor (no handler arg) from the deprecated ones (still take it, but nil is OK). --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Relaxes
WithLogHandler(nil)andWithLogger(nil)across the three engines' compiler packages so they're a no-op instead of returning an error. This makes them behave the way the rest of the package already does post-#85/#99 — every other nil-handler path routes throughhelpers.SetupLogger, which inherits fromslog.Default().Before
After
The "true no-op" detail (rather than "set field to nil") matters:
WithLogHandler(real)followed byWithLogHandler(nil)now leaves the real handler in place, matching caller intuition.Idiomatic precedent
slog.NewTextHandler(w, nil)accepts nil opts and falls back to defaults.otelslog.WithLoggerProviderdefaults to the global provider when the option is omitted.Option(nil)as semantically equivalent to not calling the option at all — the absence already produces the default.Out of scope
The redundant
if cfg.handler != nilguards inpolyscript.go:251/262/273andengines/{extism,risor,starlark}/new.go:72/85exist solely to work around the now-removed nil-rejection. They become harmless once this lands. Cleanup is tracked under #112 and depends on this PR.Files changed
engines/{extism,risor,starlark}/compiler/options.go— relaxed both options + godocengines/{extism,risor,starlark}/compiler/options_test.go— flipped "errors on nil" → "no-op on nil + prior state preserved"engines/{risor,starlark}/compiler/compiler_test.go— same flip for the higher-level "Option error handling" subtestfmtimport dropped from risor and starlarkcompiler/options.go(no longer used; extism keeps it)Test plan
go test -race -count=1 ./...— greengo vet ./...— cleanCloses #111
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code