amadan

oficina / sheets Public

calc: fix wrong COUNT corpus entry by threading literal-vs-reference origin

Code review caught a real bug: COUNT(1,2,"a",TRUE) was in the corpus
with want=2. Real Excel/LibreOffice return 3 -- a logical value typed
DIRECTLY as an argument is counted by COUNT; only a logical value read
FROM a reference is skipped. fnCount skipped KindBool unconditionally,
and the corpus was hand-authored against that same wrong mental model,
so it passed by construction -- the exact failure mode task 8's own
report flagged as a risk for SUM/AVERAGE/MIN/MAX but did not catch for
COUNT.

Fix, not a relabeled expectation: SUM, AVERAGE, COUNT, MIN and MAX now
register as nodeFuncs (registry.go) instead of ordinary Funcs, so each
can see whether an argument came from a bare *Ref/*Range (evalAggArgs,
fn_math.go) or was typed directly into the formula. A value read from a
reference keeps the old behaviour (blank/text/logical cells silently
skipped); a value typed directly now also coerces a boolean to 1/0 and
a numeric-looking string to a number, and errors (#VALUE!) on a literal
string that doesn't parse -- all matching Microsoft's own documented
rule ("logical values and text representations of numbers that you
type directly into the list of arguments are counted"). This also
closes the SUM/AVERAGE/MIN/MAX gap the first version of fn_math.go
explicitly disclosed but left open.

The corpus's COUNT(1,2,"a",TRUE) entry is corrected to want=3 -- the
true value, not a value chosen to match the old code -- and
corpusSHA256 updated to match. TestAverageOfNoNumbersIsDiv0 (which
relied on the old "literal text is silently ignored" behaviour) is
rewritten to reach zero-numeric-values through reference cells instead,
since a literal non-numeric string is now correctly #VALUE!, not
silently skipped.

Also in this commit (same review pass, same touched files):

- fnPower now guards its result, not just its inputs: math.Pow(-8,
  1.0/3) returns NaN even though -8 and 1/3 are each fine alone (a cube
  root written as a fractional power), and an overflowing result
  (POWER(10,400)) returns +Inf. Both are now #NUM!, matching Excel,
  with tests.
- VLOOKUP's omitted range_lookup (defaults to TRUE/approximate) and
  MATCH's omitted match_type (defaults to 1/nearest-ascending) are now
  pinned by direct unit tests -- exactly the kind of default a
  self-authored corpus can't validate, since nothing forces a corpus
  author to remember to omit an argument.
- TestAgainstGoldenCorpus renamed to TestAgainstReferenceCorpus, and
  "golden" reworded to "reference" throughout calc's corpus vocabulary
  -- the name showed on a CI dashboard implying external verification
  the hand-authored file doesn't have.
- clockEnv's unused `wall` field and `advanceWallClock` method removed:
  nothing in this package ever read `wall`, so advancing it could not,
  by construction, affect TestClockIsPinnedWithinAPass's outcome -- a
  test whose structure implied a mechanism ("advancing the wall clock
  exercises something") it did not actually have. The real guard was
  already documented as the `grep -rn 'time\.Now'` check; the comment
  now says so without the theatrical setup.

fn_math.go split into fn_math.go (SUM/AVERAGE/COUNT/COUNTA/MIN/MAX, the
literal-vs-reference machinery) and fn_math_round.go (ROUND family,
ABS, SQRT, POWER, MOD) to stay under the 300-line cap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01APtNStXDZ5H2fAc1ZuGqtS
Paul Campbell 4f12eaf9c13e5170aad28bb77f61071dd8b31b64 parent 1e86c97
8 files changed, +465 −242
  • internal/sheets/calc/corpus_test.go +32 −40
  • internal/sheets/calc/fn_lookup_test.go +21 −0
  • internal/sheets/calc/fn_math.go +141 −186
  • internal/sheets/calc/fn_math_round.go +164 −0
  • internal/sheets/calc/fn_math_test.go +80 −2
  • internal/sheets/calc/registry.go +23 −10
  • internal/sheets/calc/testdata/corpus/formulas.json +2 −2
  • internal/sheets/calc/testdata/corpus/volatile-excluded.txt +2 −2

Intent

full ledger: core

diff --git a/internal/sheets/calc/corpus_test.go b/internal/sheets/calc/corpus_test.go
index 64f6af1..534b35c 100644
--- a/internal/sheets/calc/corpus_test.go
+++ b/internal/sheets/calc/corpus_test.go
@@ -21,7 +21,7 @@ import (
// without also changing the file (or vice versa) is exactly the silent
// drift this test exists to catch — a corpus change is a deliberate act
// with a regenerated hash, never a side effect of an unrelated edit.
-const corpusSHA256 = "17b23ef50a9c9cf8aa01cdb37964f2d4b9ae458c1281b85ac4a0bbbb6d1d9b98"
+const corpusSHA256 = "8f79b9dc83b611810adb4add49ad1878a0bf9c0a7570ce0424a7849f4f36a491"
// corpusValue is formulas.json's wire format for a Value: kind names one
// of "empty", "num", "str", "bool", "err", and only the matching field is
@@ -98,7 +98,7 @@ const exclusionsPath = "testdata/corpus/volatile-excluded.txt"
// environment has soffice on PATH. Every "want" value was computed by
// hand-tracing this package's own arithmetic and string rules against
// well-established, widely-documented spreadsheet semantics, not
-// verified against an independent implementation. TestAgainstGoldenCorpus
+// verified against an independent implementation. TestAgainstReferenceCorpus
// therefore currently proves self-consistency with the reasoning that
// went into the corpus, not yet third-party agreement with a real
// spreadsheet. Replacing this file with a real
@@ -180,7 +180,7 @@ var corpusEnv = mapEnv{
{3, 1}: {Kind: KindNum, Num: 30}, {3, 2}: {Kind: KindStr, Str: "thirty"},
}
-func TestAgainstGoldenCorpus(t *testing.T) {
+func TestAgainstReferenceCorpus(t *testing.T) {
entries := loadCorpus(t)
excluded := loadExclusions(t)
for _, e := range entries {
@@ -216,46 +216,38 @@ func TestExclusionsAreRealFunctions(t *testing.T) {
}
// clockEnv models one recalculation pass for TestClockIsPinnedWithinAPass:
-// Now() answers the instant pinned at construction and never re-reads
-// anything. wall exists only so the test can move something OTHER than
-// the pinned instant and assert it made no difference — deliberately not
-// what Now() answers, which is the entire point (see the test's own doc
-// comment).
-type clockEnv struct {
- pinned time.Time
- wall time.Time
-}
-
-func newEnvAt(t time.Time) *clockEnv {
- return &clockEnv{pinned: t, wall: t}
-}
-
-func (e *clockEnv) advanceWallClock(d time.Duration) {
- e.wall = e.wall.Add(d)
-}
-
-func (e *clockEnv) CellValue(CellKey) Value { return Value{} }
-func (e *clockEnv) Now() time.Time { return e.pinned }
-
-// A recalculation pass sees one instant. If anything under us reads the
-// wall clock directly, this fails — which is the point. A pinned clock
-// that something beneath re-derives is a pin that only looks like one.
-//
-// This test's own power to catch that is bounded: clockEnv.wall is a
-// value nothing in this package other than this test ever reads, so
-// advancing it cannot, by construction, change what a correctly-written
-// fnNow computes — the load-bearing guarantee is that fn_date.go's fnNow
-// and fnToday call env.Now() and nothing else (no time.Now(), no package
-// clock). This test is the regression pin for that fact once true, not a
-// substitute for having checked it: `grep -rn 'time\.Now' internal/sheets/calc`
-// finds no hits outside this file (which uses time.Date, not time.Now)
-// and fn_date_test.go's TestNowTodayRejectArguments (which uses
-// time.Now() only to seed an env whose CellValue/Now never gets called
-// through NOW/TODAY's args path).
+// Now() always answers the instant pinned at construction, full stop. An
+// earlier version of this type also carried a "wall clock" field that a
+// test method advanced between the two Eval calls below, mimicking the
+// brief's own pseudocode — but nothing in this package ever read that
+// field, so advancing it could not, by construction, change the
+// outcome: the test would have passed identically with that call
+// deleted. A test whose structure implies a mechanism ("advancing the
+// wall clock exercises something") it does not actually have is the
+// same defect class this task's own corpus mistake was — see
+// TestClockIsPinnedWithinAPass's doc comment for what the real guard is.
+type clockEnv time.Time
+
+func newEnvAt(t time.Time) clockEnv { return clockEnv(t) }
+func (e clockEnv) CellValue(CellKey) Value { return Value{} }
+func (e clockEnv) Now() time.Time { return time.Time(e) }
+
+// A recalculation pass sees one instant: two calls to NOW() inside one
+// pass must agree. This assertion's own reach is bounded — two Eval
+// calls in immediate succession would very likely produce the same
+// answer even if fn_date.go called the real wall clock directly, since a
+// test runs in microseconds, so agreement here cannot by itself
+// distinguish "correctly pinned" from "happened to run fast twice in a
+// row." The actual guard is structural, not this assertion: fn_date.go's
+// fnNow and fnToday call env.Now() and nothing else (no time.Now(), no
+// package-level clock) — `grep -rn 'time\.Now' internal/sheets/calc`
+// finds no hit in any non-test file (fn_date_test.go's own use seeds a
+// pinned env; it never reaches NOW()/TODAY()'s own code path). This test
+// is the regression pin for that fact staying true, not a substitute for
+// having checked it.
func TestClockIsPinnedWithinAPass(t *testing.T) {
env := newEnvAt(time.Date(2026, 8, 30, 12, 0, 0, 0, time.UTC))
first := Eval(mustParse(t, "NOW()"), env)
- env.advanceWallClock(time.Hour) // does not touch the pass's instant
second := Eval(mustParse(t, "NOW()"), env)
if first.Num != second.Num {
t.Fatal("NOW() moved inside one pass; something is reading the wall clock")
diff --git a/internal/sheets/calc/fn_lookup_test.go b/internal/sheets/calc/fn_lookup_test.go
index 269e1de..17dea7e 100644
--- a/internal/sheets/calc/fn_lookup_test.go
+++ b/internal/sheets/calc/fn_lookup_test.go
@@ -86,3 +86,24 @@ func TestVLookupTableArrayMustBeRange(t *testing.T) {
t.Fatalf("VLOOKUP with scalar table_array = %v, want #VALUE!", v)
}
}
+
+// range_lookup defaults to TRUE (approximate match) when omitted — one
+// of the two "classic trap" defaults a self-authored corpus can't
+// validate (nothing forces a corpus author to remember to omit an
+// argument), so it needs a direct test rather than relying on a corpus
+// row.
+func TestVLookupOmittedRangeLookupDefaultsToApproximate(t *testing.T) {
+ v := Eval(mustParse(t, "VLOOKUP(25,A1:B3,2)"), lookupTable())
+ if v.Kind != KindStr || v.Str != "twenty" {
+ t.Fatalf("VLOOKUP with range_lookup omitted = %v, want \"twenty\" (approximate match by default)", v)
+ }
+}
+
+// match_type defaults to 1 (largest value <= lookup, ascending) when
+// omitted — the second classic trap default.
+func TestMatchOmittedMatchTypeDefaultsToNearestAscending(t *testing.T) {
+ v := Eval(mustParse(t, "MATCH(25,A1:A3)"), lookupTable())
+ if v.Kind != KindNum || v.Num != 2 {
+ t.Fatalf("MATCH with match_type omitted = %v, want 2 (nearest-ascending by default)", v)
+ }
+}
diff --git a/internal/sheets/calc/fn_math.go b/internal/sheets/calc/fn_math.go
index 0f045bf..6f84251 100644
--- a/internal/sheets/calc/fn_math.go
+++ b/internal/sheets/calc/fn_math.go
@@ -1,56 +1,123 @@
package calc
-import "math"
+// fn_math.go: SUM, AVERAGE, COUNT, COUNTA, MIN, MAX — the aggregate
+// family. Rounding/misc math (ROUND, ROUNDUP, ROUNDDOWN, ABS, SQRT,
+// POWER, MOD) lives in fn_math_round.go; split out to stay under the
+// 300-line cap once this file grew the literal-vs-reference machinery
+// below.
func init() {
- Register("SUM", fnSum)
- Register("AVERAGE", fnAverage)
- Register("COUNT", fnCount)
+ registerNode("SUM", fnSum)
+ registerNode("AVERAGE", fnAverage)
+ registerNode("COUNT", fnCount)
Register("COUNTA", fnCounta)
- Register("MIN", fnMin)
- Register("MAX", fnMax)
- Register("ROUND", fnRound)
- Register("ROUNDUP", fnRoundUp)
- Register("ROUNDDOWN", fnRoundDown)
- Register("ABS", fnAbs)
- Register("SQRT", fnSqrt)
- Register("POWER", fnPower)
- Register("MOD", fnMod)
-}
-
-// numericValues applies to SUM, AVERAGE, MIN and MAX: any KindErr
-// argument propagates immediately (left-most wins, matching every other
-// error path in this package — one bad cell breaks a real spreadsheet's
-// SUM the same way). KindEmpty and KindStr are skipped rather than
-// coerced — this matches how Excel treats a *range's* blank/text cells
-// for these four functions.
+ registerNode("MIN", fnMin)
+ registerNode("MAX", fnMax)
+}
+
+// taggedValue pairs a value with whether it came from a cell reference —
+// a bare *Ref, or one of a *Range's flattened cells — rather than being
+// typed directly into the formula. SUM, AVERAGE, COUNT, MIN and MAX all
+// need this distinction: Excel silently ignores a blank, text, or
+// logical value read FROM a reference, but coerces (or errors on) that
+// same kind of value when it is typed DIRECTLY as an argument —
+// SUM(A1) where A1 contains TRUE is 0, but SUM(TRUE) is 1. Microsoft's
+// own documentation for SUM/AVERAGE/COUNT/MIN/MAX all use the same
+// phrase for the direct-argument case: "logical values and text
+// representations of numbers that you type directly into the list of
+// arguments are counted."
//
-// What it does NOT match is Excel's other rule that a text argument
-// passed directly (not through a range) coerces if it looks numeric —
-// real Excel's SUM(1,"2") is 3. evalCallArg has already flattened a
-// range's cells and a literal argument into the same []Value with no way
-// to tell them apart by the time a Func sees them (see registry.go's
-// nodeFunc doc comment for the general shape of this problem), so this
-// task picks the one rule that is right for a range and wrong for a
-// literal, rather than the reverse — ranges are the overwhelmingly more
-// common shape for these four functions in practice. Flagged here for
-// whoever eventually threads argument-origin through Func's signature.
-func numericValues(args []Value) ([]float64, *Value) {
+// This is why these five register as nodeFuncs (registry.go) instead of
+// ordinary Funcs: by the time an ordinary Func sees a flattened
+// []Value, a reference's cells and a literal argument are
+// indistinguishable — the exact gap flagged, but not yet closed, the
+// first time this file was written.
+type taggedValue struct {
+ Value
+ fromRef bool
+}
+
+// evalAggArgs evaluates a call's argument nodes into taggedValues. A bare
+// *Ref and each cell of a *Range are both tagged fromRef=true — Excel
+// treats a single-cell reference the same as a one-cell range for this
+// purpose, not like a literal. Everything else (a number/string/bool
+// literal, or any computed expression such as an arithmetic operation or
+// a nested call) evaluates through Eval and is tagged fromRef=false.
+func evalAggArgs(nodes []Node, env Env) []taggedValue {
+ var out []taggedValue
+ for _, n := range nodes {
+ switch t := n.(type) {
+ case *Range:
+ cells, ok := rangeCells(t)
+ if !ok {
+ out = append(out, taggedValue{Value{Kind: KindErr, Err: ErrRef}, true})
+ continue
+ }
+ for _, k := range cells {
+ out = append(out, taggedValue{env.CellValue(k), true})
+ }
+ case *Ref:
+ out = append(out, taggedValue{env.CellValue(refKey(t)), true})
+ default:
+ out = append(out, taggedValue{Eval(n, env), false})
+ }
+ }
+ return out
+}
+
+// numericValues applies to SUM, AVERAGE, MIN and MAX. Any KindErr value
+// propagates immediately, left-most wins — matching every other error
+// path in this package. A value read from a reference (fromRef=true)
+// counts only if it is already KindNum: a blank, a text cell, or a
+// logical cell is silently skipped, matching how Excel treats a range's
+// own blank/text/logical cells. A value typed directly as an argument
+// (fromRef=false) additionally coerces a KindBool to 1/0 and a KindStr
+// that parses as a number — Excel's own "typed directly into the list of
+// arguments" rule (see taggedValue's doc comment). A literal string that
+// does NOT parse as a number is #VALUE! — unlike a range's text cell, a
+// literal you plainly meant to be numeric is not silently ignored.
+func numericValues(args []taggedValue) ([]float64, *Value) {
var nums []float64
for i := range args {
a := args[i]
if a.Kind == KindErr {
- return nil, &a
+ v := a.Value
+ return nil, &v
}
- if a.Kind == KindNum {
+ if a.fromRef {
+ if a.Kind == KindNum {
+ nums = append(nums, a.Num)
+ }
+ continue
+ }
+ switch a.Kind {
+ case KindNum:
nums = append(nums, a.Num)
+ case KindBool:
+ if a.Bool {
+ nums = append(nums, 1)
+ } else {
+ nums = append(nums, 0)
+ }
+ case KindStr:
+ n, ek := toNumber(a.Value)
+ if ek != ErrNone {
+ v := errValue(ErrValue)
+ return nil, &v
+ }
+ nums = append(nums, n)
+ default:
+ // KindEmpty typed directly: unreachable in practice — the
+ // only source of KindEmpty is an untouched cell, and every
+ // cell-reference path is already tagged fromRef=true above
+ // (evalAggArgs) — kept only for defensiveness.
}
}
return nums, nil
}
-func fnSum(args []Value) Value {
- nums, errv := numericValues(args)
+func fnSum(argNodes []Node, env Env) Value {
+ nums, errv := numericValues(evalAggArgs(argNodes, env))
if errv != nil {
return *errv
}
@@ -61,8 +128,8 @@ func fnSum(args []Value) Value {
return Value{Kind: KindNum, Num: total}
}
-func fnAverage(args []Value) Value {
- nums, errv := numericValues(args)
+func fnAverage(argNodes []Node, env Env) Value {
+ nums, errv := numericValues(evalAggArgs(argNodes, env))
if errv != nil {
return *errv
}
@@ -76,25 +143,46 @@ func fnAverage(args []Value) Value {
return Value{Kind: KindNum, Num: total / float64(len(nums))}
}
-// fnCount counts only KindNum arguments — not KindEmpty, KindStr,
-// KindBool or KindErr. Unlike SUM/AVERAGE/MIN/MAX, an error argument does
-// not propagate: COUNT is a tally, and a broken cell elsewhere in a range
-// is common enough (a VLOOKUP helper column full of #N/A, say) that
-// COUNT/COUNTA are relied on to scan past it rather than becoming it.
-func fnCount(args []Value) Value {
+// fnCount counts numeric values: a KindNum from anywhere, plus — per
+// numericValues' doc comment on the same literal-vs-reference
+// distinction — a KindBool or a numeric-looking KindStr typed DIRECTLY
+// as an argument (COUNT(1,2,"a",TRUE) is 3: "a" doesn't parse as a
+// number and is skipped, but TRUE does count because it was typed
+// directly, not read from a reference). Unlike SUM/AVERAGE/MIN/MAX, no
+// argument here propagates an error — COUNT is a tally, and a broken
+// cell elsewhere in a range is common enough (a VLOOKUP helper column
+// full of #N/A, say) that COUNT/COUNTA are relied on to scan past it
+// rather than becoming it.
+func fnCount(argNodes []Node, env Env) Value {
n := 0
- for _, a := range args {
- if a.Kind == KindNum {
+ for _, a := range evalAggArgs(argNodes, env) {
+ if a.Kind == KindErr {
+ continue
+ }
+ if a.fromRef {
+ if a.Kind == KindNum {
+ n++
+ }
+ continue
+ }
+ switch a.Kind {
+ case KindNum, KindBool:
n++
+ case KindStr:
+ if _, ek := toNumber(a.Value); ek == ErrNone {
+ n++
+ }
}
}
return Value{Kind: KindNum, Num: float64(n)}
}
// fnCounta counts every argument that is not KindEmpty — numbers,
-// strings, booleans and (per fnCount's doc comment) errors all count as
-// "present." This is the property COUNT vs. COUNTA hinges on (task 8's
-// brief names it directly): neither counts a blank cell, but COUNTA
+// strings, booleans and errors all count as "present," from either a
+// reference or a literal alike, which is why (unlike its four siblings
+// above) COUNTA needs no literal-vs-reference distinction and stays an
+// ordinary Func. This is the property COUNT vs. COUNTA hinges on (task
+// 8's brief names it directly): neither counts a blank cell, but COUNTA
// counts everything else that COUNT would skip.
func fnCounta(args []Value) Value {
n := 0
@@ -106,8 +194,8 @@ func fnCounta(args []Value) Value {
return Value{Kind: KindNum, Num: float64(n)}
}
-func fnMin(args []Value) Value {
- nums, errv := numericValues(args)
+func fnMin(argNodes []Node, env Env) Value {
+ nums, errv := numericValues(evalAggArgs(argNodes, env))
if errv != nil {
return *errv
}
@@ -123,8 +211,8 @@ func fnMin(args []Value) Value {
return Value{Kind: KindNum, Num: m}
}
-func fnMax(args []Value) Value {
- nums, errv := numericValues(args)
+func fnMax(argNodes []Node, env Env) Value {
+ nums, errv := numericValues(evalAggArgs(argNodes, env))
if errv != nil {
return *errv
}
@@ -139,136 +227,3 @@ func fnMax(args []Value) Value {
}
return Value{Kind: KindNum, Num: m}
}
-
-// twoNumArgs evaluates the common "exactly two numeric arguments" shape
-// shared by ROUND/ROUNDUP/ROUNDDOWN/POWER/MOD: an error argument
-// propagates left-most-wins, a wrong argument count or a non-coercible
-// argument is #VALUE!.
-func twoNumArgs(args []Value) (a, b float64, errv Value, ok bool) {
- if len(args) != 2 {
- return 0, 0, errValue(ErrValue), false
- }
- if args[0].Kind == KindErr {
- return 0, 0, args[0], false
- }
- if args[1].Kind == KindErr {
- return 0, 0, args[1], false
- }
- x, ek := toNumber(args[0])
- if ek != ErrNone {
- return 0, 0, errValue(ek), false
- }
- y, ek := toNumber(args[1])
- if ek != ErrNone {
- return 0, 0, errValue(ek), false
- }
- return x, y, Value{}, true
-}
-
-// roundAt implements the shared shape behind ROUND (mode 0, half away
-// from zero — Excel's own rounding rule, not banker's rounding),
-// ROUNDUP (mode 1, always away from zero regardless of the dropped
-// digits) and ROUNDDOWN (mode 2, always toward zero — truncation).
-// digits may be negative (round to tens, hundreds, ...), matching Excel.
-func roundAt(x float64, digits int, mode int) float64 {
- mult := math.Pow(10, float64(digits))
- v := x * mult
- switch mode {
- case 0:
- if v >= 0 {
- v = math.Floor(v + 0.5)
- } else {
- v = math.Ceil(v - 0.5)
- }
- case 1:
- if v >= 0 {
- v = math.Ceil(v)
- } else {
- v = math.Floor(v)
- }
- default:
- if v >= 0 {
- v = math.Floor(v)
- } else {
- v = math.Ceil(v)
- }
- }
- return v / mult
-}
-
-func fnRound(args []Value) Value {
- x, d, errv, ok := twoNumArgs(args)
- if !ok {
- return errv
- }
- return Value{Kind: KindNum, Num: roundAt(x, int(d), 0)}
-}
-
-func fnRoundUp(args []Value) Value {
- x, d, errv, ok := twoNumArgs(args)
- if !ok {
- return errv
- }
- return Value{Kind: KindNum, Num: roundAt(x, int(d), 1)}
-}
-
-func fnRoundDown(args []Value) Value {
- x, d, errv, ok := twoNumArgs(args)
- if !ok {
- return errv
- }
- return Value{Kind: KindNum, Num: roundAt(x, int(d), 2)}
-}
-
-func fnAbs(args []Value) Value {
- if len(args) != 1 {
- return errValue(ErrValue)
- }
- if args[0].Kind == KindErr {
- return args[0]
- }
- x, ek := toNumber(args[0])
- if ek != ErrNone {
- return errValue(ek)
- }
- return Value{Kind: KindNum, Num: math.Abs(x)}
-}
-
-func fnSqrt(args []Value) Value {
- if len(args) != 1 {
- return errValue(ErrValue)
- }
- if args[0].Kind == KindErr {
- return args[0]
- }
- x, ek := toNumber(args[0])
- if ek != ErrNone {
- return errValue(ek)
- }
- if x < 0 {
- return errValue(ErrNum)
- }
- return Value{Kind: KindNum, Num: math.Sqrt(x)}
-}
-
-func fnPower(args []Value) Value {
- x, y, errv, ok := twoNumArgs(args)
- if !ok {
- return errv
- }
- return Value{Kind: KindNum, Num: math.Pow(x, y)}
-}
-
-// fnMod implements MOD the way Excel does: the result takes the sign of
-// the divisor (n - d*floor(n/d)), not the sign of the dividend the way
-// Go's own % operator would give — MOD(-10,3) is 2, not -1.
-func fnMod(args []Value) Value {
- x, y, errv, ok := twoNumArgs(args)
- if !ok {
- return errv
- }
- if y == 0 {
- return errValue(ErrDiv0)
- }
- return Value{Kind: KindNum, Num: x - y*math.Floor(x/y)}
-}
diff --git a/internal/sheets/calc/fn_math_round.go b/internal/sheets/calc/fn_math_round.go
new file mode 100644
index 0000000..31aefe6
--- /dev/null
+++ b/internal/sheets/calc/fn_math_round.go
@@ -0,0 +1,164 @@
+package calc
+
+import "math"
+
+// fn_math_round.go: ROUND, ROUNDUP, ROUNDDOWN, ABS, SQRT, POWER, MOD —
+// split from fn_math.go (SUM/AVERAGE/COUNT/COUNTA/MIN/MAX) to stay under
+// the 300-line cap; still the same "math family" the plan groups
+// together, just none of these need the literal-vs-reference machinery
+// fn_math.go carries.
+
+func init() {
+ Register("ROUND", fnRound)
+ Register("ROUNDUP", fnRoundUp)
+ Register("ROUNDDOWN", fnRoundDown)
+ Register("ABS", fnAbs)
+ Register("SQRT", fnSqrt)
+ Register("POWER", fnPower)
+ Register("MOD", fnMod)
+}
+
+// twoNumArgs evaluates the common "exactly two numeric arguments" shape
+// shared by ROUND/ROUNDUP/ROUNDDOWN/POWER/MOD: an error argument
+// propagates left-most-wins, a wrong argument count or a non-coercible
+// argument is #VALUE!.
+func twoNumArgs(args []Value) (a, b float64, errv Value, ok bool) {
+ if len(args) != 2 {
+ return 0, 0, errValue(ErrValue), false
+ }
+ if args[0].Kind == KindErr {
+ return 0, 0, args[0], false
+ }
+ if args[1].Kind == KindErr {
+ return 0, 0, args[1], false
+ }
+ x, ek := toNumber(args[0])
+ if ek != ErrNone {
+ return 0, 0, errValue(ek), false
+ }
+ y, ek := toNumber(args[1])
+ if ek != ErrNone {
+ return 0, 0, errValue(ek), false
+ }
+ return x, y, Value{}, true
+}
+
+// roundAt implements the shared shape behind ROUND (mode 0, half away
+// from zero — Excel's own rounding rule, not banker's rounding),
+// ROUNDUP (mode 1, always away from zero regardless of the dropped
+// digits) and ROUNDDOWN (mode 2, always toward zero — truncation).
+// digits may be negative (round to tens, hundreds, ...), matching Excel.
+func roundAt(x float64, digits int, mode int) float64 {
+ mult := math.Pow(10, float64(digits))
+ v := x * mult
+ switch mode {
+ case 0:
+ if v >= 0 {
+ v = math.Floor(v + 0.5)
+ } else {
+ v = math.Ceil(v - 0.5)
+ }
+ case 1:
+ if v >= 0 {
+ v = math.Ceil(v)
+ } else {
+ v = math.Floor(v)
+ }
+ default:
+ if v >= 0 {
+ v = math.Floor(v)
+ } else {
+ v = math.Ceil(v)
+ }
+ }
+ return v / mult
+}
+
+func fnRound(args []Value) Value {
+ x, d, errv, ok := twoNumArgs(args)
+ if !ok {
+ return errv
+ }
+ return Value{Kind: KindNum, Num: roundAt(x, int(d), 0)}
+}
+
+func fnRoundUp(args []Value) Value {
+ x, d, errv, ok := twoNumArgs(args)
+ if !ok {
+ return errv
+ }
+ return Value{Kind: KindNum, Num: roundAt(x, int(d), 1)}
+}
+
+func fnRoundDown(args []Value) Value {
+ x, d, errv, ok := twoNumArgs(args)
+ if !ok {
+ return errv
+ }
+ return Value{Kind: KindNum, Num: roundAt(x, int(d), 2)}
+}
+
+func fnAbs(args []Value) Value {
+ if len(args) != 1 {
+ return errValue(ErrValue)
+ }
+ if args[0].Kind == KindErr {
+ return args[0]
+ }
+ x, ek := toNumber(args[0])
+ if ek != ErrNone {
+ return errValue(ek)
+ }
+ return Value{Kind: KindNum, Num: math.Abs(x)}
+}
+
+func fnSqrt(args []Value) Value {
+ if len(args) != 1 {
+ return errValue(ErrValue)
+ }
+ if args[0].Kind == KindErr {
+ return args[0]
+ }
+ x, ek := toNumber(args[0])
+ if ek != ErrNone {
+ return errValue(ek)
+ }
+ if x < 0 {
+ return errValue(ErrNum)
+ }
+ return Value{Kind: KindNum, Num: math.Sqrt(x)}
+}
+
+// fnPower guards its result, not just its inputs, unlike fnSqrt: a
+// negative base with a non-integer exponent (POWER(-8, 1.0/3) — a cube
+// root written as a fractional power) makes math.Pow return NaN for
+// inputs that are each individually fine on their own, and an
+// overflowing result (POWER(10, 400)) returns +Inf. Both are #NUM! in
+// Excel, never a value a cell should hold — Value has no way to
+// represent "not a number" other than the error kind that means exactly
+// that.
+func fnPower(args []Value) Value {
+ x, y, errv, ok := twoNumArgs(args)
+ if !ok {
+ return errv
+ }
+ r := math.Pow(x, y)
+ if math.IsNaN(r) || math.IsInf(r, 0) {
+ return errValue(ErrNum)
+ }
+ return Value{Kind: KindNum, Num: r}
+}
+
+// fnMod implements MOD the way Excel does: the result takes the sign of
+// the divisor (n - d*floor(n/d)), not the sign of the dividend the way
+// Go's own % operator would give — MOD(-10,3) is 2, not -1.
+func fnMod(args []Value) Value {
+ x, y, errv, ok := twoNumArgs(args)
+ if !ok {
+ return errv
+ }
+ if y == 0 {
+ return errValue(ErrDiv0)
+ }
+ return Value{Kind: KindNum, Num: x - y*math.Floor(x/y)}
+}
diff --git a/internal/sheets/calc/fn_math_test.go b/internal/sheets/calc/fn_math_test.go
index cc1499b..8f656dc 100644
--- a/internal/sheets/calc/fn_math_test.go
+++ b/internal/sheets/calc/fn_math_test.go
@@ -44,10 +44,15 @@ func TestAverage(t *testing.T) {
}
}
+// A literal non-numeric string passed directly is #VALUE! (see
+// TestSumLiteralNonNumericStringIsValueError), not silently ignored — so
+// this test reaches "zero numeric values" the way Excel actually allows:
+// text read FROM references, which AVERAGE skips rather than errors on.
func TestAverageOfNoNumbersIsDiv0(t *testing.T) {
- v := Eval(mustParse(t, `AVERAGE("a","b")`), emptyEnv{})
+ env := mapEnv{{1, 1}: {Kind: KindStr, Str: "a"}, {1, 2}: {Kind: KindStr, Str: "b"}}
+ v := Eval(mustParse(t, "AVERAGE(A1,B1)"), env)
if v.Kind != KindErr || v.Err != ErrDiv0 {
- t.Fatalf(`AVERAGE("a","b") = %v, want #DIV/0!`, v)
+ t.Fatalf("AVERAGE(A1,B1) = %v, want #DIV/0!", v)
}
}
@@ -139,3 +144,76 @@ func TestMathWrongArgCountIsValueError(t *testing.T) {
}
}
}
+
+// POWER guards its result, not just its inputs: a negative base with a
+// non-integer exponent makes math.Pow return NaN even though -8 and
+// 1.0/3 are each individually fine — this is a cube root written as a
+// fractional power, and Excel returns #NUM!, not a NaN masquerading as
+// a number.
+func TestPowerNaNResultIsNum(t *testing.T) {
+ v := Eval(mustParse(t, "POWER(-8,1/3)"), emptyEnv{})
+ if v.Kind != KindErr || v.Err != ErrNum {
+ t.Fatalf("POWER(-8,1/3) = %v, want #NUM!", v)
+ }
+}
+
+// An overflowing result is also #NUM!, not +Inf.
+func TestPowerOverflowIsNum(t *testing.T) {
+ v := Eval(mustParse(t, "POWER(10,400)"), emptyEnv{})
+ if v.Kind != KindErr || v.Err != ErrNum {
+ t.Fatalf("POWER(10,400) = %v, want #NUM!", v)
+ }
+}
+
+// The literal-vs-reference distinction (taggedValue, fn_math.go):
+// SUM/AVERAGE/COUNT/MIN/MAX all treat a boolean or a numeric-looking
+// string typed DIRECTLY as an argument differently from the same kind of
+// value read FROM a cell reference. These tests pin that distinction
+// directly, closing the gap fn_math.go's first version flagged but did
+// not fix (caught by code review against a real-Excel-behavior corpus
+// entry for COUNT).
+
+func TestSumLiteralBooleanCoerces(t *testing.T) {
+ if got := evalNum(t, "SUM(TRUE,1)", emptyEnv{}); got != 2 {
+ t.Errorf("SUM(TRUE,1) = %v, want 2 (literal TRUE counts as 1)", got)
+ }
+}
+
+func TestSumReferenceBooleanIgnored(t *testing.T) {
+ env := mapEnv{{1, 1}: {Kind: KindBool, Bool: true}}
+ if got := evalNum(t, "SUM(A1,1)", env); got != 1 {
+ t.Errorf("SUM(A1,1) with A1=TRUE = %v, want 1 (reference TRUE ignored)", got)
+ }
+}
+
+func TestSumLiteralNonNumericStringIsValueError(t *testing.T) {
+ v := Eval(mustParse(t, `SUM(1,"abc")`), emptyEnv{})
+ if v.Kind != KindErr || v.Err != ErrValue {
+ t.Fatalf(`SUM(1,"abc") = %v, want #VALUE! (a literal you meant as a number, not silently skipped)`, v)
+ }
+}
+
+func TestMinMaxLiteralBooleanCoerces(t *testing.T) {
+ if got := evalNum(t, "MIN(TRUE,5)", emptyEnv{}); got != 1 {
+ t.Errorf("MIN(TRUE,5) = %v, want 1", got)
+ }
+ if got := evalNum(t, "MAX(FALSE,5)", emptyEnv{}); got != 5 {
+ t.Errorf("MAX(FALSE,5) = %v, want 5", got)
+ }
+}
+
+// The corpus row this exact case pins (testdata/corpus/formulas.json):
+// a logical value typed directly IS counted, but "a" isn't numeric so
+// it isn't — 3, not 4 and not 2.
+func TestCountLiteralBooleanAndNumericString(t *testing.T) {
+ if got := evalNum(t, `COUNT(1,2,"a",TRUE)`, emptyEnv{}); got != 3 {
+ t.Errorf(`COUNT(1,2,"a",TRUE) = %v, want 3`, got)
+ }
+}
+
+func TestCountReferenceBooleanNotCounted(t *testing.T) {
+ env := mapEnv{{1, 1}: {Kind: KindBool, Bool: true}}
+ if got := evalNum(t, "COUNT(A1)", env); got != 0 {
+ t.Errorf("COUNT(A1) with A1=TRUE = %v, want 0 (reference logical not counted)", got)
+ }
+}
diff --git a/internal/sheets/calc/registry.go b/internal/sheets/calc/registry.go
index e4127a2..c73db7a 100644
--- a/internal/sheets/calc/registry.go
+++ b/internal/sheets/calc/registry.go
@@ -49,16 +49,29 @@ func registerClock(name string, fn clockFunc) {
clockFuncTable[name] = fn
}
-// nodeFunc is the signature for a function that needs a range argument's
-// shape as written — its width and height — rather than only the
-// flattened, boundary-free []Value evalCallArg already produces for an
-// ordinary Func. VLOOKUP, INDEX and MATCH (fn_lookup.go) all turn a
-// row/column offset into a position within a table/array argument, and
-// by the time evalCallArgs has flattened every argument into one shared
-// slice, both the shape AND the boundary between one argument's cells and
-// the next are gone — unrecoverable, not merely inconvenient. These three
-// see the raw argument AST nodes instead and evaluate what they need
-// themselves, the same way evalCall would.
+// nodeFunc is the signature for a function that needs to see its own
+// argument nodes rather than only the flattened, boundary-free []Value
+// evalCallArgs already produces for an ordinary Func — because an
+// ordinary Func's []Value has thrown away information the function
+// actually needs. Two distinct things get thrown away, and each has its
+// own nodeFunc registrants:
+//
+// - Shape: VLOOKUP, INDEX and MATCH (fn_lookup.go) all turn a
+// row/column offset into a position within a table/array argument,
+// and by the time evalCallArgs has flattened every argument into one
+// shared slice, both a range's width/height AND the boundary between
+// one argument's cells and the next are gone — unrecoverable, not
+// merely inconvenient.
+// - Origin: SUM, AVERAGE, COUNT, MIN and MAX (fn_math.go) all need to
+// know whether a given value was read from a cell reference or typed
+// directly into the formula — Excel treats a logical or text value
+// differently depending on which (fn_math.go's taggedValue doc
+// comment has the concrete example and the Excel documentation
+// wording this is drawn from) — and a flattened []Value has already
+// erased that distinction too.
+//
+// Both kinds see the raw argument AST nodes instead and evaluate what
+// they need themselves, the same way evalCall would.
type nodeFunc func(argNodes []Node, env Env) Value
var nodeFuncTable = map[string]nodeFunc{}
diff --git a/internal/sheets/calc/testdata/corpus/formulas.json b/internal/sheets/calc/testdata/corpus/formulas.json
index 8146b1b..1725a8e 100644
--- a/internal/sheets/calc/testdata/corpus/formulas.json
+++ b/internal/sheets/calc/testdata/corpus/formulas.json
@@ -1,5 +1,5 @@
{
- "note": "HAND-AUTHORED, NOT YET LIBREOFFICE-VERIFIED. tools/gencorpus (repo root) exists to regenerate this file from a real LibreOffice headless run, but no machine in the environment that wrote this file has soffice on PATH. Every \"want\" value below was computed by a person hand-tracing this package's arithmetic/string/lookup rules against well-established, widely-documented spreadsheet semantics -- not against an independent implementation. TestAgainstGoldenCorpus (corpus_test.go) currently proves self-consistency with that reasoning, not third-party agreement with a real spreadsheet. Regenerate with `go run ./tools/gencorpus -out internal/sheets/calc/testdata/corpus/formulas.json` on a machine with LibreOffice installed, review the diff, then update corpusSHA256 in corpus_test.go and this note.",
+ "note": "HAND-AUTHORED, NOT YET LIBREOFFICE-VERIFIED. tools/gencorpus (repo root) exists to regenerate this file from a real LibreOffice headless run, but no machine in the environment that wrote this file has soffice on PATH. Every \"want\" value below was computed by a person hand-tracing this package's arithmetic/string/lookup rules against well-established, widely-documented spreadsheet semantics -- not against an independent implementation. TestAgainstReferenceCorpus (corpus_test.go) currently proves self-consistency with that reasoning, not third-party agreement with a real spreadsheet. Regenerate with `go run ./tools/gencorpus -out internal/sheets/calc/testdata/corpus/formulas.json` on a machine with LibreOffice installed, review the diff, then update corpusSHA256 in corpus_test.go and this note.",
"entries": [
{"function": "SUM", "expr": "SUM(1,2,3)", "want": {"kind": "num", "num": 6}},
{"function": "SUM", "expr": "SUM(1,2,3,4,5)", "want": {"kind": "num", "num": 15}},
@@ -7,7 +7,7 @@
{"function": "SUM", "expr": "SUM(1.5,2.5)", "want": {"kind": "num", "num": 4}},
{"function": "AVERAGE", "expr": "AVERAGE(2,4,6)", "want": {"kind": "num", "num": 4}},
{"function": "AVERAGE", "expr": "AVERAGE(1,2,3,4)", "want": {"kind": "num", "num": 2.5}},
- {"function": "COUNT", "expr": "COUNT(1,2,\"a\",TRUE)", "want": {"kind": "num", "num": 2}},
+ {"function": "COUNT", "expr": "COUNT(1,2,\"a\",TRUE)", "want": {"kind": "num", "num": 3}},
{"function": "COUNTA", "expr": "COUNTA(1,2,\"a\",TRUE)", "want": {"kind": "num", "num": 4}},
{"function": "MIN", "expr": "MIN(4,2,9,1)", "want": {"kind": "num", "num": 1}},
{"function": "MAX", "expr": "MAX(4,2,9,1)", "want": {"kind": "num", "num": 9}},
diff --git a/internal/sheets/calc/testdata/corpus/volatile-excluded.txt b/internal/sheets/calc/testdata/corpus/volatile-excluded.txt
index 7080045..ffa415a 100644
--- a/internal/sheets/calc/testdata/corpus/volatile-excluded.txt
+++ b/internal/sheets/calc/testdata/corpus/volatile-excluded.txt
@@ -1,10 +1,10 @@
# testdata/corpus/volatile-excluded.txt
#
# Functions listed here are volatile: they return a different value on
-# every recalculation, so no single "golden" value captured by one
+# every recalculation, so no single reference value captured by one
# LibreOffice run (or one gencorpus run) can ever match a later run's
# answer -- including this package's own re-evaluation of the same
-# corpus file in a later `go test` invocation. TestAgainstGoldenCorpus
+# corpus file in a later `go test` invocation. TestAgainstReferenceCorpus
# (corpus_test.go) skips any corpus row whose Function is listed here.
#
# This file is a list somebody edits, not a filter somebody widens: it is