Summary
Two paths into engine construction handle nil slog.Handler differently:
| Site |
Behavior on nil |
engines/risor/compiler/options.go:40-42 WithLogHandler |
returns fmt.Errorf("log handler cannot be nil") |
engines/extism/compiler/options.go:33-34 WithLogHandler |
same — errors |
engines/starlark/compiler/options.go:40-41 WithLogHandler |
same — errors |
engines/{extism,risor,starlark}/evaluator/evaluator.go New(handler, ...) |
accepts nil; routes through helpers.SetupLogger which inherits from slog.Default() |
engines/{extism,risor,starlark}/evaluator/response.go newEvalResult |
same — accepts nil |
platform/script/executableUnit.go:49-56 NewExecutableUnit |
same — accepts nil |
Post-#99, every constructor except the WithLogHandler functional option treats nil as "inherit from slog.Default()." The WithLogHandler option is the lone outlier that rejects nil.
Why it matters
It pushes nil-checks onto every caller. The façade code in polyscript.go:251-274 and engines/{extism,risor,starlark}/new.go:72-74,85-87 already works around it:
if cfg.handler != nil {
opts = append(opts, compiler.WithLogHandler(cfg.handler))
}
That guard exists solely to keep WithLogHandler(nil) from returning an error. If WithLogHandler accepted nil and delegated to SetupLogger (matching the rest of the package), all six call sites collapse to a plain unconditional append, and the public API stops surprising callers who write engine.WithLogHandler(myHandler) where myHandler happens to be nil.
Proposal
Make WithLogHandler(nil) a no-op (or, equivalently, store nil and let the existing setupLogger() chain at engines/*/compiler/options.go invoke helpers.SetupLogger, which already handles nil correctly).
Concretely
- Drop the nil-rejection in all three
WithLogHandler implementations:
engines/risor/compiler/options.go:40-42
engines/extism/compiler/options.go:33-34 (need to verify line)
engines/starlark/compiler/options.go:40-41
- Same treatment for
WithLogger(nil) for symmetry — currently also rejects nil at engines/*/compiler/options.go lines 55-57 (risor), and equivalents in extism/starlark.
- Simplify the six call sites in
polyscript.go and engines/*/new.go that currently nil-guard around the option.
- Update or drop the existing tests that assert the error return for
WithLogHandler(nil) / WithLogger(nil).
Out of scope
- Behavior of explicitly-supplied non-nil handlers — unchanged.
- The
helpers.SetupLogger contract itself — unchanged.
Tradeoff to consider
The current behavior could be defended as "fail loudly if a caller mistakenly passes nil." But the rest of the package — including the engine New() constructors callers reach more often — has already chosen the silent-inherit semantics post-#99 / #85. Symmetry probably wins; if loud failure is preferred, the fix would instead go the other direction (make New() and newEvalResult() reject nil too), but that would re-open #99.
Files
engines/risor/compiler/options.go
engines/extism/compiler/options.go
engines/starlark/compiler/options.go
polyscript.go — simplify call sites
engines/{extism,risor,starlark}/new.go — simplify call sites
engines/*/compiler/options_test.go — update assertions
Surfaced during review of #110 (which closed #99). Filed to track the remaining inconsistency.
Summary
Two paths into engine construction handle nil
slog.Handlerdifferently:nilengines/risor/compiler/options.go:40-42WithLogHandlerfmt.Errorf("log handler cannot be nil")engines/extism/compiler/options.go:33-34WithLogHandlerengines/starlark/compiler/options.go:40-41WithLogHandlerengines/{extism,risor,starlark}/evaluator/evaluator.goNew(handler, ...)helpers.SetupLoggerwhich inherits fromslog.Default()engines/{extism,risor,starlark}/evaluator/response.gonewEvalResultplatform/script/executableUnit.go:49-56NewExecutableUnitPost-#99, every constructor except the
WithLogHandlerfunctional option treats nil as "inherit fromslog.Default()." TheWithLogHandleroption is the lone outlier that rejects nil.Why it matters
It pushes nil-checks onto every caller. The façade code in
polyscript.go:251-274andengines/{extism,risor,starlark}/new.go:72-74,85-87already works around it:That guard exists solely to keep
WithLogHandler(nil)from returning an error. IfWithLogHandleraccepted nil and delegated toSetupLogger(matching the rest of the package), all six call sites collapse to a plain unconditional append, and the public API stops surprising callers who writeengine.WithLogHandler(myHandler)wheremyHandlerhappens to be nil.Proposal
Make
WithLogHandler(nil)a no-op (or, equivalently, store nil and let the existingsetupLogger()chain atengines/*/compiler/options.goinvokehelpers.SetupLogger, which already handles nil correctly).Concretely
WithLogHandlerimplementations:engines/risor/compiler/options.go:40-42engines/extism/compiler/options.go:33-34(need to verify line)engines/starlark/compiler/options.go:40-41WithLogger(nil)for symmetry — currently also rejects nil atengines/*/compiler/options.golines 55-57 (risor), and equivalents in extism/starlark.polyscript.goandengines/*/new.gothat currently nil-guard around the option.WithLogHandler(nil)/WithLogger(nil).Out of scope
helpers.SetupLoggercontract itself — unchanged.Tradeoff to consider
The current behavior could be defended as "fail loudly if a caller mistakenly passes nil." But the rest of the package — including the engine
New()constructors callers reach more often — has already chosen the silent-inherit semantics post-#99 / #85. Symmetry probably wins; if loud failure is preferred, the fix would instead go the other direction (makeNew()andnewEvalResult()reject nil too), but that would re-open #99.Files
engines/risor/compiler/options.goengines/extism/compiler/options.goengines/starlark/compiler/options.gopolyscript.go— simplify call sitesengines/{extism,risor,starlark}/new.go— simplify call sitesengines/*/compiler/options_test.go— update assertionsSurfaced during review of #110 (which closed #99). Filed to track the remaining inconsistency.