amadan

oficina / sheets Public

Render: precedence-aware bracketing so translated formulas re-parse correctly

Render previously joined Left op Right with no parentheses, so any tree
built from a lower-precedence child under a tighter parent (or a
same-precedence child on associativity's unprotected side) silently
regrouped on re-parse: (1+2)*3 rendered as 1+2*3 (9 becomes 7),
-(A1+A2) rendered as -A1+A2 (for A1=1,A2=2: -3 becomes 1), and
8-(3-1) rendered as 8-3-1 (6 becomes 4). TestReferenceBehaviourPerOperation
never exercised any of these compositions, so the contract table it backs
was blind to the bug.

Render now mirrors the parser's own precedence-climbing structure: each
Binary/Unary operand is wrapped in parens whenever its precedence is
strictly looser than its parent's, or tied at a precedence associativity
doesn't protect on that side (right operand for the left-associative
levels, left operand for the right-associative ^). Added
TestRenderRoundTripsThroughReparse, which checks Parse(Render(n)) against
n via reflect.DeepEqual rather than string equality, since that is the
property that determines what a copied formula actually computes.
Paul Campbell 9d02c1d5b039bacb30b6f80061dce5acdbd8a09a parent df19b0d
2 files changed, +144 −9
  • internal/sheets/calc/translate.go +93 −8
  • internal/sheets/calc/translate_test.go +51 −1

Intent

full ledger: core

diff --git a/internal/sheets/calc/translate.go b/internal/sheets/calc/translate.go
index 4a51561..7421ab7 100644
--- a/internal/sheets/calc/translate.go
+++ b/internal/sheets/calc/translate.go
@@ -45,13 +45,17 @@ func Translate(n Node, dRow, dCol int) Node {
// Render is the inverse of Parse for the node shapes Parse can produce:
// numbers, strings (re-quoted with `"` doubled), booleans, references and
// ranges (with both absolute markers preserved in position), unary minus,
-// left-associative binary operators, and upper-cased function calls. It
-// does not attempt to reproduce the source's original parenthesisation or
-// whitespace — only a re-parseable, semantically equivalent formula. This
-// is also what the formula bar shows, so the round trip on absolute
-// markers matters beyond the tests: a Render that dropped a "$" would
+// binary operators, and upper-cased function calls. It does not reproduce
+// the source's original whitespace or its exact parenthesisation — a
+// redundant paren the source didn't need is never replayed — but for every
+// tree Parse can produce, Parse(Render(n)) reproduces a tree equal to n:
+// Render inserts parentheses wherever precedence or associativity would
+// otherwise regroup the expression, the mirror of the precedence-climbing
+// parser above. This is also what the formula bar shows, so both
+// guarantees matter beyond the tests: a Render that dropped a "$" would
// silently turn an absolute reference into a relative one the next time it
-// was copied.
+// was copied, and a Render that dropped a needed paren would silently
+// change the value a cell computes.
func Render(n Node) string {
switch t := n.(type) {
case *Num:
@@ -68,9 +72,10 @@ func Render(n Node) string {
case *Range:
return renderRef(t.From) + ":" + renderRef(t.To)
case *Unary:
- return t.Op + Render(t.X)
+ return t.Op + renderOperand(t.X, precUnary, false)
case *Binary:
- return Render(t.Left) + t.Op + Render(t.Right)
+ p := binaryPrec(t.Op)
+ return renderOperand(t.Left, p, false) + t.Op + renderOperand(t.Right, p, true)
case *Call:
args := make([]string, len(t.Args))
for i, a := range t.Args {
@@ -82,6 +87,86 @@ func Render(n Node) string {
}
}
+// Precedence levels, loosest to tightest, matching Parse's grammar
+// (comparison, concat, additive, term, power, then unary — see the
+// precedence note on Parse). Atoms (numbers, strings, refs, ranges,
+// calls) never need parenthesising as anyone's child, so they sort above
+// even unary.
+const (
+ precComparison = iota + 1
+ precConcat
+ precAdditive
+ precTerm
+ precPower
+ precUnary
+ precAtom
+)
+
+// binaryPrec maps a Binary's operator to its precedence level.
+func binaryPrec(op string) int {
+ switch op {
+ case "=", "<>", "<", "<=", ">", ">=":
+ return precComparison
+ case "&":
+ return precConcat
+ case "+", "-":
+ return precAdditive
+ case "*", "/":
+ return precTerm
+ case "^":
+ return precPower
+ default:
+ return precAtom
+ }
+}
+
+// nodePrec reports the precedence level a node binds at when it appears as
+// someone else's operand: a Binary's by its operator, a Unary's fixed at
+// precUnary (it parses tighter than any binary operator but looser than
+// nothing — see the -2^2 = 4 note on Parse), everything else at precAtom.
+func nodePrec(n Node) int {
+ switch t := n.(type) {
+ case *Binary:
+ return binaryPrec(t.Op)
+ case *Unary:
+ return precUnary
+ default:
+ return precAtom
+ }
+}
+
+// renderOperand renders n as a child sitting in a slot of precedence
+// parentPrec, adding parentheses exactly when omitting them would let the
+// child regroup under re-parsing: when it binds strictly looser than the
+// parent, or when it binds at exactly the parent's precedence on the side
+// associativity doesn't protect (see samePrecedenceNeedsParens).
+func renderOperand(n Node, parentPrec int, isRight bool) string {
+ cp := nodePrec(n)
+ needParens := cp < parentPrec || (cp == parentPrec && samePrecedenceNeedsParens(parentPrec, isRight))
+ s := Render(n)
+ if needParens {
+ return "(" + s + ")"
+ }
+ return s
+}
+
+// samePrecedenceNeedsParens decides, for a child at exactly its parent's
+// precedence, whether it still needs parens to reparse into the same tree.
+// Every level Parse builds is left-associative except ^, so a
+// left-associative parent needs parens around a same-precedence RIGHT
+// child (8-(3-1) must keep its parens: "8-3-1" reparses as (8-3)-1), while
+// the right-associative power needs them around a same-precedence LEFT
+// child instead ((2^3)^2 must keep its parens: "2^3^2" reparses as
+// 2^(3^2)). The unary slot (parentPrec == precUnary, always called with
+// isRight == false) falls into the left-associative branch, which is also
+// the answer it wants: chained unary minus ("--A1") never needs parens.
+func samePrecedenceNeedsParens(parentPrec int, isRight bool) bool {
+ if parentPrec == precPower {
+ return !isRight
+ }
+ return isRight
+}
+
// renderRef reproduces a single reference's A1 text, including whichever
// dollar signs are set: neither ($A1 has one, A$1 has the other, $A$1 has
// both), matching how decodeRef reads them back.
diff --git a/internal/sheets/calc/translate_test.go b/internal/sheets/calc/translate_test.go
index 4afe60c..dade354 100644
--- a/internal/sheets/calc/translate_test.go
+++ b/internal/sheets/calc/translate_test.go
@@ -1,6 +1,9 @@
package calc
-import "testing"
+import (
+ "reflect"
+ "testing"
+)
func TestTranslateRelativeRef(t *testing.T) {
n, _ := Parse("A1")
@@ -63,3 +66,50 @@ func TestReferenceBehaviourPerOperation(t *testing.T) {
}
}
}
+
+// TestRenderRoundTripsThroughReparse pins Render's actual contract: not
+// that its output string matches some expected literal, but that
+// re-parsing it reproduces a tree equal to the one Render started from.
+// String equality would be too strong (Render need not replay redundant
+// source parens) and too weak (it would miss a wrong-but-plausible-looking
+// rendering); re-parsing and comparing trees is the property that
+// determines what a copied formula actually computes. Each case is a
+// precedence or associativity composition the four-row contract table
+// above never exercises, so a regression here would have passed that
+// table silently.
+func TestRenderRoundTripsThroughReparse(t *testing.T) {
+ cases := []struct {
+ name, src string
+ }{
+ // A looser-binding left child of a tighter parent: without
+ // parens "1+2*3" reparses to 1+(2*3), a different tree — 9
+ // would silently become 7.
+ {"parenthesized_addition_inside_multiplication", "(1+2)*3"},
+ // Parse only ever puts a Binary directly under a Unary via
+ // source parens (parseUnary's operand otherwise recurses
+ // through parseUnary/parsePrimary, never through a binary
+ // level); without parens "-A1+A2" reparses as (-A1)+A2 — for
+ // A1=1, A2=2 that is -3 silently becoming 1.
+ {"negated_parenthesized_sum", "-(A1+A2)"},
+ // Left-associative operators need parens around a
+ // same-precedence RIGHT operand: without them "8-3-1"
+ // reparses as (8-3)-1 — 6 would silently become 4.
+ {"left_associative_subtraction_needs_right_parens", "8-(3-1)"},
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ want, err := Parse(tc.src)
+ if err != nil {
+ t.Fatalf("parse %q: %v", tc.src, err)
+ }
+ rendered := Render(want)
+ got, err := Parse(rendered)
+ if err != nil {
+ t.Fatalf("reparse %q (rendered from %q): %v", rendered, tc.src, err)
+ }
+ if !reflect.DeepEqual(want, got) {
+ t.Fatalf("%q rendered as %q, which reparses to a different tree:\nwant %#v\ngot %#v", tc.src, rendered, want, got)
+ }
+ })
+ }
+}