Skip to content

fix(math fns) Math.min/Math.max truncating Float (and String) arguments#136

Open
arthurmaciel wants to merge 2 commits into
anzellai:mainfrom
arthurmaciel:fix/go-math-minmax-float
Open

fix(math fns) Math.min/Math.max truncating Float (and String) arguments#136
arthurmaciel wants to merge 2 commits into
anzellai:mainfrom
arthurmaciel:fix/go-math-minmax-float

Conversation

@arthurmaciel

@arthurmaciel arthurmaciel commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Problem

Math.min / Math.max are polymorphic — a -> a -> a ("any comparable type",
per Sky.Core.Math). But the Go backend coerced their arguments through AsInt:

  • the typed-companion pass emitted rt.Math_minT(rt.AsInt(x), rt.AsInt(y))
    (typedKernelArgCoerce + typedKernelLiterals), and
  • the polymorphic fallback rt.Math_min / rt.Math_max also compared via AsInt.

So a Float Math.min / max truncated to Int. The range of [0.4 … 1.3]
collapses to 0..1, which mis-scales every Std.Ui.Chart sparkline and heatmap —
the heatmap renders a single cell instead of the full grid. String args were
meaningless too.

-- range computed with Math.min / Math.max over a List Float
Chart.heatmap cfg grid
Chart.sparkline cfg [ 0.4, 0.6, 0.5, 0.8, 0.7, 0.9, 1.0, 0.95, 1.2, 1.15, 1.3 ]

Before / After

26-sparkline-before-after 26-heatmap-before-after

Fix

  1. rt.Math_min / rt.Math_max compare via skyLessThan (the polymorphic
    comparator already used by List_sortBy) instead of AsInt.
  2. Drop Math.min / Math.max from typedKernelArgCoerce + typedKernelLiterals
    so they lower to the now-correct polymorphic kernel; the use-site coercion
    narrows the result.
  3. Same float/string-aware comparison in the runtimeGoSource fallback.

Math.abs is intentionally left untouched — its Sky type is Int -> Int, so
AsInt is correct.

Tests

runtime-go/rt/math_minmax_poly_test.go covers Float / Int / String min/max.
go test ./rt/ passes.

Related — Elm conformance

Sky.Core.Math mirrors Elm's
Basics; this
restores min / max to Elm's polymorphic comparable -> comparable -> comparable.
It may be worth a broader review of Sky's conformance to Elm Basics — for
example, Elm's abs : number -> number accepts Float, whereas Sky's abs is
currently Int -> Int.

Math.min / Math.max are polymorphic (`a -> a -> a`, "any comparable type" —
Sky.Core.Math), but the Go backend coerced their arguments through AsInt: the P8
typed-companion pass emitted rt.Math_minT(rt.AsInt(x), rt.AsInt(y)) (typedKernelArgCoerce
+ typedKernelLiterals), and the polymorphic fallback rt.Math_min/rt.Math_max also
compared via AsInt. So a Float Math.min/max TRUNCATED to Int — e.g. the range of
[0.4 … 1.3] collapsed to 0..1, mis-scaling every Std.Ui.Chart sparkline and heatmap
(the heatmap rendered a single cell instead of the full grid). Strings were
meaningless too.

Fix: (1) rt.Math_min/rt.Math_max compare via skyLessThan (the existing polymorphic
comparator used by List_sortBy) instead of AsInt; (2) drop Math.min/Math.max from
typedKernelArgCoerce + typedKernelLiterals so they lower to the (now-correct)
polymorphic kernel — the use-site coercion narrows the result. Math.abs stays
(its Sky type is Int -> Int, so AsInt is correct). Regression tests cover Float,
Int, and String min/max.
@arthurmaciel arthurmaciel changed the title Fix Math.min/Math.max truncating Float (and String) arguments fix(math libs) Math.min/Math.max truncating Float (and String) arguments Jun 20, 2026
The bundled console (sky-bundled/console/) uses Std.Ui.Chart, so its committed
generated Go (runtime-go/rt/console_app/main.go) now lowers Math.min/Math.max to
the polymorphic rt.Math_min/rt.Math_max instead of the Int companion. Regenerated
via scripts/regenerate-console.sh so the inline-copy drift check passes.
@arthurmaciel arthurmaciel changed the title fix(math libs) Math.min/Math.max truncating Float (and String) arguments fix(math fns) Math.min/Math.max truncating Float (and String) arguments Jun 20, 2026
@anzellai

Copy link
Copy Markdown
Owner

it makes sense, yes Go's side of AsInt is kind of annoying as that's underlying Go's int type, doesn't cover Elm's equivalent or Go's comparable. Perhaps I need to use Go's generic here with AsNum, I will have a proper think after v0.17 is shipped.
thanks!

arthurmaciel added a commit to arthurmaciel/sky that referenced this pull request Jun 20, 2026
equiv-render.sh + two normalisers add STRICT render-equivalence for the UI shapes
the examples-sweep's weak modes (live=scenario-boot, tui=pty-no-crash) miss.

live (equiv_normalize_html.py): serve both, GET /, extract #sky-root, byte-diff
after canonicalising legitimate implementation-detail differences — sky-id
separators, attribute order, event wire-encoding, pseudo/mq/anim/tr style-delivery
— and masking SVG chart coords (known Go float-truncation, PR anzellai#136). The backends
are committed to BEHAVIOURAL parity, not byte parity, so the test compares what
the user sees. Verified: 26-ui-showcase → 0-line diff (structural parity); a
textarea/badge/structure regression would re-surface.

tui (equiv_tui_grid.py): capture the initial frame in a fixed 80xN pty, render via
pyte to a styled cell grid (char + fg/bg/bold/italic/underline), diff. Catches
layout + styling. Verified it detects the current 24 divergences (collapses to ~0
after the Tui renderer-parity fixes land).

Normalisers verified against real both-backend captures; full build-harness run +
CI wiring deferred until the Tui fixes land (commit green, not known-red) and to
avoid racing the shared cargo target while the Tui implementer builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants