From 7d2e698f8f92dda362fee6d57468b25d01afb070 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Mon, 20 Jul 2026 21:36:56 +0200 Subject: [PATCH 1/8] fix: terminate the Request on non-finite floats and non-reflexive UI values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the scattered NaN/±Inf handling with a uniform fail-fast policy. A non-finite float64, or a UI value that is not equal to itself (issue #179), has no valid rendering, wire, or map-key representation, so log the cause and terminate the offending Request instead of blanking, deduping, or silently dropping it. Two domains converge on the existing Request.Cancel (cancel + log): - Non-reflexive comparable values: Request.NewElement now guards every element with tag.NewErrNotUsableAsTag (catches NaN and runtime-incomparable) in all builds, replacing the debug-only NewErrNotComparable panic. Container children flow through NewElement, so this fixes the reconcile churn in #179 centrally without touching ContainerHelper. - Non-finite floats: InputFloat (render/update/input) and the click path (runAtof/callEventHandler) terminate on any NaN/±Inf, from the browser or server-side, dropping the empty-string coercion and the NaN self-inequality dedup. Adds the ErrValueNotFinite sentinel. sanitizeFloatForT keeps its finite-out-of-range check (an infinity now falls out as ErrFloatOutOfRange) and no longer rejects finiteness itself. Removes the now-unused exported bind.ErrFloatNotFinite (breaking). --- click.go | 7 +- click_test.go | 72 ++++++++++++----- element_test.go | 37 ++++++--- errors.go | 11 +++ eventhandler.go | 10 +++ eventhandler_test.go | 37 +++++++++ lib/bind/setterfloat64.go | 25 +++--- lib/bind/setterfloat64_test.go | 39 ++++----- lib/ui/containerhelper_test.go | 44 ++++++++++ lib/ui/input_widgets.go | 59 ++++++-------- lib/ui/input_widgets_test.go | 143 +++++++++------------------------ request.go | 13 +-- request_test.go | 18 ++--- 13 files changed, 298 insertions(+), 217 deletions(-) diff --git a/click.go b/click.go index 0da17023..29368d71 100644 --- a/click.go +++ b/click.go @@ -113,10 +113,15 @@ func runFormatFloat(value float64) string { func runAtof(value string) (n float64, ok bool) { var err error n, err = strconv.ParseFloat(value, 64) - ok = err == nil && !math.IsInf(n, 0) && !math.IsNaN(n) + ok = err == nil return } +// finite reports whether f is neither NaN nor infinite. +func finite(f float64) bool { + return !math.IsNaN(f) && !math.IsInf(f, 0) +} + func runAtoi(value string) (n int, ok bool) { var err error n, err = strconv.Atoi(value) diff --git a/click_test.go b/click_test.go index 038b9b78..2f51b932 100644 --- a/click_test.go +++ b/click_test.go @@ -1,6 +1,7 @@ package jaws import ( + "math" "strings" "testing" ) @@ -63,31 +64,11 @@ func TestParseClickData(t *testing.T) { in: "bad 20 0 save", wantOK: false, }, - { - name: "nan x", - in: "NaN 20 0 save", - wantOK: false, - }, - { - name: "infinite x", - in: "+Inf 20 0 save", - wantOK: false, - }, { name: "invalid y", in: "10 bad 0 save", wantOK: false, }, - { - name: "nan y", - in: "10 NaN 0 save", - wantOK: false, - }, - { - name: "infinite y", - in: "10 -Inf 0 save", - wantOK: false, - }, { name: "invalid keystate", in: "10 20 bad save", @@ -123,6 +104,51 @@ func TestParseClickData(t *testing.T) { } } +func TestParseClickDataAcceptsNonFinite(t *testing.T) { + // runAtof no longer rejects non-finite coordinates; the event dispatch terminates + // the Request instead (see TestCallEventHandlerTerminatesOnNonFiniteClick). + tests := []struct { + name string + in string + check func(Click) bool + }{ + {"nan x", "NaN 20 0 save", func(c Click) bool { return math.IsNaN(c.X) && c.Y == 20 }}, + {"infinite x", "+Inf 20 0 save", func(c Click) bool { return math.IsInf(c.X, 1) && c.Y == 20 }}, + {"nan y", "10 NaN 0 save", func(c Click) bool { return c.X == 10 && math.IsNaN(c.Y) }}, + {"infinite y", "10 -Inf 0 save", func(c Click) bool { return c.X == 10 && math.IsInf(c.Y, -1) }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clk, _, ok := parseClickData(tt.in) + if !ok { + t.Fatalf("ok = false, want true") + } + if !tt.check(clk) { + t.Fatalf("unexpected click %+v", clk) + } + }) + } +} + +func TestFinite(t *testing.T) { + tests := []struct { + f float64 + want bool + }{ + {0, true}, + {-1.5, true}, + {math.MaxFloat64, true}, + {math.NaN(), false}, + {math.Inf(1), false}, + {math.Inf(-1), false}, + } + for _, tt := range tests { + if got := finite(tt.f); got != tt.want { + t.Errorf("finite(%v) = %v, want %v", tt.f, got, tt.want) + } + } +} + func TestClickString(t *testing.T) { got := (Click{Name: "x", X: 1.25, Y: 2.5, Shift: true, Control: true, Alt: true}).String() want := "1.25 2.5 7 x" @@ -142,6 +168,12 @@ func Fuzz_parseClickData(f *testing.F) { if !ok { return } + // parseClickData accepts non-finite coordinates (the event dispatch terminates + // the Request on them); Click.String round-trips only finite values, since + // NaN != NaN defeats the equality check below. + if !finite(clk.X) || !finite(clk.Y) { + return + } encoded := clk.String() clk2, after2, ok := parseClickData(encoded) diff --git a/element_test.go b/element_test.go index 8ea9089a..eb596d48 100644 --- a/element_test.go +++ b/element_test.go @@ -2,11 +2,13 @@ package jaws import ( "bytes" + "context" "errors" "fmt" "html/template" "io" "log/slog" + "math" "net/http" "net/http/httptest" "reflect" @@ -736,15 +738,6 @@ func TestElement_ApplyGetterDebugBranches(t *testing.T) { if _, _, err := elem.ApplyGetter(agErr); err != tag.ErrNotComparable { t.Fatalf("expected init err, got %v", err) } - - if deadlock.Debug { - defer func() { - if recover() == nil { - t.Fatal("expected panic for non-comparable UI in debug mode") - } - }() - rq.NewElement(testUnhashableUI{m: map[string]int{"x": 1}}) - } } type testClickHandler struct{} @@ -1230,3 +1223,29 @@ func TestElement_JawsInit(t *testing.T) { t.Error(err) } } + +type nonReflexiveUI struct{ f float64 } + +func (nonReflexiveUI) JawsRender(*Element, io.Writer, []any) error { return nil } +func (nonReflexiveUI) JawsUpdate(*Element) {} + +// TestNewElementTerminatesOnNonReflexiveUI verifies that a UI value that is not equal +// to itself (a comparable struct holding NaN) terminates the Request and logs the +// cause, rather than being silently accepted and later corrupting map-key lookups. +func TestNewElementTerminatesOnNonReflexiveUI(t *testing.T) { + tj := newTestJaws() + t.Cleanup(tj.Close) + rq := newWrappedTestRequest(tj.Jaws, nil) + if rq == nil { + t.Fatal("nil request") + } + + rq.NewElement(nonReflexiveUI{f: math.NaN()}) + + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + if !strings.Contains(tj.log.String(), "not usable as a UI value") { + t.Fatalf("expected termination to be logged, got %q", tj.log.String()) + } +} diff --git a/errors.go b/errors.go index 4f8030e8..06efcc20 100644 --- a/errors.go +++ b/errors.go @@ -32,6 +32,17 @@ var ErrValueUnchanged = errors.New("value unchanged") // with [errors.Is]; the wrapped text identifies which channel overflowed. var ErrRequestOverloaded = errors.New("request overloaded") +// ErrValueNotFinite indicates a [Request] was torn down because a NaN or infinite +// float64 reached the UI. +// +// A non-finite value has no valid rendering or wire representation and, in the case +// of NaN, is not even equal to itself, so it cannot be coerced safely. Rather than +// silently blanking or dropping it, the Request that produced it is cancelled. The +// cancellation cause reachable via [context.Cause] on [Request.Context] wraps this +// sentinel, so it can be matched with [errors.Is]; the wrapped text identifies the +// offending value. +var ErrValueNotFinite = errors.New("float value is not finite") + // ErrRequestAlreadyClaimed is returned when [Jaws.UseRequest] is called more than once for a [Request]. var ErrRequestAlreadyClaimed = errors.New("request already claimed") diff --git a/eventhandler.go b/eventhandler.go index dc56308e..3cec91a7 100644 --- a/eventhandler.go +++ b/eventhandler.go @@ -2,6 +2,7 @@ package jaws import ( "errors" + "fmt" "reflect" "github.com/linkdata/jaws/lib/what" @@ -36,6 +37,15 @@ func callEventHandler(obj any, elem *Element, wht what.What, value string) (err var clk Click var ok bool if clk, _, ok = parseClickData(value); ok { + if !finite(clk.X) || !finite(clk.Y) { + // A non-finite coordinate cannot come from a well-behaved browser; + // terminate the Request rather than dispatch a garbage click. Report the + // event handled (nil) so the dispatch loop stops without also alerting a + // connection that is being torn down. + elem.Request.Cancel(fmt.Errorf("%w: click %v,%v", ErrValueNotFinite, clk.X, clk.Y)) + err = nil + return + } if wht == what.Click { if h, ok := obj.(ClickHandler); ok { err = h.JawsClick(elem, clk) diff --git a/eventhandler_test.go b/eventhandler_test.go index 86208416..1df90faa 100644 --- a/eventhandler_test.go +++ b/eventhandler_test.go @@ -1,6 +1,7 @@ package jaws import ( + "context" "errors" "fmt" "html/template" @@ -220,6 +221,42 @@ func TestRequest_CallAllEventHandlersRequiresFrozenElement(t *testing.T) { }) } +func TestCallEventHandlerTerminatesOnNonFiniteClick(t *testing.T) { + tests := []struct { + name string + wht what.What + click string + }{ + {"click nan x", what.Click, "NaN 2 0 name"}, + {"click +inf y", what.Click, "1 +Inf 0 name"}, + {"click -inf x", what.Click, "-Inf 2 0 name"}, + {"contextmenu nan y", what.ContextMenu, "1 NaN 0 name"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rq := newTestRequest(t) + defer rq.Close() + rec := &clickInputSetRecorder{} + elem := rq.NewElement(testDivWidget{inner: "x"}) + elem.AddHandlers(clickOnlyComboHandler{rec: rec}) + elem.Freeze() + + value := tt.click + "\t" + elem.Jid().String() + "\t" + // The dispatch reports the event handled (nil) after terminating, so the + // dying connection is not also sent an alert. + if err := rq.callAllEventHandlers(0, tt.wht, value); err != nil { + t.Fatalf("callAllEventHandlers err = %v, want nil", err) + } + if cause := context.Cause(rq.Context()); !errors.Is(cause, ErrValueNotFinite) { + t.Fatalf("cause = %v, want wrapping ErrValueNotFinite", cause) + } + if rec.clickCalls != 0 { + t.Fatalf("click handler fired %d times, want 0", rec.clickCalls) + } + }) + } +} + func Test_CallEventHandlers_ClickDispatchCombinations(t *testing.T) { rq := newTestRequest(t) defer rq.Close() diff --git a/lib/bind/setterfloat64.go b/lib/bind/setterfloat64.go index 525caeb7..bc0bead9 100644 --- a/lib/bind/setterfloat64.go +++ b/lib/bind/setterfloat64.go @@ -9,12 +9,8 @@ import ( "github.com/linkdata/jaws/lib/tag" ) -var ( - // ErrFloatNotFinite reports that a float value is NaN or infinite. - ErrFloatNotFinite = errors.New("float value is not finite") - // ErrFloatOutOfRange reports that a finite float does not fit the target type. - ErrFloatOutOfRange = errors.New("float value out of range for target type") -) +// ErrFloatOutOfRange reports that a finite float does not fit the target type. +var ErrFloatOutOfRange = errors.New("float value out of range for target type") type numeric interface { ~float32 | ~float64 | @@ -26,9 +22,15 @@ type setterFloat64[T numeric] struct { Setter[T] } -// sanitizeFloatForT validates value before it is converted to T. It rejects -// non-finite values for every numeric T, and for integer T also rejects values -// whose truncation toward zero falls outside the type's representable range. +// sanitizeFloatForT validates value before it is converted to T. For integer T it +// rejects values whose truncation toward zero falls outside the type's +// representable range; for float32 it rejects a float64 that overflows to infinity +// on conversion. +// +// value is expected to be finite. The number and range input widgets terminate the +// Request on a non-finite value before it reaches the setter, so finiteness is not +// re-validated here: an infinity is incidentally rejected as out of range, but a NaN +// passed directly would slip through and convert to an implementation-defined value. // // The conversion T(value) truncates toward zero, so the lower bound is compared // against math.Trunc(value): a fractional value like -128.5 truncates to the valid @@ -48,11 +50,6 @@ type setterFloat64[T numeric] struct { // such as "type Celsius float64" falls through to the float default branch and is // range-checked as if it were its predeclared underlying type. func sanitizeFloatForT[T numeric](value float64) error { - // Non-finite values, typically from the untrusted browser, corrupt the bound value; - // NaN in particular defeats the equality-based update dedup (NaN != NaN). Reject them. - if math.IsNaN(value) || math.IsInf(value, 0) { - return ErrFloatNotFinite - } var lo, hiExcl float64 switch any(T(0)).(type) { case int8: diff --git a/lib/bind/setterfloat64_test.go b/lib/bind/setterfloat64_test.go index 77bef080..56078ffb 100644 --- a/lib/bind/setterfloat64_test.go +++ b/lib/bind/setterfloat64_test.go @@ -142,17 +142,18 @@ func Test_setterFloat64_conversionSignalsCanonicalChange(t *testing.T) { }) } -// Test_setterFloat64_sanitizesUntrustedInput covers the float-from-client guard: -// non-finite values are rejected for every numeric type and out-of-range values -// are rejected before the (otherwise wrapping) float->int conversion. The bound -// value must be left unchanged on rejection. +// Test_setterFloat64_sanitizesUntrustedInput covers the setter's range guard: +// out-of-range values are rejected before the (otherwise wrapping) float->int +// conversion, and an infinity is incidentally rejected as out of range. The bound +// value must be left unchanged on rejection. Finiteness itself is enforced by the +// input widgets, not this setter. func Test_setterFloat64_sanitizesUntrustedInput(t *testing.T) { - t.Run("rejects NaN and Inf", func(t *testing.T) { + t.Run("rejects Inf as out of range", func(t *testing.T) { ts := newTestSetter(int8(5)) s := MakeSetterFloat64(ts) - for _, bad := range []float64{math.NaN(), math.Inf(1), math.Inf(-1)} { - if err := s.JawsSet(nil, bad); !errors.Is(err, ErrFloatNotFinite) { - t.Errorf("JawsSet(%v): expected ErrFloatNotFinite, got %v", bad, err) + for _, bad := range []float64{math.Inf(1), math.Inf(-1)} { + if err := s.JawsSet(nil, bad); !errors.Is(err, ErrFloatOutOfRange) { + t.Errorf("JawsSet(%v): expected ErrFloatOutOfRange, got %v", bad, err) } } if ts.Get() != 5 { @@ -202,13 +203,13 @@ func Test_setterFloat64_sanitizesUntrustedInput(t *testing.T) { } // assertIntTypeGuard exercises sanitizeFloatForT's per-type branch for an integer -// type T: NaN/Inf is rejected, an out-of-range value overflows, and an in-range -// value is accepted. +// type T: an infinite value is rejected as out of range, an out-of-range finite +// value overflows, and an in-range value is accepted. func assertIntTypeGuard[T numeric](t *testing.T, name string, inRange, tooBig float64) { t.Helper() s := MakeSetterFloat64(newTestSetter(T(0))) - if err := s.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatNotFinite) { - t.Errorf("%s: Inf: got %v, want ErrFloatNotFinite", name, err) + if err := s.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatOutOfRange) { + t.Errorf("%s: Inf: got %v, want ErrFloatOutOfRange", name, err) } if err := s.JawsSet(nil, tooBig); !errors.Is(err, ErrFloatOutOfRange) { t.Errorf("%s: %v: got %v, want ErrFloatOutOfRange", name, tooBig, err) @@ -219,9 +220,9 @@ func assertIntTypeGuard[T numeric](t *testing.T, name string, inRange, tooBig fl } // Test_setterFloat64_coversNumericTypes exercises every case of the type switch in -// sanitizeFloatForT: each integer type rejects out-of-range values, float32 rejects -// non-finite and finite-but-overflowing values, and float64 takes the -// finiteness-only default case. +// sanitizeFloatForT: each integer type rejects out-of-range values, and float32 +// rejects both an infinity and a finite value that overflows the float32 range (an +// in-range value is accepted). func Test_setterFloat64_coversNumericTypes(t *testing.T) { assertIntTypeGuard[int8](t, "int8", 1, 128) assertIntTypeGuard[int16](t, "int16", 1, 32768) @@ -234,11 +235,11 @@ func Test_setterFloat64_coversNumericTypes(t *testing.T) { assertIntTypeGuard[uint64](t, "uint64", 1, 18446744073709551616.0) // 2^64 assertIntTypeGuard[uint](t, "uint", 1, 18446744073709551616.0) // 2^64 - // float32 rejects non-finite values and finite values that overflow the float32 - // range (they would otherwise convert to ±Inf); an in-range value is accepted. + // float32 rejects an infinity and finite values that overflow the float32 range + // (they would otherwise convert to ±Inf); an in-range value is accepted. fs := MakeSetterFloat64(newTestSetter(float32(0))) - if err := fs.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatNotFinite) { - t.Errorf("float32 Inf: got %v, want ErrFloatNotFinite", err) + if err := fs.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatOutOfRange) { + t.Errorf("float32 Inf: got %v, want ErrFloatOutOfRange", err) } if err := fs.JawsSet(nil, 1e30); err != nil { t.Errorf("float32 1e30: got %v, want nil", err) diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index 3e8a4126..2ba17df2 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -1,10 +1,12 @@ package ui import ( + "context" "errors" "html/template" "io" "log/slog" + "math" "net/http" "net/http/httptest" "strings" @@ -16,6 +18,7 @@ import ( "github.com/linkdata/jaws/lib/htmlio" "github.com/linkdata/jaws/lib/jid" "github.com/linkdata/jaws/lib/named" + "github.com/linkdata/jaws/lib/tag" "github.com/linkdata/jaws/lib/what" "github.com/linkdata/jaws/lib/wire" ) @@ -805,3 +808,44 @@ func TestSelectWidget_NonSetterContainer(t *testing.T) { t.Fatalf("JawsInput on non-Setter container: want nil, got %v", err) } } + +// nanChildUI is a comparable UI value that is not equal to itself, reproducing the +// non-reflexive map-key hazard from issue #179. +type nanChildUI struct{ f float64 } + +func (nanChildUI) JawsRender(_ *jaws.Element, w io.Writer, _ []any) error { + _, err := io.WriteString(w, "child") + return err +} +func (nanChildUI) JawsUpdate(*jaws.Element) {} + +// TestContainerTerminatesOnNonReflexiveChild reproduces issue #179: a container child +// whose UI value contains NaN is a legal comparable map key but is not equal to +// itself, so container reconciliation cannot match it and would recreate it every +// update. NewElement instead terminates the Request. Covered on both the initial +// render and a later update. +func TestContainerTerminatesOnNonReflexiveChild(t *testing.T) { + t.Run("render", func(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{nanChildUI{f: math.NaN()}}} + container := NewContainer("div", tc) + elem := rq.NewElement(container) + var sb strings.Builder + _ = elem.JawsRender(&sb, nil) + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + }) + + t.Run("update", func(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("ok"))}} + container := NewContainer("div", tc) + elem, _ := renderUI(t, rq, container) + tc.contents = []jaws.UI{nanChildUI{f: math.NaN()}} + container.JawsUpdate(elem) + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + }) +} diff --git a/lib/ui/input_widgets.go b/lib/ui/input_widgets.go index d64c01a8..e2f23372 100644 --- a/lib/ui/input_widgets.go +++ b/lib/ui/input_widgets.go @@ -1,6 +1,7 @@ package ui import ( + "fmt" "html/template" "io" "math" @@ -136,37 +137,27 @@ type InputFloat struct { bind.Setter[float64] } -// emptyValueAttr is the explicit value="" prepended for a non-finite float so -// the widget still owns the value slot even though its formatted value is empty. -var emptyValueAttr = htmlio.Attr("value", "") +// finite reports whether f is neither NaN nor infinite. +func finite(f float64) bool { + return !math.IsNaN(f) && !math.IsInf(f, 0) +} -// str formats value for a number or range control, rendering non-finite values -// (NaN, ±Inf) as the empty string rather than an unparseable "NaN"/"+Inf" -// literal. +// str formats value for a number or range control. func (u *InputFloat) str(value float64) string { - if math.IsNaN(value) || math.IsInf(value, 0) { - return "" - } return strconv.FormatFloat(value, 'f', -1, 64) } func (u *InputFloat) renderFloatInput(elem *jaws.Element, w io.Writer, htmlType string, params ...any) (err error) { var getterAttrs []template.HTMLAttr if getterAttrs, err = u.applyGetterAttrs(elem, u.Setter); err == nil { - attrs := append(elem.ApplyParams(params), getterAttrs...) v := u.JawsGet(elem) - u.Last.Store(v) - // A finite value goes down the direct valueAttr path with no extra - // allocation. A non-finite value formats as "", which WriteHTMLInput would - // omit, letting a caller-supplied value= from params or a binder's - // InitialHTMLAttr take over the control while the bound value stays - // non-finite; prepend an explicit value="" so the widget owns the value slot - // (the HTML parser keeps the first of duplicate attributes). - s := u.str(v) - if s == "" { - attrs = append([]template.HTMLAttr{emptyValueAttr}, attrs...) + if !finite(v) { + elem.Cancel(fmt.Errorf("%w: %g", jaws.ErrValueNotFinite, v)) + return } - err = htmlio.WriteHTMLInput(w, elem.Jid(), htmlType, s, attrs) + u.Last.Store(v) + attrs := append(elem.ApplyParams(params), getterAttrs...) + err = htmlio.WriteHTMLInput(w, elem.Jid(), htmlType, u.str(v), attrs) } return } @@ -174,17 +165,20 @@ func (u *InputFloat) renderFloatInput(elem *jaws.Element, w io.Writer, htmlType // JawsUpdate updates the input value when the bound float64 value changes. func (u *InputFloat) JawsUpdate(elem *jaws.Element) { v := u.JawsGet(elem) + if !finite(v) { + elem.Cancel(fmt.Errorf("%w: %g", jaws.ErrValueNotFinite, v)) + return + } // An empty Last (no value stored yet, e.g. an update-only Register that never ran // renderFloatInput) makes the float64 assertion fail with ok==false; send the // initial value unconditionally in that case, matching how the other input // widgets' nil != value comparison sends on their first update. prev, ok := u.Last.Swap(v).(float64) // Compare raw float64 values, not rendered strings: this can skip rare cosmetic - // changes such as -0 -> 0, but avoids formatting on the common unchanged path. - // NaN != NaN, so a plain compare would re-send a NaN bound value on every update - // cycle (JawsInput rejects NaN from the browser, but the server can bind one); - // treat NaN -> NaN as unchanged. A real transition into or out of NaN still sends. - if !ok || (prev != v && !(math.IsNaN(prev) && math.IsNaN(v))) { + // changes such as -0 -> 0, but avoids formatting on the common unchanged path. A + // non-finite value can no longer be stored (it terminates the Request above), so + // prev is always finite and a plain compare is safe. + if !ok || prev != v { elem.SetValue(u.str(v)) } } @@ -202,13 +196,12 @@ func (u *InputFloat) JawsInput(elem *jaws.Element, value string) (err error) { // Parse errors are malformed client frames: jaws.js reads elem.value from // browser number/range controls. Leave Last as the last accepted value. if v, err = strconv.ParseFloat(value, 64); err == nil { - // The browser is untrusted and strconv.ParseFloat accepts "NaN"/"Inf". - // Reject non-finite input here (mirroring click.go's runAtof) so it never - // reaches the bound value or u.Last. NaN is especially harmful: it defeats - // the Last.Swap(v) != v dedup in JawsUpdate (NaN != NaN), which would - // re-emit a SetValue on every update cycle. - if math.IsNaN(v) || math.IsInf(v, 0) { - return bind.ErrFloatNotFinite + // The browser is untrusted and strconv.ParseFloat accepts "NaN"/"Inf". A + // non-finite value has no valid bound representation and cannot come from a + // well-behaved browser, so terminate the Request rather than store it. + if !finite(v) { + elem.Cancel(fmt.Errorf("%w: %g", jaws.ErrValueNotFinite, v)) + return } u.Last.Store(v) err = u.maybeDirty(elem, u.Setter.JawsSet(elem, v)) diff --git a/lib/ui/input_widgets_test.go b/lib/ui/input_widgets_test.go index 9fc389f0..3da336a4 100644 --- a/lib/ui/input_widgets_test.go +++ b/lib/ui/input_widgets_test.go @@ -1,6 +1,7 @@ package ui import ( + "context" "errors" "html/template" "math" @@ -266,31 +267,33 @@ func TestInputFloat_ReconcilesConvertedValues(t *testing.T) { } } -// TestInputFloat_RejectsNonFinite verifies that NaN/Inf from the untrusted browser -// (which strconv.ParseFloat accepts) is rejected and never reaches the bound value. -func TestInputFloat_RejectsNonFinite(t *testing.T) { - _, rq := newCoreRequest(t) - sf := newTestSetter(7.5) - number := NewNumber(sf) - elem, _ := renderUI(t, rq, number) +// TestInputFloat_TerminatesOnNonFiniteInput verifies that NaN/Inf from the untrusted +// browser (which strconv.ParseFloat accepts) terminates the Request and never reaches +// the bound value. JawsInput reports the event handled (nil). +func TestInputFloat_TerminatesOnNonFiniteInput(t *testing.T) { for _, bad := range []string{"NaN", "Inf", "-Inf", "+Inf"} { - if err := number.JawsInput(elem, bad); !errors.Is(err, bind.ErrFloatNotFinite) { - t.Fatalf("JawsInput(%q): expected ErrFloatNotFinite, got %v", bad, err) - } - if sf.Get() != 7.5 { - t.Fatalf("JawsInput(%q) mutated bound value to %v", bad, sf.Get()) - } + t.Run(bad, func(t *testing.T) { + _, rq := newCoreRequest(t) + sf := newTestSetter(7.5) + number := NewNumber(sf) + elem, _ := renderUI(t, rq, number) + if err := number.JawsInput(elem, bad); err != nil { + t.Fatalf("JawsInput(%q) err = %v, want nil", bad, err) + } + if cause := context.Cause(rq.Context()); !errors.Is(cause, jaws.ErrValueNotFinite) { + t.Fatalf("JawsInput(%q): cause = %v, want wrapping ErrValueNotFinite", bad, cause) + } + if sf.Get() != 7.5 { + t.Fatalf("JawsInput(%q) mutated bound value to %v", bad, sf.Get()) + } + }) } } -// TestInputFloat_NonFiniteRendersEmpty verifies that a non-finite bound float64 -// renders an explicit empty value= attribute for either widget sharing -// InputFloat.str, never the unparseable value="NaN"/"+Inf" literal. A number -// then shows a blank control; a range shows the browser's constraint-sanitized -// default value. The explicit value="" also lets the widget own the value slot, -// which TestInputFloat_NonFiniteOwnsValueAttr covers. -func TestInputFloat_NonFiniteRendersEmpty(t *testing.T) { - _, rq := newCoreRequest(t) +// TestInputFloat_TerminatesOnNonFiniteRender verifies that rendering a widget whose +// bound float64 is non-finite terminates the Request instead of emitting an +// unparseable "NaN"/"+Inf" literal or a coerced empty control. +func TestInputFloat_TerminatesOnNonFiniteRender(t *testing.T) { widgets := []struct { htmlType string make func(bind.Setter[float64]) jaws.UI @@ -300,49 +303,19 @@ func TestInputFloat_NonFiniteRendersEmpty(t *testing.T) { } for _, w := range widgets { for _, v := range []float64{math.NaN(), math.Inf(1), math.Inf(-1)} { - f := v - sf := newTestSetter(f) + _, rq := newCoreRequest(t) + sf := newTestSetter(v) _, got := renderUI(t, rq, w.make(sf)) - mustMatch(t, `^$`, got) if strings.Contains(got, "NaN") || strings.Contains(got, "Inf") { t.Fatalf("%s rendered non-finite literal for %v: %s", w.htmlType, v, got) } + if cause := context.Cause(rq.Context()); !errors.Is(cause, jaws.ErrValueNotFinite) { + t.Fatalf("%s %v: cause = %v, want wrapping ErrValueNotFinite", w.htmlType, v, cause) + } } } } -// TestInputFloat_NonFiniteOwnsValueAttr verifies that the widget's own value= -// attribute takes precedence over a caller-supplied value= from params or a -// binder's InitialHTMLAttr. A non-finite bound value formats as "", which -// WriteHTMLInput would otherwise omit, letting the caller value= own the control -// while the bound value stays non-finite. The widget emits a leading value="" so -// the HTML parser (first duplicate attribute wins) keeps the widget's value. -func TestInputFloat_NonFiniteOwnsValueAttr(t *testing.T) { - for _, htmlType := range []string{"number", "range"} { - make := func(g bind.Setter[float64]) jaws.UI { return NewNumber(g) } - if htmlType == "range" { - make = func(g bind.Setter[float64]) jaws.UI { return NewRange(g) } - } - - // Caller value= via a raw string param. The widget's value="" precedes it, - // so the HTML parser keeps the widget's value. - _, rq := newCoreRequest(t) - sf := newTestSetter(math.NaN()) - _, got := renderUI(t, rq, make(sf), `value="17"`) - mustMatch(t, `^$`, got) - - // Caller value= via a binder InitialHTMLAttr hook, likewise preceded by the - // widget's own value="". - var mu deadlock.Mutex - f := math.NaN() - b := bind.New(&mu, &f).InitialHTMLAttr(func(bind.Binder[float64], *jaws.Element) template.HTMLAttr { - return `value="17"` - }) - _, got = renderUI(t, rq, make(b)) - mustMatch(t, `type="`+htmlType+`" value="" value="17"`, got) - } -} - func TestInputFloat_RegisterSendsInitialZeroValue(t *testing.T) { jw, err := jaws.New() if err != nil { @@ -373,61 +346,19 @@ func TestInputFloat_RegisterSendsInitialZeroValue(t *testing.T) { } } -// TestInputFloat_NaNBoundValueDoesNotReemit verifies that a server-bound NaN value -// is sent once on transition but not re-emitted on every subsequent update. NaN != NaN -// would otherwise defeat the JawsUpdate dedup and re-send SetValue each cycle. The -// transition sends an empty control (not "NaN"), matching the parse-side contract. -func TestInputFloat_NaNBoundValueDoesNotReemit(t *testing.T) { - jw, err := jaws.New() - if err != nil { - t.Fatal(err) - } - t.Cleanup(jw.Close) - go jw.Serve() - - tr := jawstest.NewTestRequest(jw, nil) - if tr == nil { - t.Fatal("expected test request") - } - defer tr.Close() - <-tr.ReadyCh - +// TestInputFloat_TerminatesOnNonFiniteUpdate verifies that a server-bound value going +// non-finite terminates the Request on the next update rather than being coerced or +// re-emitted. +func TestInputFloat_TerminatesOnNonFiniteUpdate(t *testing.T) { + _, rq := newCoreRequest(t) sf := newTestSetter(1.5) number := NewNumber(sf) - elem := tr.NewElement(number) - var sb strings.Builder - if err := elem.JawsRender(&sb, nil); err != nil { - t.Fatal(err) - } - - waitValue := func() (string, bool) { - deadline := time.After(300 * time.Millisecond) - for { - select { - case msg := <-tr.OutCh: - if msg.What == what.Value { - return msg.Data, true - } - case <-deadline: - return "", false - } - } - } + elem, _ := renderUI(t, rq, number) - // A real transition to NaN is sent once, as an empty control. sf.Set(math.NaN()) number.JawsUpdate(elem) - tr.InCh <- wire.WsMsg{} // wake the loop so the queued op flushes to OutCh - if v, ok := waitValue(); !ok || v != "" { - t.Fatalf("expected one SetValue %q on transition to NaN, got ok=%v v=%q", "", ok, v) - } - - // The value is still NaN: further updates must not re-emit. - number.JawsUpdate(elem) - number.JawsUpdate(elem) - tr.InCh <- wire.WsMsg{} // wake the loop - if v, ok := waitValue(); ok { - t.Fatalf("NaN bound value re-emitted SetValue %q", v) + if cause := context.Cause(rq.Context()); !errors.Is(cause, jaws.ErrValueNotFinite) { + t.Fatalf("cause = %v, want wrapping ErrValueNotFinite", cause) } } diff --git a/request.go b/request.go index df35b83c..49fc9cfb 100644 --- a/request.go +++ b/request.go @@ -4,6 +4,7 @@ import ( "cmp" "context" "errors" + "fmt" "html" "io" "net/http" @@ -623,12 +624,14 @@ func (rq *Request) newElementLocked(ui UI) (elem *Element) { // multiplicity requirements are caller obligations; NewElement does not enforce // them. See [UI] for the complete contract. // -// Panics in debug builds when ui does not satisfy [UI]'s comparability requirement. +// If ui is unusable as a map key — not comparable at runtime, or not equal to +// itself, as a value holding [math.NaN] is — the Request is terminated with a cause +// wrapping [tag.ErrNotUsableAsTag]. Such a value cannot be matched reliably during +// element reconciliation, so it is a fatal contract violation rather than something +// to coerce. The Element is still created and returned so callers need not nil-check. func (rq *Request) NewElement(ui UI) *Element { - if deadlock.Debug { - if err := tag.NewErrNotComparable(ui); err != nil { - panic(err) - } + if err := tag.NewErrNotUsableAsTag(ui); err != nil { + rq.Cancel(fmt.Errorf("jaws: %T is not usable as a UI value: %w", ui, err)) } rq.mu.Lock() defer rq.mu.Unlock() diff --git a/request_test.go b/request_test.go index 8563c9b7..c8fdf487 100644 --- a/request_test.go +++ b/request_test.go @@ -2444,20 +2444,18 @@ func nextOutboundMsg(t *testing.T, rq *testRequest) wire.WsMsg { } } -func TestRequest_NewElement_DebugComparableCheck(t *testing.T) { - if !deadlock.Debug { - t.Skip("debug checks not enabled") - } - +func TestRequest_NewElement_TerminatesOnNonComparableUI(t *testing.T) { rq := newTestRequest(t) defer rq.Close() - defer func() { - if recover() == nil { - t.Fatal("expected panic for non-comparable UI when comparable check is enabled") - } - }() + // A UI value that is not comparable at runtime cannot be used as a map key; like a + // non-reflexive value it terminates the Request rather than being accepted and + // panicking later at first map use. rq.NewElement(testUnhashableUI{m: map[string]int{"x": 1}}) + + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } } func TestRequest_getElementByJidLocked_DebugUnsortedPanics(t *testing.T) { From 3266b02c42185f308a6ee5e104b3b011356b3b1b Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Mon, 20 Jul 2026 22:01:04 +0200 Subject: [PATCH 2/8] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20guard?= =?UTF-8?q?=20container=20reconcile,=20keep=20bind=20guard,=20catch=20over?= =?UTF-8?q?flow?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to review feedback on the non-finite/non-reflexive termination change: - Container reconcile hashed a wanted child UI before reaching the NewElement guard, so an interface-held slice/map (runtime-incomparable) panicked instead of cancelling. Validate each child with a shared cancelUnusableChild helper before the pool lookup, in both RenderContainer and reconcile, so unusable children never enter u.contents. Adds an incomparable-update regression (the NaN child was hashable and missed this). - Restore the finite guard in bind.sanitizeFloatForT and the exported bind.ErrFloatNotFinite. MakeSetterFloat64 is public, so a direct JawsSet(..., NaN) must not store NaN or do an implementation-defined integer conversion; the widget-layer termination is not the only caller. - Overflow strings ("1e999") parse to ±Inf with a range error, so the err==nil gates in InputFloat.JawsInput and click.runAtof skipped the finiteness check, leaving inputs live and clicks dropped. Classify the parsed result before the error so overflow terminates like a literal NaN/Inf. Adds overflow cases. - Update the UI/JawsContains contract docs and the Number/Range docs to require reflexivity and describe all-build Request cancellation, replacing the stale debug-only-panic and blank/default-render wording. --- click.go | 6 ++- click_test.go | 2 + contracts.go | 31 ++++++------- eventhandler_test.go | 1 + lib/bind/setterfloat64.go | 25 ++++++----- lib/bind/setterfloat64_test.go | 39 ++++++++--------- lib/ui/containerhelper.go | 36 +++++++++++++-- lib/ui/containerhelper_test.go | 80 +++++++++++++++++++++------------- lib/ui/input_widgets.go | 31 +++++++------ lib/ui/input_widgets_test.go | 8 ++-- lib/ui/number.go | 8 ++-- lib/ui/range.go | 10 ++--- 12 files changed, 171 insertions(+), 106 deletions(-) diff --git a/click.go b/click.go index 29368d71..8e84057d 100644 --- a/click.go +++ b/click.go @@ -1,6 +1,7 @@ package jaws import ( + "errors" "fmt" "math" "strconv" @@ -113,7 +114,10 @@ func runFormatFloat(value float64) string { func runAtof(value string) (n float64, ok bool) { var err error n, err = strconv.ParseFloat(value, 64) - ok = err == nil + // A range error (e.g. "1e999") still yields a usable ±Inf; only a syntax error is + // a malformed frame. Accept the value either way so the event dispatch's finiteness + // check can terminate the Request on a non-finite coordinate. + ok = err == nil || errors.Is(err, strconv.ErrRange) return } diff --git a/click_test.go b/click_test.go index 2f51b932..3d989a5e 100644 --- a/click_test.go +++ b/click_test.go @@ -116,6 +116,8 @@ func TestParseClickDataAcceptsNonFinite(t *testing.T) { {"infinite x", "+Inf 20 0 save", func(c Click) bool { return math.IsInf(c.X, 1) && c.Y == 20 }}, {"nan y", "10 NaN 0 save", func(c Click) bool { return c.X == 10 && math.IsNaN(c.Y) }}, {"infinite y", "10 -Inf 0 save", func(c Click) bool { return c.X == 10 && math.IsInf(c.Y, -1) }}, + {"overflow x", "1e999 20 0 save", func(c Click) bool { return math.IsInf(c.X, 1) && c.Y == 20 }}, + {"overflow y", "10 -1e999 0 save", func(c Click) bool { return c.X == 10 && math.IsInf(c.Y, -1) }}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/contracts.go b/contracts.go index 3e3c91f1..cf949492 100644 --- a/contracts.go +++ b/contracts.go @@ -11,13 +11,14 @@ import ( type Container interface { // JawsContains returns the current child [UI] values contained by elem. // - // The returned [UI] values must be comparable, since they are used as map keys - // (see [UI] for the comparability requirement), and the slice contents must not - // be modified after returning it. Returning a child UI again from a later call - // lets the container reuse its existing live [Element]. The same UI may occur - // more than once in one returned slice only when its type documents support for - // backing multiple live Elements. A child UI must not be shared with a different - // [Request]. + // The returned [UI] values must be comparable and equal to themselves, since they + // are used as map keys (see [UI] for the requirement); a value that is not, such + // as one holding NaN, cancels the [Request] instead of being reconciled. The slice + // contents must not be modified after returning it. Returning such a child UI again + // from a later call lets the container reuse its existing live [Element]. The same + // UI may occur more than once in one returned slice only when its type documents + // support for backing multiple live Elements. A child UI must not be shared with a + // different [Request]. JawsContains(elem *Element) (contents []UI) } @@ -67,14 +68,14 @@ type TemplateLookuper interface { // Application state referenced by UI values may be shared across Requests when // synchronized as required. // -// In addition, all UI objects must be comparable so they can be used as map keys. -// The compile-time type must be comparable; debug builds additionally perform a -// runtime value-level check in [Request.NewElement] and panic on a value that is -// statically comparable but not comparable at runtime (for example a comparable -// struct holding a func in an interface field). Production builds rely on the -// static check alone, so such a value is accepted and instead panics when first -// used as a map key; callers must therefore ensure UI values are genuinely -// comparable. +// In addition, all UI objects must be comparable and equal to themselves, so they +// can be used as map keys. The compile-time type must be comparable; +// [Request.NewElement] additionally performs a runtime value-level check in every +// build and cancels the Request when the value is not comparable at runtime (for +// example a comparable struct holding a func in an interface field) or not equal to +// itself (a value holding NaN). The cancellation cause wraps +// [github.com/linkdata/jaws/lib/tag.ErrNotUsableAsTag]. Callers must therefore +// ensure UI values are genuinely comparable and reflexive. type UI interface { Renderer Updater diff --git a/eventhandler_test.go b/eventhandler_test.go index 1df90faa..922b62a7 100644 --- a/eventhandler_test.go +++ b/eventhandler_test.go @@ -230,6 +230,7 @@ func TestCallEventHandlerTerminatesOnNonFiniteClick(t *testing.T) { {"click nan x", what.Click, "NaN 2 0 name"}, {"click +inf y", what.Click, "1 +Inf 0 name"}, {"click -inf x", what.Click, "-Inf 2 0 name"}, + {"click overflow x", what.Click, "1e999 2 0 name"}, {"contextmenu nan y", what.ContextMenu, "1 NaN 0 name"}, } for _, tt := range tests { diff --git a/lib/bind/setterfloat64.go b/lib/bind/setterfloat64.go index bc0bead9..525caeb7 100644 --- a/lib/bind/setterfloat64.go +++ b/lib/bind/setterfloat64.go @@ -9,8 +9,12 @@ import ( "github.com/linkdata/jaws/lib/tag" ) -// ErrFloatOutOfRange reports that a finite float does not fit the target type. -var ErrFloatOutOfRange = errors.New("float value out of range for target type") +var ( + // ErrFloatNotFinite reports that a float value is NaN or infinite. + ErrFloatNotFinite = errors.New("float value is not finite") + // ErrFloatOutOfRange reports that a finite float does not fit the target type. + ErrFloatOutOfRange = errors.New("float value out of range for target type") +) type numeric interface { ~float32 | ~float64 | @@ -22,15 +26,9 @@ type setterFloat64[T numeric] struct { Setter[T] } -// sanitizeFloatForT validates value before it is converted to T. For integer T it -// rejects values whose truncation toward zero falls outside the type's -// representable range; for float32 it rejects a float64 that overflows to infinity -// on conversion. -// -// value is expected to be finite. The number and range input widgets terminate the -// Request on a non-finite value before it reaches the setter, so finiteness is not -// re-validated here: an infinity is incidentally rejected as out of range, but a NaN -// passed directly would slip through and convert to an implementation-defined value. +// sanitizeFloatForT validates value before it is converted to T. It rejects +// non-finite values for every numeric T, and for integer T also rejects values +// whose truncation toward zero falls outside the type's representable range. // // The conversion T(value) truncates toward zero, so the lower bound is compared // against math.Trunc(value): a fractional value like -128.5 truncates to the valid @@ -50,6 +48,11 @@ type setterFloat64[T numeric] struct { // such as "type Celsius float64" falls through to the float default branch and is // range-checked as if it were its predeclared underlying type. func sanitizeFloatForT[T numeric](value float64) error { + // Non-finite values, typically from the untrusted browser, corrupt the bound value; + // NaN in particular defeats the equality-based update dedup (NaN != NaN). Reject them. + if math.IsNaN(value) || math.IsInf(value, 0) { + return ErrFloatNotFinite + } var lo, hiExcl float64 switch any(T(0)).(type) { case int8: diff --git a/lib/bind/setterfloat64_test.go b/lib/bind/setterfloat64_test.go index 56078ffb..77bef080 100644 --- a/lib/bind/setterfloat64_test.go +++ b/lib/bind/setterfloat64_test.go @@ -142,18 +142,17 @@ func Test_setterFloat64_conversionSignalsCanonicalChange(t *testing.T) { }) } -// Test_setterFloat64_sanitizesUntrustedInput covers the setter's range guard: -// out-of-range values are rejected before the (otherwise wrapping) float->int -// conversion, and an infinity is incidentally rejected as out of range. The bound -// value must be left unchanged on rejection. Finiteness itself is enforced by the -// input widgets, not this setter. +// Test_setterFloat64_sanitizesUntrustedInput covers the float-from-client guard: +// non-finite values are rejected for every numeric type and out-of-range values +// are rejected before the (otherwise wrapping) float->int conversion. The bound +// value must be left unchanged on rejection. func Test_setterFloat64_sanitizesUntrustedInput(t *testing.T) { - t.Run("rejects Inf as out of range", func(t *testing.T) { + t.Run("rejects NaN and Inf", func(t *testing.T) { ts := newTestSetter(int8(5)) s := MakeSetterFloat64(ts) - for _, bad := range []float64{math.Inf(1), math.Inf(-1)} { - if err := s.JawsSet(nil, bad); !errors.Is(err, ErrFloatOutOfRange) { - t.Errorf("JawsSet(%v): expected ErrFloatOutOfRange, got %v", bad, err) + for _, bad := range []float64{math.NaN(), math.Inf(1), math.Inf(-1)} { + if err := s.JawsSet(nil, bad); !errors.Is(err, ErrFloatNotFinite) { + t.Errorf("JawsSet(%v): expected ErrFloatNotFinite, got %v", bad, err) } } if ts.Get() != 5 { @@ -203,13 +202,13 @@ func Test_setterFloat64_sanitizesUntrustedInput(t *testing.T) { } // assertIntTypeGuard exercises sanitizeFloatForT's per-type branch for an integer -// type T: an infinite value is rejected as out of range, an out-of-range finite -// value overflows, and an in-range value is accepted. +// type T: NaN/Inf is rejected, an out-of-range value overflows, and an in-range +// value is accepted. func assertIntTypeGuard[T numeric](t *testing.T, name string, inRange, tooBig float64) { t.Helper() s := MakeSetterFloat64(newTestSetter(T(0))) - if err := s.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatOutOfRange) { - t.Errorf("%s: Inf: got %v, want ErrFloatOutOfRange", name, err) + if err := s.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatNotFinite) { + t.Errorf("%s: Inf: got %v, want ErrFloatNotFinite", name, err) } if err := s.JawsSet(nil, tooBig); !errors.Is(err, ErrFloatOutOfRange) { t.Errorf("%s: %v: got %v, want ErrFloatOutOfRange", name, tooBig, err) @@ -220,9 +219,9 @@ func assertIntTypeGuard[T numeric](t *testing.T, name string, inRange, tooBig fl } // Test_setterFloat64_coversNumericTypes exercises every case of the type switch in -// sanitizeFloatForT: each integer type rejects out-of-range values, and float32 -// rejects both an infinity and a finite value that overflows the float32 range (an -// in-range value is accepted). +// sanitizeFloatForT: each integer type rejects out-of-range values, float32 rejects +// non-finite and finite-but-overflowing values, and float64 takes the +// finiteness-only default case. func Test_setterFloat64_coversNumericTypes(t *testing.T) { assertIntTypeGuard[int8](t, "int8", 1, 128) assertIntTypeGuard[int16](t, "int16", 1, 32768) @@ -235,11 +234,11 @@ func Test_setterFloat64_coversNumericTypes(t *testing.T) { assertIntTypeGuard[uint64](t, "uint64", 1, 18446744073709551616.0) // 2^64 assertIntTypeGuard[uint](t, "uint", 1, 18446744073709551616.0) // 2^64 - // float32 rejects an infinity and finite values that overflow the float32 range - // (they would otherwise convert to ±Inf); an in-range value is accepted. + // float32 rejects non-finite values and finite values that overflow the float32 + // range (they would otherwise convert to ±Inf); an in-range value is accepted. fs := MakeSetterFloat64(newTestSetter(float32(0))) - if err := fs.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatOutOfRange) { - t.Errorf("float32 Inf: got %v, want ErrFloatOutOfRange", err) + if err := fs.JawsSet(nil, math.Inf(1)); !errors.Is(err, ErrFloatNotFinite) { + t.Errorf("float32 Inf: got %v, want ErrFloatNotFinite", err) } if err := fs.JawsSet(nil, 1e30); err != nil { t.Errorf("float32 1e30: got %v, want nil", err) diff --git a/lib/ui/containerhelper.go b/lib/ui/containerhelper.go index 3ba39bf9..996f3ab6 100644 --- a/lib/ui/containerhelper.go +++ b/lib/ui/containerhelper.go @@ -1,6 +1,7 @@ package ui import ( + "fmt" "html/template" "io" "slices" @@ -9,6 +10,7 @@ import ( "github.com/linkdata/jaws" "github.com/linkdata/jaws/lib/htmlio" + "github.com/linkdata/jaws/lib/tag" ) // ContainerHelper is a helper for widgets that render dynamic child collections. @@ -66,6 +68,11 @@ func (u *ContainerHelper) RenderContainer(elem *jaws.Element, w io.Writer, outer if err == nil { var contents []*jaws.Element for _, childUI := range u.Container.JawsContains(elem) { + // Skip a child that cannot be used as a pool key so it never enters + // u.contents; a later reconcile pool build would otherwise hash it. + if cancelUnusableChild(elem, childUI) { + continue + } childElem := elem.Request.NewElement(childUI) contents = append(contents, childElem) if err = childElem.JawsRender(w, nil); err != nil { @@ -138,12 +145,26 @@ func (u *ContainerHelper) deleteContent(elem *jaws.Element) { u.mu.Unlock() } +// cancelUnusableChild reports whether childUI cannot be used as a container pool +// key — not comparable at runtime, or not equal to itself (a value holding NaN) — +// and, when so, terminates the Request with a cause wrapping [tag.ErrNotUsableAsTag]. +// Callers skip the child so it never becomes a live [jaws.Element]. This mirrors the +// guard in [jaws.Request.NewElement] but runs before the value is hashed as a map key. +func cancelUnusableChild(elem *jaws.Element, childUI jaws.UI) bool { + if err := tag.NewErrNotUsableAsTag(childUI); err != nil { + elem.Request.Cancel(fmt.Errorf("jaws: container child %T is not usable as a UI value: %w", childUI, err)) + return true + } + return false +} + // reconcile matches u.contents to wantContents under u.mu and returns the // Elements to append, the leftover Elements to remove, and the old and new Jid -// orders. The lock is released with defer so that using a child UI as a map key -// cannot leave u.mu held if it panics: a UI that passes the static comparability -// check can still be non-comparable at runtime (a comparable struct holding e.g. -// a func in an interface field). This mirrors the defense in jaws.setDirty. +// orders. Each wanted child is validated with cancelUnusableChild before it is used +// as a pool key, so a value that is non-reflexive or non-comparable at runtime (a +// comparable struct holding e.g. a func in an interface field) terminates the Request +// instead of missing the lookup or panicking. The lock is still released with defer +// as a further guard against a panic leaving u.mu held, mirroring jaws.setDirty. func (u *ContainerHelper) reconcile(elem *jaws.Element, wantContents []jaws.UI) (toAppend, toRemove []*jaws.Element, oldOrder, newOrder []jaws.Jid) { u.mu.Lock() defer u.mu.Unlock() @@ -160,6 +181,13 @@ func (u *ContainerHelper) reconcile(elem *jaws.Element, wantContents []jaws.UI) newOrder = make([]jaws.Jid, 0, len(wantContents)) u.contents = u.contents[:0] for _, childUI := range wantContents { + // Validate before childUI is used as a pool key below: a non-reflexive value + // (one holding NaN) would silently miss the lookup, and a runtime-incomparable + // one would panic hashing it. Skipping it here also keeps it out of u.contents, + // so the pool build above never hits an unusable key. + if cancelUnusableChild(elem, childUI) { + continue + } var childElem *jaws.Element // Reuse a pooled Element, discarding any that were deleted out-of-band (a // what.Delete broadcast targeting a tag the child registered, or a browser diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index 2ba17df2..cb49f091 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -809,8 +809,8 @@ func TestSelectWidget_NonSetterContainer(t *testing.T) { } } -// nanChildUI is a comparable UI value that is not equal to itself, reproducing the -// non-reflexive map-key hazard from issue #179. +// nanChildUI is a comparable UI value that is not equal to itself (a non-reflexive +// map key), reproducing issue #179. type nanChildUI struct{ f float64 } func (nanChildUI) JawsRender(_ *jaws.Element, w io.Writer, _ []any) error { @@ -819,33 +819,53 @@ func (nanChildUI) JawsRender(_ *jaws.Element, w io.Writer, _ []any) error { } func (nanChildUI) JawsUpdate(*jaws.Element) {} -// TestContainerTerminatesOnNonReflexiveChild reproduces issue #179: a container child -// whose UI value contains NaN is a legal comparable map key but is not equal to -// itself, so container reconciliation cannot match it and would recreate it every -// update. NewElement instead terminates the Request. Covered on both the initial -// render and a later update. -func TestContainerTerminatesOnNonReflexiveChild(t *testing.T) { - t.Run("render", func(t *testing.T) { - _, rq := newCoreRequest(t) - tc := &testContainer{contents: []jaws.UI{nanChildUI{f: math.NaN()}}} - container := NewContainer("div", tc) - elem := rq.NewElement(container) - var sb strings.Builder - _ = elem.JawsRender(&sb, nil) - if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { - t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) - } - }) +// incomparableChildUI is statically comparable (an interface field) but panics when +// compared or hashed at runtime, since it holds a slice. +type incomparableChildUI struct{ v any } - t.Run("update", func(t *testing.T) { - _, rq := newCoreRequest(t) - tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("ok"))}} - container := NewContainer("div", tc) - elem, _ := renderUI(t, rq, container) - tc.contents = []jaws.UI{nanChildUI{f: math.NaN()}} - container.JawsUpdate(elem) - if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { - t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) - } - }) +func (incomparableChildUI) JawsRender(_ *jaws.Element, w io.Writer, _ []any) error { + _, err := io.WriteString(w, "child") + return err +} +func (incomparableChildUI) JawsUpdate(*jaws.Element) {} + +// TestContainerTerminatesOnUnusableChild covers issue #179 and its +// runtime-incomparable sibling: a container child UI that is not equal to itself +// (holds NaN) or not comparable at runtime (holds an interface-wrapped slice) cannot +// be a container pool key. The container must terminate the Request rather than churn +// (NaN) or panic hashing the key (incomparable). The incomparable update case pins +// that reconcile validates before the pool lookup, which would otherwise panic. +func TestContainerTerminatesOnUnusableChild(t *testing.T) { + bad := []struct { + name string + make func() jaws.UI + }{ + {"nan", func() jaws.UI { return nanChildUI{f: math.NaN()} }}, + {"incomparable", func() jaws.UI { return incomparableChildUI{v: []int{1}} }}, + } + for _, b := range bad { + t.Run(b.name+" render", func(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{b.make()}} + container := NewContainer("div", tc) + elem := rq.NewElement(container) + var sb strings.Builder + _ = elem.JawsRender(&sb, nil) + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + }) + + t.Run(b.name+" update", func(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("ok"))}} + container := NewContainer("div", tc) + elem, _ := renderUI(t, rq, container) + tc.contents = []jaws.UI{b.make()} + container.JawsUpdate(elem) // must terminate, not panic + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + }) + } } diff --git a/lib/ui/input_widgets.go b/lib/ui/input_widgets.go index e2f23372..d0fe4f2d 100644 --- a/lib/ui/input_widgets.go +++ b/lib/ui/input_widgets.go @@ -192,21 +192,24 @@ func (u *InputFloat) JawsInput(elem *jaws.Element, value string) (err error) { // when the setter reports unchanged; that would race ordinary typing. value = "0" } - var v float64 - // Parse errors are malformed client frames: jaws.js reads elem.value from - // browser number/range controls. Leave Last as the last accepted value. - if v, err = strconv.ParseFloat(value, 64); err == nil { - // The browser is untrusted and strconv.ParseFloat accepts "NaN"/"Inf". A - // non-finite value has no valid bound representation and cannot come from a - // well-behaved browser, so terminate the Request rather than store it. - if !finite(v) { - elem.Cancel(fmt.Errorf("%w: %g", jaws.ErrValueNotFinite, v)) - return - } - u.Last.Store(v) - err = u.maybeDirty(elem, u.Setter.JawsSet(elem, v)) + v, err := strconv.ParseFloat(value, 64) + // The browser is untrusted and strconv.ParseFloat accepts "NaN"/"Inf" (no error) + // and returns ±Inf with a range error for an overflowing magnitude like "1e999". + // A non-finite result has no valid bound representation and cannot come from a + // well-behaved browser, so terminate the Request rather than store it. Check the + // parsed value before the error so overflow is caught, not treated as malformed. + if !finite(v) { + elem.Cancel(fmt.Errorf("%w: %g", jaws.ErrValueNotFinite, v)) + return nil } - return + // A remaining error is a syntax error: a malformed client frame (jaws.js reads + // elem.value from browser number/range controls). Leave Last as the last accepted + // value. + if err != nil { + return err + } + u.Last.Store(v) + return u.maybeDirty(elem, u.Setter.JawsSet(elem, v)) } // InputDate is the reusable base for date input widgets. diff --git a/lib/ui/input_widgets_test.go b/lib/ui/input_widgets_test.go index 3da336a4..bc458fbc 100644 --- a/lib/ui/input_widgets_test.go +++ b/lib/ui/input_widgets_test.go @@ -268,10 +268,12 @@ func TestInputFloat_ReconcilesConvertedValues(t *testing.T) { } // TestInputFloat_TerminatesOnNonFiniteInput verifies that NaN/Inf from the untrusted -// browser (which strconv.ParseFloat accepts) terminates the Request and never reaches -// the bound value. JawsInput reports the event handled (nil). +// browser terminates the Request and never reaches the bound value. This covers both +// the literals strconv.ParseFloat accepts ("NaN"/"Inf") and an overflowing magnitude +// like "1e999", which parses to ±Inf with a range error. JawsInput reports the event +// handled (nil). func TestInputFloat_TerminatesOnNonFiniteInput(t *testing.T) { - for _, bad := range []string{"NaN", "Inf", "-Inf", "+Inf"} { + for _, bad := range []string{"NaN", "Inf", "-Inf", "+Inf", "1e999", "-1e999"} { t.Run(bad, func(t *testing.T) { _, rq := newCoreRequest(t) sf := newTestSetter(7.5) diff --git a/lib/ui/number.go b/lib/ui/number.go index 9dd45f98..5073e7e5 100644 --- a/lib/ui/number.go +++ b/lib/ui/number.go @@ -15,8 +15,10 @@ type Number struct{ InputFloat } // NewNumber returns a number input widget bound to g. // -// A non-finite bound value (NaN or ±Inf) renders as a blank control rather than -// an unparseable value. Contrast [NewRange], whose control cannot be blank. +// The bound value must be finite. A non-finite value (NaN or ±Inf) has no valid +// rendering or wire representation, so rendering, updating, or receiving one from +// the browser cancels the [jaws.Request] with a cause wrapping +// [jaws.ErrValueNotFinite]. func NewNumber(g bind.Setter[float64]) *Number { return &Number{InputFloat{Setter: g}} } // JawsRender renders ui as an HTML number input. @@ -26,7 +28,7 @@ func (u *Number) JawsRender(elem *jaws.Element, w io.Writer, params []any) error // Number renders an HTML number input. // -// See [NewNumber] for how a non-finite bound value renders. +// See [NewNumber] for how a non-finite bound value is handled. func (rw RequestWriter) Number(value any, params ...any) error { return rw.NewUI(NewNumber(bind.MakeSetterFloat64(value)), params...) } diff --git a/lib/ui/range.go b/lib/ui/range.go index 5480c27f..ab728014 100644 --- a/lib/ui/range.go +++ b/lib/ui/range.go @@ -15,10 +15,10 @@ type Range struct{ InputFloat } // NewRange returns a range input widget bound to g. // -// A range control cannot display a non-finite value: a bound NaN or ±Inf shows -// as the browser's constraint-sanitized default value for the control, not a -// blank field, while the bound value stays non-finite. Use [NewNumber] if the -// bound value may be non-finite. +// The bound value must be finite. A non-finite value (NaN or ±Inf) has no valid +// rendering or wire representation, so rendering, updating, or receiving one from +// the browser cancels the [jaws.Request] with a cause wrapping +// [jaws.ErrValueNotFinite]. func NewRange(g bind.Setter[float64]) *Range { return &Range{InputFloat{Setter: g}} } // JawsRender renders ui as an HTML range input. @@ -28,7 +28,7 @@ func (u *Range) JawsRender(elem *jaws.Element, w io.Writer, params []any) error // Range renders an HTML range input. // -// See [NewRange] for how a non-finite bound value renders. +// See [NewRange] for how a non-finite bound value is handled. func (rw RequestWriter) Range(value any, params ...any) error { return rw.NewUI(NewRange(bind.MakeSetterFloat64(value)), params...) } From 95f12619fadb61237ba2d714d2dca9845e513345 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Mon, 20 Jul 2026 22:35:18 +0200 Subject: [PATCH 3/8] =?UTF-8?q?fix:=20address=20review=20round=202=20?= =?UTF-8?q?=E2=80=94=20cancel=20outside=20the=20container=20lock,=20UI-spe?= =?UTF-8?q?cific=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ContainerHelper cancelled the Request while holding u.mu (reconcile validated each child inside the lock). Request.Cancel runs the user logger synchronously, which the locking contract forbids under a lock: a logger re-entering the container deadlocks. Validate the whole wanted-children slice with cancelUnusableChildren before locking (and before the render loop in RenderContainer), aborting on the first unusable child so later children are not created after fatal cancellation. Adds a reentrant-logger deadlock regression (verified to fail without the fix via a 2s timeout). - The cancellation cause reused tag.NewErrNotUsableAsTag, whose message advises implementing JawsGetTag — a non-fix for a raw jaws.UI map key. Add jaws.NewErrUnusableUI: UI-specific text that still matches tag.ErrNotUsableAsTag (and tag.ErrNotComparable) via errors.Is. NewElement and the container use it. - Document the cancellation side effect on the exported InputFloat type and its JawsUpdate/JawsInput methods, and update the tracked JAWS skill's comparability rules to require reflexivity and describe all-build cancellation. - Handle the previously blank-assigned render error in the container regression. --- .agents/skills/jaws/SKILL.md | 13 ++++-- element_test.go | 2 +- errunusableui.go | 46 +++++++++++++++++++++ lib/ui/containerhelper.go | 73 +++++++++++++++++++--------------- lib/ui/containerhelper_test.go | 52 +++++++++++++++++++++++- lib/ui/input_widgets.go | 11 +++++ request.go | 14 +++---- 7 files changed, 165 insertions(+), 46 deletions(-) create mode 100644 errunusableui.go diff --git a/.agents/skills/jaws/SKILL.md b/.agents/skills/jaws/SKILL.md index 4d9e0a3e..bcf2d582 100644 --- a/.agents/skills/jaws/SKILL.md +++ b/.agents/skills/jaws/SKILL.md @@ -35,7 +35,11 @@ JaWS is an immediate-mode, server-driven UI framework, not an MVC framework. ## Hard framework constraints -- Every JaWS `UI` value must be comparable. +- Every JaWS `UI` value must be comparable at runtime **and equal to itself**, since + it is used as a map key. A value that is only statically comparable (an interface + field holding a slice/map/func) or that holds a `NaN` (so `v != v`) is rejected: + `Request.NewElement` and the container widgets cancel the `Request` in all builds, + with a cause matching `tag.ErrNotUsableAsTag`, rather than silently accepting it. - Every JaWS `UI` value is request-scoped. Once used by one Request, never use that value with another Request; construct fresh widgets per request. The widgets may still refer to shared, synchronized application state, binders, @@ -43,9 +47,10 @@ JaWS is an immediate-mode, server-driven UI framework, not an MVC framework. - Within its Request, a `UI` value normally backs one live `Element`. Reuse one value for multiple live Elements only when its concrete type documents that support and retains no state that can differ between those Elements. -- `Container.JawsContains` must return hashable `UI` items, and the returned - slice must not be mutated after return. A UI value may occur more than once in - one returned slice only when its type supports multiple live Elements. +- `Container.JawsContains` must return `UI` items that are comparable and equal to + themselves (see above); returning one that is not cancels the `Request`. The + returned slice must not be mutated after return. A UI value may occur more than + once in one returned slice only when its type supports multiple live Elements. - Treat the package documentation shown by `go doc github.com/linkdata/jaws/lib/ui` as the canonical standard-widget multiplicity summary, and consult each concrete type's docs for its conditions. diff --git a/element_test.go b/element_test.go index eb596d48..4d4351ea 100644 --- a/element_test.go +++ b/element_test.go @@ -1245,7 +1245,7 @@ func TestNewElementTerminatesOnNonReflexiveUI(t *testing.T) { if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) } - if !strings.Contains(tj.log.String(), "not usable as a UI value") { + if !strings.Contains(tj.log.String(), "not usable as a jaws.UI value") { t.Fatalf("expected termination to be logged, got %q", tj.log.String()) } } diff --git a/errunusableui.go b/errunusableui.go new file mode 100644 index 00000000..63f10376 --- /dev/null +++ b/errunusableui.go @@ -0,0 +1,46 @@ +package jaws + +import ( + "reflect" + + "github.com/linkdata/jaws/lib/tag" +) + +// errUnusableUI reports that a value cannot be used as a [UI] value because it is +// not comparable at runtime, or not equal to itself as a value holding NaN is. +// +// It matches [tag.ErrNotUsableAsTag] and [tag.ErrNotComparable] with errors.Is, but +// carries UI-specific text: the tag package's advice to implement JawsGetTag cannot +// make a raw UI value usable as a map[UI] key, so it must not be surfaced here. +type errUnusableUI struct { + t reflect.Type +} + +func (e errUnusableUI) Error() (s string) { + s = "value" + if e.t != nil { + s = e.t.String() + } + return s + " is not usable as a jaws.UI value: it must be comparable and equal to itself" +} + +// Is reports whether target is a tag sentinel this error stands in for, so callers +// can match it with errors.Is. +func (errUnusableUI) Is(target error) bool { + return target == tag.ErrNotUsableAsTag || target == tag.ErrNotComparable +} + +// NewErrUnusableUI returns a non-nil error when ui cannot be used as a [UI] value, +// and nil when it can. +// +// A UI value is unusable when it is not comparable at runtime, or not equal to itself +// as a value holding NaN is, because it is used as a map key and would either panic +// or fail to match. The returned error matches [tag.ErrNotUsableAsTag] with errors.Is. +// [Request.NewElement] and the container widgets use it to terminate a Request handed +// such a value. +func NewErrUnusableUI(ui UI) error { + if tag.NewErrNotUsableAsTag(ui) != nil { + return errUnusableUI{t: reflect.TypeOf(ui)} + } + return nil +} diff --git a/lib/ui/containerhelper.go b/lib/ui/containerhelper.go index 996f3ab6..de3091b0 100644 --- a/lib/ui/containerhelper.go +++ b/lib/ui/containerhelper.go @@ -1,7 +1,6 @@ package ui import ( - "fmt" "html/template" "io" "slices" @@ -10,7 +9,6 @@ import ( "github.com/linkdata/jaws" "github.com/linkdata/jaws/lib/htmlio" - "github.com/linkdata/jaws/lib/tag" ) // ContainerHelper is a helper for widgets that render dynamic child collections. @@ -67,16 +65,18 @@ func (u *ContainerHelper) RenderContainer(elem *jaws.Element, w io.Writer, outer _, err = w.Write(b) if err == nil { var contents []*jaws.Element - for _, childUI := range u.Container.JawsContains(elem) { - // Skip a child that cannot be used as a pool key so it never enters - // u.contents; a later reconcile pool build would otherwise hash it. - if cancelUnusableChild(elem, childUI) { - continue - } - childElem := elem.Request.NewElement(childUI) - contents = append(contents, childElem) - if err = childElem.JawsRender(w, nil); err != nil { - break + // Validate every child before creating any Element: an unusable child (one + // that is not comparable at runtime, or not equal to itself) terminates the + // Request, and the rest must not be rendered. Keeping unusable children out of + // u.contents also stops a later reconcile pool build from hashing one. + children := u.Container.JawsContains(elem) + if !cancelUnusableChildren(elem, children) { + for _, childUI := range children { + childElem := elem.Request.NewElement(childUI) + contents = append(contents, childElem) + if err = childElem.JawsRender(w, nil); err != nil { + break + } } } // Always emit the closing tag, even on a child-render error, to balance @@ -145,27 +145,41 @@ func (u *ContainerHelper) deleteContent(elem *jaws.Element) { u.mu.Unlock() } -// cancelUnusableChild reports whether childUI cannot be used as a container pool -// key — not comparable at runtime, or not equal to itself (a value holding NaN) — -// and, when so, terminates the Request with a cause wrapping [tag.ErrNotUsableAsTag]. -// Callers skip the child so it never becomes a live [jaws.Element]. This mirrors the -// guard in [jaws.Request.NewElement] but runs before the value is hashed as a map key. -func cancelUnusableChild(elem *jaws.Element, childUI jaws.UI) bool { - if err := tag.NewErrNotUsableAsTag(childUI); err != nil { - elem.Request.Cancel(fmt.Errorf("jaws: container child %T is not usable as a UI value: %w", childUI, err)) - return true +// cancelUnusableChildren terminates the Request and reports true if any child cannot +// be used as a container pool key: not comparable at runtime, or not equal to itself +// (a value holding NaN). It aborts on the first such child. +// +// It must be called without holding u.mu. [jaws.Request.Cancel] runs the user logger +// synchronously, and the jaws locking contract forbids that under a lock; a logger +// re-entering the container would otherwise deadlock. Validating the whole slice up +// front also stops the caller creating or rendering later children once the Request +// is terminating. The cancellation cause matches [jaws.NewErrUnusableUI]. +func cancelUnusableChildren(elem *jaws.Element, children []jaws.UI) bool { + for _, childUI := range children { + if err := jaws.NewErrUnusableUI(childUI); err != nil { + elem.Request.Cancel(err) + return true + } } return false } // reconcile matches u.contents to wantContents under u.mu and returns the // Elements to append, the leftover Elements to remove, and the old and new Jid -// orders. Each wanted child is validated with cancelUnusableChild before it is used -// as a pool key, so a value that is non-reflexive or non-comparable at runtime (a -// comparable struct holding e.g. a func in an interface field) terminates the Request -// instead of missing the lookup or panicking. The lock is still released with defer -// as a further guard against a panic leaving u.mu held, mirroring jaws.setDirty. +// orders. +// +// The wanted children are validated with cancelUnusableChildren before u.mu is +// locked, since that terminates the Request through the user logger, which the +// locking contract forbids under a lock. A non-reflexive value (one holding NaN) +// would miss the pool lookup and a runtime-incomparable one would panic hashing it, +// so on the first such child the Request is terminated and reconcile returns nothing +// to append, remove or reorder. The lock is still released with defer as a further +// guard against a panic leaving u.mu held, mirroring jaws.setDirty. func (u *ContainerHelper) reconcile(elem *jaws.Element, wantContents []jaws.UI) (toAppend, toRemove []*jaws.Element, oldOrder, newOrder []jaws.Jid) { + if cancelUnusableChildren(elem, wantContents) { + return + } + u.mu.Lock() defer u.mu.Unlock() @@ -181,13 +195,6 @@ func (u *ContainerHelper) reconcile(elem *jaws.Element, wantContents []jaws.UI) newOrder = make([]jaws.Jid, 0, len(wantContents)) u.contents = u.contents[:0] for _, childUI := range wantContents { - // Validate before childUI is used as a pool key below: a non-reflexive value - // (one holding NaN) would silently miss the lookup, and a runtime-incomparable - // one would panic hashing it. Skipping it here also keeps it out of u.contents, - // so the pool build above never hits an unusable key. - if cancelUnusableChild(elem, childUI) { - continue - } var childElem *jaws.Element // Reuse a pooled Element, discarding any that were deleted out-of-band (a // what.Delete broadcast targeting a tag the child registered, or a browser diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index cb49f091..f9733a55 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -850,7 +850,11 @@ func TestContainerTerminatesOnUnusableChild(t *testing.T) { container := NewContainer("div", tc) elem := rq.NewElement(container) var sb strings.Builder - _ = elem.JawsRender(&sb, nil) + // The render still balances its tags and returns nil; the unusable child is + // skipped and the Request is terminated (asserted via the cancellation cause). + if err := elem.JawsRender(&sb, nil); err != nil { + t.Fatalf("JawsRender err = %v, want nil", err) + } if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) } @@ -869,3 +873,49 @@ func TestContainerTerminatesOnUnusableChild(t *testing.T) { }) } } + +type reentrantLogger struct{ onError func() } + +func (reentrantLogger) Info(string, ...any) {} +func (reentrantLogger) Warn(string, ...any) {} +func (l reentrantLogger) Error(string, ...any) { + if l.onError != nil { + l.onError() + } +} + +// TestContainerCancelNotUnderLock pins that terminating the Request for an unusable +// child does not run while u.mu is held: cancelUnusableChildren runs before reconcile +// locks. Request.Cancel invokes the user logger synchronously, so a logger that +// re-enters the container's lock would deadlock the update goroutine if cancellation +// held u.mu. The timeout turns that regression into a failure instead of a hang. +func TestContainerCancelNotUnderLock(t *testing.T) { + jw, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("ok"))}} + container := NewContainer("div", tc) + elem, _ := renderUI(t, rq, container) + + jw.Logger = reentrantLogger{onError: func() { + // Re-enter the container's lock. If the cancellation that triggered this log + // held u.mu, this second Lock on the same goroutine would deadlock. + container.mu.Lock() + _ = len(container.contents) + container.mu.Unlock() + }} + + tc.contents = []jaws.UI{nanChildUI{f: math.NaN()}} + done := make(chan struct{}) + go func() { + defer close(done) + container.JawsUpdate(elem) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("JawsUpdate deadlocked: Request cancellation ran while holding u.mu") + } + + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } +} diff --git a/lib/ui/input_widgets.go b/lib/ui/input_widgets.go index d0fe4f2d..49386bec 100644 --- a/lib/ui/input_widgets.go +++ b/lib/ui/input_widgets.go @@ -132,6 +132,11 @@ func (u *InputBool) JawsInput(elem *jaws.Element, value string) (err error) { // InputFloat is the reusable base for float64 input widgets. // // A widget embedding InputFloat must back at most one live [jaws.Element]. +// +// The bound value must be finite. A non-finite value (NaN or ±Inf) has no valid +// rendering or wire representation, so rendering it, [InputFloat.JawsUpdate], and +// [InputFloat.JawsInput] cancel the [jaws.Request] with a cause matching +// [jaws.ErrValueNotFinite] rather than coerce it. type InputFloat struct { Input bind.Setter[float64] @@ -163,6 +168,8 @@ func (u *InputFloat) renderFloatInput(elem *jaws.Element, w io.Writer, htmlType } // JawsUpdate updates the input value when the bound float64 value changes. +// +// A non-finite bound value cancels the [jaws.Request]; see [InputFloat]. func (u *InputFloat) JawsUpdate(elem *jaws.Element) { v := u.JawsGet(elem) if !finite(v) { @@ -184,6 +191,10 @@ func (u *InputFloat) JawsUpdate(elem *jaws.Element) { } // JawsInput stores a browser-side float64 input value. +// +// A non-finite value — the "NaN"/"Inf" literals or an overflowing magnitude such as +// "1e999" — cancels the [jaws.Request] (see [InputFloat]) and reports the event +// handled (nil error). func (u *InputFloat) JawsInput(elem *jaws.Element, value string) (err error) { if value == "" { // Empty is a normal in-progress edit state for number/range controls: diff --git a/request.go b/request.go index 49fc9cfb..60ee78af 100644 --- a/request.go +++ b/request.go @@ -4,7 +4,6 @@ import ( "cmp" "context" "errors" - "fmt" "html" "io" "net/http" @@ -625,13 +624,14 @@ func (rq *Request) newElementLocked(ui UI) (elem *Element) { // them. See [UI] for the complete contract. // // If ui is unusable as a map key — not comparable at runtime, or not equal to -// itself, as a value holding [math.NaN] is — the Request is terminated with a cause -// wrapping [tag.ErrNotUsableAsTag]. Such a value cannot be matched reliably during -// element reconciliation, so it is a fatal contract violation rather than something -// to coerce. The Element is still created and returned so callers need not nil-check. +// itself, as a value holding NaN is — the Request is terminated with a cause matching +// [tag.ErrNotUsableAsTag] (see [NewErrUnusableUI]). Such a value cannot be matched +// reliably during element reconciliation, so it is a fatal contract violation rather +// than something to coerce. The Element is still created and returned so callers need +// not nil-check. func (rq *Request) NewElement(ui UI) *Element { - if err := tag.NewErrNotUsableAsTag(ui); err != nil { - rq.Cancel(fmt.Errorf("jaws: %T is not usable as a UI value: %w", ui, err)) + if err := NewErrUnusableUI(ui); err != nil { + rq.Cancel(err) } rq.mu.Lock() defer rq.mu.Unlock() From ae55a3d4f1bb2783d674660c6cd9946e23618c88 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Mon, 20 Jul 2026 23:22:03 +0200 Subject: [PATCH 4/8] =?UTF-8?q?fix:=20address=20review=20round=203=20?= =?UTF-8?q?=E2=80=94=20nil=20UI,=20error=20wording,=20and=20append-heavy?= =?UTF-8?q?=20regression?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - NewErrUnusableUI(nil) returned nil, so a nil UI passed container validation and later panicked in JawsRender. Reject nil explicitly (clear "nil is not usable" message), and make Element.JawsRender/JawsUpdate treat a nil UI as a no-op so a direct NewElement(nil) yields a harmless Element rather than panicking. Adds a validator table test (nil / NaN / statically-incomparable / runtime-incomparable / valid, matching both tag identities), a direct nil regression, and a container nil child case. - errUnusableUI only implements Is, not Unwrap, so the cause does not "wrap" tag.ErrNotUsableAsTag. Reword the UI contract to say it "matches ... with errors.Is", and name tag.ErrNotUsableAsTag (not the NewErrUnusableUI constructor) in the container comment. - Fix the append-heavy benchmark regression. The all-build guard in NewElement was a round-0 over-generalization: a raw UI value is used as a map key only in the container pool (tags go through TagExpand; direct elements are keyed by Jid and never reconciled), so the container is the correct and sufficient guard. Restore NewElement's original debug-only comparability assertion, leaving the container's pre-lock scan as the sole all-build guard, and make that scan cheap (one deferred recover for the whole slice, a plain self-comparison per child). This removes the per-child double validation. benchstat (Apple M5 Max, count=8) vs the regressed state: AppendHeavy -3.34%, Mixed -12.13%; AppendHeavy is now statistically level with the no-validation floor (123.4µs vs 122.3µs, p=0.41). Allocations unchanged. - Strengthen the reentrant-logger deadlock test to assert the logger callback ran, so the check cannot pass vacuously. Update contract docs and the JAWS skill to describe the container-level guard and NewElement's debug-only assertion. --- .agents/skills/jaws/SKILL.md | 7 +-- contracts.go | 15 ++++--- element.go | 13 ++++-- element_test.go | 25 +---------- errunusableui.go | 20 ++++----- errunusableui_test.go | 79 ++++++++++++++++++++++++++++++++++ lib/ui/containerhelper.go | 36 +++++++++++++--- lib/ui/containerhelper_test.go | 9 ++++ request.go | 18 ++++---- request_test.go | 21 +++++---- 10 files changed, 173 insertions(+), 70 deletions(-) create mode 100644 errunusableui_test.go diff --git a/.agents/skills/jaws/SKILL.md b/.agents/skills/jaws/SKILL.md index bcf2d582..62e049b5 100644 --- a/.agents/skills/jaws/SKILL.md +++ b/.agents/skills/jaws/SKILL.md @@ -37,9 +37,10 @@ JaWS is an immediate-mode, server-driven UI framework, not an MVC framework. - Every JaWS `UI` value must be comparable at runtime **and equal to itself**, since it is used as a map key. A value that is only statically comparable (an interface - field holding a slice/map/func) or that holds a `NaN` (so `v != v`) is rejected: - `Request.NewElement` and the container widgets cancel the `Request` in all builds, - with a cause matching `tag.ErrNotUsableAsTag`, rather than silently accepting it. + field holding a slice/map/func) or that holds a `NaN` (so `v != v`) is rejected: the + container widgets — the only place a raw `UI` value is used as a map key — cancel the + `Request` in all builds (cause matches `tag.ErrNotUsableAsTag`), and + `Request.NewElement` asserts runtime comparability in debug builds. - Every JaWS `UI` value is request-scoped. Once used by one Request, never use that value with another Request; construct fresh widgets per request. The widgets may still refer to shared, synchronized application state, binders, diff --git a/contracts.go b/contracts.go index cf949492..3944a526 100644 --- a/contracts.go +++ b/contracts.go @@ -69,13 +69,14 @@ type TemplateLookuper interface { // synchronized as required. // // In addition, all UI objects must be comparable and equal to themselves, so they -// can be used as map keys. The compile-time type must be comparable; -// [Request.NewElement] additionally performs a runtime value-level check in every -// build and cancels the Request when the value is not comparable at runtime (for -// example a comparable struct holding a func in an interface field) or not equal to -// itself (a value holding NaN). The cancellation cause wraps -// [github.com/linkdata/jaws/lib/tag.ErrNotUsableAsTag]. Callers must therefore -// ensure UI values are genuinely comparable and reflexive. +// can be used as map keys. The compile-time type must be comparable; the container +// widgets additionally check each child value at runtime and cancel the Request, in +// every build, when it is not comparable at runtime (for example a comparable struct +// holding a func in an interface field) or not equal to itself (a value holding NaN), +// with a cause matching [github.com/linkdata/jaws/lib/tag.ErrNotUsableAsTag] under +// errors.Is. That is the only place a raw UI value is used as a map key; outside a +// container [Request.NewElement] asserts runtime comparability in debug builds. +// Callers must ensure UI values are genuinely comparable and reflexive. type UI interface { Renderer Updater diff --git a/element.go b/element.go index cdb2a03c..0b6801f5 100644 --- a/element.go +++ b/element.go @@ -148,9 +148,12 @@ var debugCommentSanitizer = strings.NewReplacer("-->", "==>", "--!>", "==>") // JawsRender calls [Renderer.JawsRender] for this [Element]. // // Do not call this yourself unless it is from within another JawsRender implementation. +// +// A nil [UI] renders as a no-op; this can only arise from [Request.NewElement] having +// been given a nil ui. func (elem *Element) JawsRender(w io.Writer, params []any) (err error) { - if !elem.deleted.Load() { - if err = elem.UI().JawsRender(elem, w, params); err == nil { + if ui := elem.UI(); ui != nil && !elem.deleted.Load() { + if err = ui.JawsRender(elem, w, params); err == nil { if elem.Jaws.Debug { err = elem.renderDebug(w) } @@ -166,9 +169,11 @@ func (elem *Element) JawsRender(w io.Writer, params []any) (err error) { // JawsUpdate calls [Updater.JawsUpdate] for this [Element]. // // Do not call this yourself unless it is from within another JawsUpdate implementation. +// +// A nil [UI] is a no-op (see [Element.JawsRender]). func (elem *Element) JawsUpdate() { - if !elem.deleted.Load() { - elem.UI().JawsUpdate(elem) + if ui := elem.UI(); ui != nil && !elem.deleted.Load() { + ui.JawsUpdate(elem) } } diff --git a/element_test.go b/element_test.go index 4d4351ea..10b656b5 100644 --- a/element_test.go +++ b/element_test.go @@ -2,13 +2,11 @@ package jaws import ( "bytes" - "context" "errors" "fmt" "html/template" "io" "log/slog" - "math" "net/http" "net/http/httptest" "reflect" @@ -1224,28 +1222,9 @@ func TestElement_JawsInit(t *testing.T) { } } +// nonReflexiveUI is a comparable UI value that is not equal to itself when it holds +// NaN; it backs TestNewErrUnusableUI. type nonReflexiveUI struct{ f float64 } func (nonReflexiveUI) JawsRender(*Element, io.Writer, []any) error { return nil } func (nonReflexiveUI) JawsUpdate(*Element) {} - -// TestNewElementTerminatesOnNonReflexiveUI verifies that a UI value that is not equal -// to itself (a comparable struct holding NaN) terminates the Request and logs the -// cause, rather than being silently accepted and later corrupting map-key lookups. -func TestNewElementTerminatesOnNonReflexiveUI(t *testing.T) { - tj := newTestJaws() - t.Cleanup(tj.Close) - rq := newWrappedTestRequest(tj.Jaws, nil) - if rq == nil { - t.Fatal("nil request") - } - - rq.NewElement(nonReflexiveUI{f: math.NaN()}) - - if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { - t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) - } - if !strings.Contains(tj.log.String(), "not usable as a jaws.UI value") { - t.Fatalf("expected termination to be logged, got %q", tj.log.String()) - } -} diff --git a/errunusableui.go b/errunusableui.go index 63f10376..1cfc186b 100644 --- a/errunusableui.go +++ b/errunusableui.go @@ -7,21 +7,21 @@ import ( ) // errUnusableUI reports that a value cannot be used as a [UI] value because it is -// not comparable at runtime, or not equal to itself as a value holding NaN is. +// nil, not comparable at runtime, or not equal to itself as a value holding NaN is. // // It matches [tag.ErrNotUsableAsTag] and [tag.ErrNotComparable] with errors.Is, but // carries UI-specific text: the tag package's advice to implement JawsGetTag cannot // make a raw UI value usable as a map[UI] key, so it must not be surfaced here. type errUnusableUI struct { - t reflect.Type + t reflect.Type // nil when the offending value was a nil UI } func (e errUnusableUI) Error() (s string) { - s = "value" + s = "nil" if e.t != nil { s = e.t.String() } - return s + " is not usable as a jaws.UI value: it must be comparable and equal to itself" + return s + " is not usable as a jaws.UI value: it must be non-nil, comparable, and equal to itself" } // Is reports whether target is a tag sentinel this error stands in for, so callers @@ -33,13 +33,13 @@ func (errUnusableUI) Is(target error) bool { // NewErrUnusableUI returns a non-nil error when ui cannot be used as a [UI] value, // and nil when it can. // -// A UI value is unusable when it is not comparable at runtime, or not equal to itself -// as a value holding NaN is, because it is used as a map key and would either panic -// or fail to match. The returned error matches [tag.ErrNotUsableAsTag] with errors.Is. -// [Request.NewElement] and the container widgets use it to terminate a Request handed -// such a value. +// A UI value is unusable when it is nil, not comparable at runtime, or not equal to +// itself as a value holding NaN is, because it is used as a map key: a nil or +// non-comparable value would panic and a NaN-bearing one would fail to match. The +// returned error matches [tag.ErrNotUsableAsTag] with errors.Is. The container widgets +// use it to terminate a Request handed such a child. func NewErrUnusableUI(ui UI) error { - if tag.NewErrNotUsableAsTag(ui) != nil { + if ui == nil || tag.NewErrNotUsableAsTag(ui) != nil { return errUnusableUI{t: reflect.TypeOf(ui)} } return nil diff --git a/errunusableui_test.go b/errunusableui_test.go new file mode 100644 index 00000000..d4699331 --- /dev/null +++ b/errunusableui_test.go @@ -0,0 +1,79 @@ +package jaws + +import ( + "context" + "errors" + "io" + "math" + "strings" + "testing" + + "github.com/linkdata/jaws/lib/tag" +) + +// ifaceSliceUI is statically comparable (an interface field) but panics when compared +// at runtime, since the interface holds a slice. +type ifaceSliceUI struct{ v any } + +func (ifaceSliceUI) JawsRender(*Element, io.Writer, []any) error { return nil } +func (ifaceSliceUI) JawsUpdate(*Element) {} + +func TestNewErrUnusableUI(t *testing.T) { + tests := []struct { + name string + ui UI + wantErr bool + }{ + {"nil", nil, true}, + {"nan struct", nonReflexiveUI{f: math.NaN()}, true}, + {"map field (statically incomparable)", testUnhashableUI{m: map[string]int{"x": 1}}, true}, + {"interface holding slice (runtime-incomparable)", ifaceSliceUI{v: []int{1}}, true}, + {"valid pointer", &testUi{}, false}, + {"valid struct", nonReflexiveUI{f: 1.5}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := NewErrUnusableUI(tt.ui) + if !tt.wantErr { + if err != nil { + t.Fatalf("NewErrUnusableUI = %v, want nil", err) + } + return + } + if err == nil { + t.Fatal("NewErrUnusableUI = nil, want error") + } + // The error stands in for both tag identities so callers can match either. + if !errors.Is(err, tag.ErrNotUsableAsTag) { + t.Errorf("err does not match tag.ErrNotUsableAsTag: %v", err) + } + if !errors.Is(err, tag.ErrNotComparable) { + t.Errorf("err does not match tag.ErrNotComparable: %v", err) + } + }) + } +} + +// TestNewElementNilUIRendersNoop verifies that NewElement(nil) does not terminate the +// Request — a nil UI is never reconciled by a container, so it is harmless — and +// returns an Element that renders and updates as a no-op rather than panicking on the +// nil UI. A nil child returned from a container is instead rejected; see +// TestContainerTerminatesOnUnusableChild. +func TestNewElementNilUIRendersNoop(t *testing.T) { + rq := newTestRequest(t) + defer rq.Close() + + elem := rq.NewElement(nil) + if cause := context.Cause(rq.Context()); cause != nil { + t.Fatalf("NewElement(nil) cancelled the Request: %v", cause) + } + + var sb strings.Builder + if err := elem.JawsRender(&sb, nil); err != nil { + t.Fatalf("JawsRender err = %v, want nil", err) + } + if sb.Len() != 0 { + t.Fatalf("nil-UI render wrote %q, want empty", sb.String()) + } + elem.JawsUpdate() // must not panic +} diff --git a/lib/ui/containerhelper.go b/lib/ui/containerhelper.go index de3091b0..71808a96 100644 --- a/lib/ui/containerhelper.go +++ b/lib/ui/containerhelper.go @@ -146,22 +146,44 @@ func (u *ContainerHelper) deleteContent(elem *jaws.Element) { } // cancelUnusableChildren terminates the Request and reports true if any child cannot -// be used as a container pool key: not comparable at runtime, or not equal to itself -// (a value holding NaN). It aborts on the first such child. +// be used as a container pool key: nil, not comparable at runtime, or not equal to +// itself (a value holding NaN). It aborts on the first such child. // // It must be called without holding u.mu. [jaws.Request.Cancel] runs the user logger // synchronously, and the jaws locking contract forbids that under a lock; a logger // re-entering the container would otherwise deadlock. Validating the whole slice up // front also stops the caller creating or rendering later children once the Request -// is terminating. The cancellation cause matches [jaws.NewErrUnusableUI]. +// is terminating. The cancellation cause matches tag.ErrNotUsableAsTag with +// errors.Is (see [jaws.NewErrUnusableUI]). func cancelUnusableChildren(elem *jaws.Element, children []jaws.UI) bool { + if bad, ok := firstUnusableChild(children); ok { + elem.Request.Cancel(jaws.NewErrUnusableUI(bad)) + return true + } + return false +} + +// firstUnusableChild returns the first child that is nil, not equal to itself, or not +// comparable at runtime, and whether one was found. +// +// A single deferred recover guards the whole scan, so a usable child costs only one +// self-comparison rather than a per-child deferred check: comparing a +// runtime-incomparable value panics, which the recover attributes to the child being +// examined (bad). This keeps the common all-usable case cheap on the container update +// hot path. +func firstUnusableChild(children []jaws.UI) (bad jaws.UI, found bool) { + defer func() { + if recover() != nil { + found = true // comparing bad panicked: not comparable at runtime + } + }() for _, childUI := range children { - if err := jaws.NewErrUnusableUI(childUI); err != nil { - elem.Request.Cancel(err) - return true + bad = childUI + if childUI == nil || childUI != childUI { + return childUI, true } } - return false + return nil, false } // reconcile matches u.contents to wantContents under u.mu and returns the diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index f9733a55..80fdc8e8 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -840,6 +841,7 @@ func TestContainerTerminatesOnUnusableChild(t *testing.T) { name string make func() jaws.UI }{ + {"nil", func() jaws.UI { return nil }}, {"nan", func() jaws.UI { return nanChildUI{f: math.NaN()} }}, {"incomparable", func() jaws.UI { return incomparableChildUI{v: []int{1}} }}, } @@ -895,12 +897,14 @@ func TestContainerCancelNotUnderLock(t *testing.T) { container := NewContainer("div", tc) elem, _ := renderUI(t, rq, container) + var reentered atomic.Bool jw.Logger = reentrantLogger{onError: func() { // Re-enter the container's lock. If the cancellation that triggered this log // held u.mu, this second Lock on the same goroutine would deadlock. container.mu.Lock() _ = len(container.contents) container.mu.Unlock() + reentered.Store(true) }} tc.contents = []jaws.UI{nanChildUI{f: math.NaN()}} @@ -915,6 +919,11 @@ func TestContainerCancelNotUnderLock(t *testing.T) { t.Fatal("JawsUpdate deadlocked: Request cancellation ran while holding u.mu") } + // Without this the deadlock check is vacuous: if the logger never ran, no re-entry + // was attempted and the test would pass even with cancellation under the lock. + if !reentered.Load() { + t.Fatal("logger callback did not run; the deadlock check would be vacuous") + } if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) } diff --git a/request.go b/request.go index 60ee78af..b73501e1 100644 --- a/request.go +++ b/request.go @@ -623,15 +623,17 @@ func (rq *Request) newElementLocked(ui UI) (elem *Element) { // multiplicity requirements are caller obligations; NewElement does not enforce // them. See [UI] for the complete contract. // -// If ui is unusable as a map key — not comparable at runtime, or not equal to -// itself, as a value holding NaN is — the Request is terminated with a cause matching -// [tag.ErrNotUsableAsTag] (see [NewErrUnusableUI]). Such a value cannot be matched -// reliably during element reconciliation, so it is a fatal contract violation rather -// than something to coerce. The Element is still created and returned so callers need -// not nil-check. +// ui must be usable as a map key — comparable at runtime and equal to itself (see +// [NewErrUnusableUI]) — because the container widgets key their children by it. Those +// widgets validate their children and terminate the Request on an unusable one before +// it reaches a map, so NewElement does not re-validate on this hot path: debug builds +// panic on a runtime-incomparable ui as a development assertion, and a nil ui yields +// an Element that renders and updates as a no-op (see [Element.JawsRender]). func (rq *Request) NewElement(ui UI) *Element { - if err := NewErrUnusableUI(ui); err != nil { - rq.Cancel(err) + if deadlock.Debug { + if err := tag.NewErrNotComparable(ui); err != nil { + panic(err) + } } rq.mu.Lock() defer rq.mu.Unlock() diff --git a/request_test.go b/request_test.go index c8fdf487..36152359 100644 --- a/request_test.go +++ b/request_test.go @@ -2444,18 +2444,23 @@ func nextOutboundMsg(t *testing.T, rq *testRequest) wire.WsMsg { } } -func TestRequest_NewElement_TerminatesOnNonComparableUI(t *testing.T) { +func TestRequest_NewElement_DebugComparableCheck(t *testing.T) { + if !deadlock.Debug { + t.Skip("debug checks not enabled") + } + rq := newTestRequest(t) defer rq.Close() - // A UI value that is not comparable at runtime cannot be used as a map key; like a - // non-reflexive value it terminates the Request rather than being accepted and - // panicking later at first map use. + // NewElement asserts runtime comparability in debug builds. The all-build guard + // against unusable UI values lives in the container widgets (see the ui package), + // since that is the only place a raw UI value is used as a map key. + defer func() { + if recover() == nil { + t.Fatal("expected panic for non-comparable UI when comparable check is enabled") + } + }() rq.NewElement(testUnhashableUI{m: map[string]int{"x": 1}}) - - if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { - t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) - } } func TestRequest_getElementByJidLocked_DebugUnsortedPanics(t *testing.T) { From efb4e92407ac8d13fd1d201cba91394bf57ee83e Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Tue, 21 Jul 2026 09:00:57 +0200 Subject: [PATCH 5/8] =?UTF-8?q?fix:=20address=20review=20round=204=20?= =?UTF-8?q?=E2=80=94=20typed-nil=20UI=20semantics,=20nil=20docs,=20focused?= =?UTF-8?q?=20benchmark?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Clarify that only a nil interface is special. A typed nil (e.g. (*Widget)(nil)) is comparable and equal to itself, so NewErrUnusableUI reports it usable and the containers reconcile it normally; whether its methods tolerate a nil receiver is the UI implementation's responsibility, consistent with how the tag system treats typed-nil tags. Element.JawsRender only no-ops a nil interface — a typed nil still dispatches to its Renderer. New tests cover the typed-nil path (direct dispatch and container acceptance) alongside the nil-interface cases. - Align the nil documentation with the behavior: a nil interface is a legal map key that does not panic; containers reject it because it has no methods to render, while a direct NewElement(nil) tolerates it as a no-op. Describe this in the UI and JawsContains contracts and the JAWS skill. - Add a focused, allocation-free validation-scan benchmark (BenchmarkContainerValidateChildren). The broad container benchmarks are dominated by rendering and allocation and too noisy to show the scan cost; the focused benchmark measures it directly (~1.5ns per child, 0 allocs). A one-off comparison showed the single-deferred-recover scan is ~63% faster than per-child validation, but end-to-end container updates are statistically level with main: this is a no-regression change, not a claimed end-to-end speedup. --- .agents/skills/jaws/SKILL.md | 14 ++++++----- contracts.go | 30 +++++++++++++---------- element.go | 5 ++-- errunusableui.go | 21 ++++++++++------ errunusableui_test.go | 43 +++++++++++++++++++++++++++++++++ lib/ui/containerhelper_test.go | 44 ++++++++++++++++++++++++++++++++++ request.go | 4 ++-- 7 files changed, 131 insertions(+), 30 deletions(-) diff --git a/.agents/skills/jaws/SKILL.md b/.agents/skills/jaws/SKILL.md index 62e049b5..4c5a1c6e 100644 --- a/.agents/skills/jaws/SKILL.md +++ b/.agents/skills/jaws/SKILL.md @@ -35,12 +35,14 @@ JaWS is an immediate-mode, server-driven UI framework, not an MVC framework. ## Hard framework constraints -- Every JaWS `UI` value must be comparable at runtime **and equal to itself**, since - it is used as a map key. A value that is only statically comparable (an interface - field holding a slice/map/func) or that holds a `NaN` (so `v != v`) is rejected: the - container widgets — the only place a raw `UI` value is used as a map key — cancel the - `Request` in all builds (cause matches `tag.ErrNotUsableAsTag`), and - `Request.NewElement` asserts runtime comparability in debug builds. +- Every JaWS `UI` value must be a non-nil interface, comparable at runtime, **and + equal to itself**, since it is used as a map key. A value that is a nil interface, + only statically comparable (an interface field holding a slice/map/func), or that + holds a `NaN` (so `v != v`) is rejected: the container widgets — the only place a raw + `UI` value is used as a map key — cancel the `Request` in all builds (cause matches + `tag.ErrNotUsableAsTag`), and `Request.NewElement` asserts runtime comparability in + debug builds. A typed nil (e.g. `(*Widget)(nil)`) is accepted as usable; its methods + must tolerate a nil receiver. - Every JaWS `UI` value is request-scoped. Once used by one Request, never use that value with another Request; construct fresh widgets per request. The widgets may still refer to shared, synchronized application state, binders, diff --git a/contracts.go b/contracts.go index 3944a526..88241e10 100644 --- a/contracts.go +++ b/contracts.go @@ -12,13 +12,14 @@ type Container interface { // JawsContains returns the current child [UI] values contained by elem. // // The returned [UI] values must be comparable and equal to themselves, since they - // are used as map keys (see [UI] for the requirement); a value that is not, such - // as one holding NaN, cancels the [Request] instead of being reconciled. The slice - // contents must not be modified after returning it. Returning such a child UI again - // from a later call lets the container reuse its existing live [Element]. The same - // UI may occur more than once in one returned slice only when its type documents - // support for backing multiple live Elements. A child UI must not be shared with a - // different [Request]. + // are used as map keys (see [UI] for the requirement); a child that is a nil + // interface, not comparable at runtime, or not equal to itself (such as one holding + // NaN) cancels the [Request] instead of being reconciled. A typed nil is usable. + // The slice contents must not be modified after returning it. Returning a usable + // child UI again from a later call lets the container reuse its existing live + // [Element]. The same UI may occur more than once in one returned slice only when + // its type documents support for backing multiple live Elements. A child UI must + // not be shared with a different [Request]. JawsContains(elem *Element) (contents []UI) } @@ -71,12 +72,15 @@ type TemplateLookuper interface { // In addition, all UI objects must be comparable and equal to themselves, so they // can be used as map keys. The compile-time type must be comparable; the container // widgets additionally check each child value at runtime and cancel the Request, in -// every build, when it is not comparable at runtime (for example a comparable struct -// holding a func in an interface field) or not equal to itself (a value holding NaN), -// with a cause matching [github.com/linkdata/jaws/lib/tag.ErrNotUsableAsTag] under -// errors.Is. That is the only place a raw UI value is used as a map key; outside a -// container [Request.NewElement] asserts runtime comparability in debug builds. -// Callers must ensure UI values are genuinely comparable and reflexive. +// every build, when it is a nil interface, not comparable at runtime (for example a +// comparable struct holding a func in an interface field), or not equal to itself (a +// value holding NaN), with a cause matching +// [github.com/linkdata/jaws/lib/tag.ErrNotUsableAsTag] under errors.Is. That is the +// only place a raw UI value is used as a map key; outside a container +// [Request.NewElement] asserts runtime comparability in debug builds. A typed nil (a +// non-nil interface holding a nil pointer) is usable, and tolerating a nil receiver +// is the concrete type's responsibility. Callers must ensure UI values are genuinely +// comparable and reflexive. type UI interface { Renderer Updater diff --git a/element.go b/element.go index 0b6801f5..60bb1bc6 100644 --- a/element.go +++ b/element.go @@ -149,8 +149,9 @@ var debugCommentSanitizer = strings.NewReplacer("-->", "==>", "--!>", "==>") // // Do not call this yourself unless it is from within another JawsRender implementation. // -// A nil [UI] renders as a no-op; this can only arise from [Request.NewElement] having -// been given a nil ui. +// A nil [UI] interface renders as a no-op; this arises only from [Request.NewElement] +// given a nil interface. A typed nil (a non-nil interface holding a nil pointer) still +// dispatches to its [Renderer], which is responsible for tolerating a nil receiver. func (elem *Element) JawsRender(w io.Writer, params []any) (err error) { if ui := elem.UI(); ui != nil && !elem.deleted.Load() { if err = ui.JawsRender(elem, w, params); err == nil { diff --git a/errunusableui.go b/errunusableui.go index 1cfc186b..53269b3b 100644 --- a/errunusableui.go +++ b/errunusableui.go @@ -6,8 +6,9 @@ import ( "github.com/linkdata/jaws/lib/tag" ) -// errUnusableUI reports that a value cannot be used as a [UI] value because it is -// nil, not comparable at runtime, or not equal to itself as a value holding NaN is. +// errUnusableUI reports that a value cannot be used as a [UI] value because it is a +// nil interface, not comparable at runtime, or not equal to itself as a value holding +// NaN is. // // It matches [tag.ErrNotUsableAsTag] and [tag.ErrNotComparable] with errors.Is, but // carries UI-specific text: the tag package's advice to implement JawsGetTag cannot @@ -33,11 +34,17 @@ func (errUnusableUI) Is(target error) bool { // NewErrUnusableUI returns a non-nil error when ui cannot be used as a [UI] value, // and nil when it can. // -// A UI value is unusable when it is nil, not comparable at runtime, or not equal to -// itself as a value holding NaN is, because it is used as a map key: a nil or -// non-comparable value would panic and a NaN-bearing one would fail to match. The -// returned error matches [tag.ErrNotUsableAsTag] with errors.Is. The container widgets -// use it to terminate a Request handed such a child. +// A UI value is unusable when it is a nil interface, not comparable at runtime, or not +// equal to itself as a value holding NaN is. Containers use it both as a map key and +// to render children: a non-comparable value panics when hashed and a NaN-bearing one +// never matches itself, while a nil interface is a legal map key but has no methods to +// render. A typed nil — a non-nil interface holding a nil pointer whose type +// implements [UI] — is comparable and equal to itself, so it is reported usable; +// whether its [Renderer] tolerates a nil receiver is the concrete type's +// responsibility. +// +// The returned error matches [tag.ErrNotUsableAsTag] with errors.Is. The container +// widgets use it to terminate a Request handed such a child. func NewErrUnusableUI(ui UI) error { if ui == nil || tag.NewErrNotUsableAsTag(ui) != nil { return errUnusableUI{t: reflect.TypeOf(ui)} diff --git a/errunusableui_test.go b/errunusableui_test.go index d4699331..83103780 100644 --- a/errunusableui_test.go +++ b/errunusableui_test.go @@ -18,6 +18,20 @@ type ifaceSliceUI struct{ v any } func (ifaceSliceUI) JawsRender(*Element, io.Writer, []any) error { return nil } func (ifaceSliceUI) JawsUpdate(*Element) {} +// typedNilUI has pointer-receiver methods that tolerate a nil receiver, so a typed nil +// (*typedNilUI)(nil) is a usable UI value that renders without dereferencing. +type typedNilUI struct{ s string } + +func (u *typedNilUI) JawsRender(_ *Element, w io.Writer, _ []any) error { + s := "typednil" + if u != nil { + s = u.s + } + _, err := io.WriteString(w, s) + return err +} +func (*typedNilUI) JawsUpdate(*Element) {} + func TestNewErrUnusableUI(t *testing.T) { tests := []struct { name string @@ -30,6 +44,9 @@ func TestNewErrUnusableUI(t *testing.T) { {"interface holding slice (runtime-incomparable)", ifaceSliceUI{v: []int{1}}, true}, {"valid pointer", &testUi{}, false}, {"valid struct", nonReflexiveUI{f: 1.5}, false}, + // A typed nil is comparable and equal to itself, so it is usable; only a nil + // interface is rejected. + {"typed nil pointer", (*typedNilUI)(nil), false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -77,3 +94,29 @@ func TestNewElementNilUIRendersNoop(t *testing.T) { } elem.JawsUpdate() // must not panic } + +// TestNewElementTypedNilUIDispatchesToRenderer documents that a typed nil UI (a +// non-nil interface holding a nil pointer) is usable — comparable and equal to itself +// — and dispatches to its Renderer rather than being treated as unusable. Tolerating +// the nil receiver is the concrete type's responsibility; typedNilUI does so. +func TestNewElementTypedNilUIDispatchesToRenderer(t *testing.T) { + rq := newTestRequest(t) + defer rq.Close() + + var ui UI = (*typedNilUI)(nil) + if err := NewErrUnusableUI(ui); err != nil { + t.Fatalf("NewErrUnusableUI(typed nil) = %v, want nil (usable)", err) + } + + elem := rq.NewElement(ui) + if cause := context.Cause(rq.Context()); cause != nil { + t.Fatalf("NewElement(typed nil) cancelled the Request: %v", cause) + } + var sb strings.Builder + if err := elem.JawsRender(&sb, nil); err != nil { + t.Fatalf("JawsRender err = %v", err) + } + if sb.String() != "typednil" { + t.Fatalf("render = %q, want %q", sb.String(), "typednil") + } +} diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index 80fdc8e8..1e72d44c 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -503,6 +503,22 @@ func benchChildren(start, count int) []jaws.UI { return contents } +// BenchmarkContainerValidateChildren isolates the pre-lock validation scan that runs +// on every container render and update. It is the path affected by scoping the +// unusable-UI guard to the container: a usable child must cost only one +// self-comparison (a single deferred recover guards the whole slice) and no +// allocation. The broad Update benchmarks are dominated by rendering and allocation, +// so this focused benchmark is the meaningful guard for the scan's cost. +func BenchmarkContainerValidateChildren(b *testing.B) { + b.ReportAllocs() + children := benchChildren(0, 1000) + for range b.N { + if _, ok := firstUnusableChild(children); ok { + b.Fatal("unexpected unusable child") + } + } +} + func BenchmarkContainerHelperUpdateAppendHeavy(b *testing.B) { b.ReportAllocs() const size = 1000 @@ -830,6 +846,34 @@ func (incomparableChildUI) JawsRender(_ *jaws.Element, w io.Writer, _ []any) err } func (incomparableChildUI) JawsUpdate(*jaws.Element) {} +// typedNilChildUI has pointer-receiver methods that tolerate a nil receiver, so a +// typed nil (*typedNilChildUI)(nil) is a usable, reflexive child. +type typedNilChildUI struct{} + +func (*typedNilChildUI) JawsRender(_ *jaws.Element, w io.Writer, _ []any) error { + _, err := io.WriteString(w, "child") + return err +} +func (*typedNilChildUI) JawsUpdate(*jaws.Element) {} + +// TestContainerAcceptsTypedNilChild documents that a typed nil child (a non-nil +// interface holding a nil pointer) is usable — comparable and equal to itself — so the +// container reconciles it normally rather than terminating the Request. Only a nil +// interface child is rejected. +func TestContainerAcceptsTypedNilChild(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{(*typedNilChildUI)(nil)}} + container := NewContainer("div", tc) + elem, _ := renderUI(t, rq, container) + if cause := context.Cause(rq.Context()); cause != nil { + t.Fatalf("render cancelled the Request: %v", cause) + } + container.JawsUpdate(elem) + if cause := context.Cause(rq.Context()); cause != nil { + t.Fatalf("update cancelled the Request: %v", cause) + } +} + // TestContainerTerminatesOnUnusableChild covers issue #179 and its // runtime-incomparable sibling: a container child UI that is not equal to itself // (holds NaN) or not comparable at runtime (holds an interface-wrapped slice) cannot diff --git a/request.go b/request.go index b73501e1..c9bc1386 100644 --- a/request.go +++ b/request.go @@ -627,8 +627,8 @@ func (rq *Request) newElementLocked(ui UI) (elem *Element) { // [NewErrUnusableUI]) — because the container widgets key their children by it. Those // widgets validate their children and terminate the Request on an unusable one before // it reaches a map, so NewElement does not re-validate on this hot path: debug builds -// panic on a runtime-incomparable ui as a development assertion, and a nil ui yields -// an Element that renders and updates as a no-op (see [Element.JawsRender]). +// panic on a runtime-incomparable ui as a development assertion, and a nil interface +// ui yields an Element that renders and updates as a no-op (see [Element.JawsRender]). func (rq *Request) NewElement(ui UI) *Element { if deadlock.Debug { if err := tag.NewErrNotComparable(ui); err != nil { From 5e6c0ea3d231979dc393ba0282ffe26652b67c76 Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Tue, 21 Jul 2026 09:22:38 +0200 Subject: [PATCH 6/8] =?UTF-8?q?fix:=20address=20review=20round=205=20?= =?UTF-8?q?=E2=80=94=20benchmark=20honesty,=20doc=20precision,=20stronger?= =?UTF-8?q?=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add b.ResetTimer() after fixture creation in BenchmarkContainerValidateChildren so the one-time benchChildren allocation is excluded; -benchtime=1x now reports 0 B/op and 0 allocs instead of the setup cost. - Documentation precision: Element.JawsUpdate now says "nil UI interface" and notes a typed nil dispatches to its Updater; NewErrUnusableUI documents both errors.Is identities (tag.ErrNotUsableAsTag and tag.ErrNotComparable) and the direct NewElement(nil) exception (tolerated as a no-op, so the error is reported only for the container's benefit). - Strengthen the typed-nil container test to assert the rendered output and that the child's Element and Jid are reused across an update (a stable reflexive key does not churn). Add TestContainerValidatesWholeSliceBeforeRender: a usable child preceding an unusable one is neither rendered nor committed, pinning that the pre-lock scan validates the whole slice before any child work. --- element.go | 3 +- errunusableui.go | 6 ++-- lib/ui/containerhelper_test.go | 50 ++++++++++++++++++++++++++++++++-- 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/element.go b/element.go index 60bb1bc6..550daeab 100644 --- a/element.go +++ b/element.go @@ -171,7 +171,8 @@ func (elem *Element) JawsRender(w io.Writer, params []any) (err error) { // // Do not call this yourself unless it is from within another JawsUpdate implementation. // -// A nil [UI] is a no-op (see [Element.JawsRender]). +// A nil [UI] interface is a no-op; a typed nil dispatches to its [Updater] (see +// [Element.JawsRender]). func (elem *Element) JawsUpdate() { if ui := elem.UI(); ui != nil && !elem.deleted.Load() { ui.JawsUpdate(elem) diff --git a/errunusableui.go b/errunusableui.go index 53269b3b..67acff1a 100644 --- a/errunusableui.go +++ b/errunusableui.go @@ -43,8 +43,10 @@ func (errUnusableUI) Is(target error) bool { // whether its [Renderer] tolerates a nil receiver is the concrete type's // responsibility. // -// The returned error matches [tag.ErrNotUsableAsTag] with errors.Is. The container -// widgets use it to terminate a Request handed such a child. +// The returned error matches both [tag.ErrNotUsableAsTag] and [tag.ErrNotComparable] +// under errors.Is. The container widgets use it to terminate a Request handed such a +// child; a nil interface passed directly to [Request.NewElement] is instead tolerated +// as a no-op Element, so this reports it unusable only for the container's benefit. func NewErrUnusableUI(ui UI) error { if ui == nil || tag.NewErrNotUsableAsTag(ui) != nil { return errUnusableUI{t: reflect.TypeOf(ui)} diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index 1e72d44c..c7aae2b4 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -512,6 +512,7 @@ func benchChildren(start, count int) []jaws.UI { func BenchmarkContainerValidateChildren(b *testing.B) { b.ReportAllocs() children := benchChildren(0, 1000) + b.ResetTimer() // exclude the one-time fixture allocation so -benchtime=1x is honest for range b.N { if _, ok := firstUnusableChild(children); ok { b.Fatal("unexpected unusable child") @@ -858,20 +859,63 @@ func (*typedNilChildUI) JawsUpdate(*jaws.Element) {} // TestContainerAcceptsTypedNilChild documents that a typed nil child (a non-nil // interface holding a nil pointer) is usable — comparable and equal to itself — so the -// container reconciles it normally rather than terminating the Request. Only a nil -// interface child is rejected. +// container renders it and, because it is a stable reflexive map key, reuses the same +// Element across updates rather than churning (contrast a NaN-bearing child). Only a +// nil interface child is rejected. func TestContainerAcceptsTypedNilChild(t *testing.T) { _, rq := newCoreRequest(t) tc := &testContainer{contents: []jaws.UI{(*typedNilChildUI)(nil)}} container := NewContainer("div", tc) - elem, _ := renderUI(t, rq, container) + elem, got := renderUI(t, rq, container) if cause := context.Cause(rq.Context()); cause != nil { t.Fatalf("render cancelled the Request: %v", cause) } + if !strings.Contains(got, "child") { + t.Fatalf("render = %q, want it to contain the child output", got) + } + if len(container.contents) != 1 { + t.Fatalf("want 1 child Element, got %d", len(container.contents)) + } + childElem := container.contents[0] + childJid := childElem.Jid() + container.JawsUpdate(elem) if cause := context.Cause(rq.Context()); cause != nil { t.Fatalf("update cancelled the Request: %v", cause) } + if len(container.contents) != 1 { + t.Fatalf("want 1 child Element after update, got %d", len(container.contents)) + } + if container.contents[0] != childElem { + t.Fatal("typed nil child was recreated on update instead of reused") + } + if got := container.contents[0].Jid(); got != childJid { + t.Fatalf("child Jid changed on update: %v -> %v", childJid, got) + } +} + +// TestContainerValidatesWholeSliceBeforeRender pins that the pre-lock scan validates +// the entire children slice before any child is created or rendered: a usable child +// preceding an unusable one is neither rendered nor committed when the Request is +// terminated. +func TestContainerValidatesWholeSliceBeforeRender(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("VALIDMARKER")), nanChildUI{f: math.NaN()}}} + container := NewContainer("div", tc) + elem := rq.NewElement(container) + var sb strings.Builder + if err := elem.JawsRender(&sb, nil); err != nil { + t.Fatalf("JawsRender err = %v, want nil", err) + } + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + if strings.Contains(sb.String(), "VALIDMARKER") { + t.Fatalf("valid prefix child was rendered before the slice was validated: %q", sb.String()) + } + if len(container.contents) != 0 { + t.Fatalf("no children should be committed, got %d", len(container.contents)) + } } // TestContainerTerminatesOnUnusableChild covers issue #179 and its From 4cd18422522f3498a530236c4221caf8e3b023bb Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Tue, 21 Jul 2026 09:34:38 +0200 Subject: [PATCH 7/8] test: prove no-child-created and no-partial-reconciliation in prevalidation tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TestContainerValidatesWholeSliceBeforeRender proved no rendering or commit but not the stated "no child created" guarantee. Assert via the Request registry that the Jid a valid prefix child would have taken is unused, so NewElement was never reached. Add TestContainerUpdateValidatesWholeSliceBeforeReconcile: a wanted slice with a usable child before an unusable one, on the update path, must abort before reconcile mutates u.contents or the pool. Assert the existing child is not removed, the new valid child is not created (its would-be Jid is unused), and u.contents is unchanged — which precludes any queued Remove/Append/Order op. --- lib/ui/containerhelper_test.go | 44 +++++++++++++++++++++++++++++++--- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index c7aae2b4..214febff 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -895,9 +895,9 @@ func TestContainerAcceptsTypedNilChild(t *testing.T) { } // TestContainerValidatesWholeSliceBeforeRender pins that the pre-lock scan validates -// the entire children slice before any child is created or rendered: a usable child -// preceding an unusable one is neither rendered nor committed when the Request is -// terminated. +// the entire children slice before any child is created, rendered, or committed: a +// usable child preceding an unusable one produces no Element, no output, and no +// committed content when the Request is terminated. func TestContainerValidatesWholeSliceBeforeRender(t *testing.T) { _, rq := newCoreRequest(t) tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("VALIDMARKER")), nanChildUI{f: math.NaN()}}} @@ -916,6 +916,44 @@ func TestContainerValidatesWholeSliceBeforeRender(t *testing.T) { if len(container.contents) != 0 { t.Fatalf("no children should be committed, got %d", len(container.contents)) } + // No child Element was created: NewElement is never reached, so the Jid that the + // valid prefix child would have taken (the one after the container's) is unused. + if child := rq.GetElementByJid(elem.Jid() + 1); child != nil { + t.Fatalf("a child Element (Jid %v) was created despite prevalidation abort", child.Jid()) + } +} + +// TestContainerUpdateValidatesWholeSliceBeforeReconcile is the update-path counterpart: +// when a wanted slice has a usable child before an unusable one, the whole slice is +// validated before reconcile touches u.contents or the pool, so there is no partial +// reconciliation — the existing child is not removed, the new valid child is not +// created, and consequently no Remove/Append/Order op is queued. +func TestContainerUpdateValidatesWholeSliceBeforeReconcile(t *testing.T) { + _, rq := newCoreRequest(t) + tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("OLD"))}} + container := NewContainer("div", tc) + elem, _ := renderUI(t, rq, container) + if len(container.contents) != 1 { + t.Fatalf("want 1 child before update, got %d", len(container.contents)) + } + childElem := container.contents[0] + nextJid := childElem.Jid() + 1 // the Jid a newly-created child would take + + tc.contents = []jaws.UI{NewSpan(testHTMLGetter("NEWVALID")), nanChildUI{f: math.NaN()}} + container.JawsUpdate(elem) + + if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { + t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) + } + if len(container.contents) != 1 || container.contents[0] != childElem { + t.Fatalf("contents changed on aborted update: partial reconciliation occurred") + } + if rq.GetElementByJid(childElem.Jid()) == nil { + t.Fatal("existing child was removed on aborted update") + } + if e := rq.GetElementByJid(nextJid); e != nil { + t.Fatalf("a new child Element (Jid %v) was created on aborted update", e.Jid()) + } } // TestContainerTerminatesOnUnusableChild covers issue #179 and its From 3b07ca728682aa9d8da8e9c47746fee7f30be88f Mon Sep 17 00:00:00 2001 From: Johan Lindh Date: Tue, 21 Jul 2026 09:43:21 +0200 Subject: [PATCH 8/8] test: prove "never created" via monotonic-Jid probe and observe reconcile outputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A GetElementByJid == nil check is also satisfied by a created-then-deleted Element, so it did not prove the "no child created" guarantee. Because Jids are monotonic, both prevalidation tests now create a probe Element after the abort and assert it receives the expected next Jid; a created (even later-deleted) child would have advanced the counter. Verified the probe fails when a stray element is created. The update test now exercises reconcile directly and asserts all four returned operation sets are empty, so UpdateContainer has nothing to queue — observing the reconciliation outputs rather than asserting the absence of ops indirectly. --- lib/ui/containerhelper_test.go | 42 +++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/lib/ui/containerhelper_test.go b/lib/ui/containerhelper_test.go index 214febff..a05dd81f 100644 --- a/lib/ui/containerhelper_test.go +++ b/lib/ui/containerhelper_test.go @@ -916,18 +916,22 @@ func TestContainerValidatesWholeSliceBeforeRender(t *testing.T) { if len(container.contents) != 0 { t.Fatalf("no children should be committed, got %d", len(container.contents)) } - // No child Element was created: NewElement is never reached, so the Jid that the - // valid prefix child would have taken (the one after the container's) is unused. - if child := rq.GetElementByJid(elem.Jid() + 1); child != nil { - t.Fatalf("a child Element (Jid %v) was created despite prevalidation abort", child.Jid()) + // No child Element was ever created: NewElement is never reached. Jids are + // monotonic (a created-then-deleted Element still advances the counter, and would + // not be found by a registry lookup), so a probe created now must take the Jid + // right after the container's; a higher Jid would mean a child was created. + probe := rq.NewElement(NewSpan(testHTMLGetter("probe"))) + if probe.Jid() != elem.Jid()+1 { + t.Fatalf("a child Element was created despite prevalidation abort: probe Jid %v, want %v", probe.Jid(), elem.Jid()+1) } } // TestContainerUpdateValidatesWholeSliceBeforeReconcile is the update-path counterpart: // when a wanted slice has a usable child before an unusable one, the whole slice is -// validated before reconcile touches u.contents or the pool, so there is no partial -// reconciliation — the existing child is not removed, the new valid child is not -// created, and consequently no Remove/Append/Order op is queued. +// validated before reconcile touches u.contents or the pool. It exercises reconcile +// directly and asserts its four operation sets are empty (so UpdateContainer queues no +// Remove/Append/Order), that u.contents is untouched, and — via a monotonic-Jid probe +// — that no new Element was created. func TestContainerUpdateValidatesWholeSliceBeforeReconcile(t *testing.T) { _, rq := newCoreRequest(t) tc := &testContainer{contents: []jaws.UI{NewSpan(testHTMLGetter("OLD"))}} @@ -937,22 +941,28 @@ func TestContainerUpdateValidatesWholeSliceBeforeReconcile(t *testing.T) { t.Fatalf("want 1 child before update, got %d", len(container.contents)) } childElem := container.contents[0] - nextJid := childElem.Jid() + 1 // the Jid a newly-created child would take - - tc.contents = []jaws.UI{NewSpan(testHTMLGetter("NEWVALID")), nanChildUI{f: math.NaN()}} - container.JawsUpdate(elem) + // Observe reconcile's outputs directly: a usable child before an unusable one must + // yield no operations at all, which is what leaves UpdateContainer nothing to queue. + toAppend, toRemove, oldOrder, newOrder := container.reconcile(elem, + []jaws.UI{NewSpan(testHTMLGetter("NEWVALID")), nanChildUI{f: math.NaN()}}) + if len(toAppend)+len(toRemove)+len(oldOrder)+len(newOrder) != 0 { + t.Fatalf("reconcile produced operations on abort: append=%d remove=%d oldOrder=%d newOrder=%d", + len(toAppend), len(toRemove), len(oldOrder), len(newOrder)) + } if cause := context.Cause(rq.Context()); !errors.Is(cause, tag.ErrNotUsableAsTag) { t.Fatalf("cause = %v, want wrapping tag.ErrNotUsableAsTag", cause) } + // No partial reconciliation: the existing content slice is untouched. if len(container.contents) != 1 || container.contents[0] != childElem { t.Fatalf("contents changed on aborted update: partial reconciliation occurred") } - if rq.GetElementByJid(childElem.Jid()) == nil { - t.Fatal("existing child was removed on aborted update") - } - if e := rq.GetElementByJid(nextJid); e != nil { - t.Fatalf("a new child Element (Jid %v) was created on aborted update", e.Jid()) + // No new Element was created (Jids are monotonic, so a created-then-deleted child + // would still have advanced the counter): a probe takes the Jid right after the + // existing child's. + probe := rq.NewElement(NewSpan(testHTMLGetter("probe"))) + if probe.Jid() != childElem.Jid()+1 { + t.Fatalf("a child Element was created on aborted update: probe Jid %v, want %v", probe.Jid(), childElem.Jid()+1) } }