From 562b1d72fb85d3b100fde16bc8dca06a29186b71 Mon Sep 17 00:00:00 2001 From: Erwan Lee Pesle Date: Sun, 25 Oct 2026 11:53:44 +0800 Subject: [PATCH 1/2] fix(format): keep the parentheses an expression's reading depends on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit exprToString parenthesised an operand only when it bound more loosely than its parent, so a right operand at the same level as `-` or `/` lost its parentheses: `thrust / (totalMass * 9.81) >= 2` rendered as `thrust / totalMass * 9.81 ≥ 2`, which denotes another expression than the one evaluated. The same held for `a - (b - c)`, comparisons nested in comparisons, and a unary minus over a compound or negated operand (`-(a + b)` rendered as `-a + b`). Operands are now parenthesised by side and operator; parentheses that change nothing are still left out. First tests for format.ts. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++ src/format.ts | 48 ++++++++++++++------ tests/format_test.ts | 104 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 13 deletions(-) create mode 100644 tests/format_test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 620344f..1f8c2ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to `@casys/constraint-solver` will be documented in this file. +## [Unreleased] + +### Fixed + +- **`exprToString` keeps the parentheses the reading depends on.** A right operand at the same level as `-` or `/` lost them, so `thrust / (totalMass * 9.81) ≥ 2` was shown as `thrust / totalMass * 9.81 ≥ 2`, another expression than the one evaluated. The same held for `a - (b - c)`, a comparison inside a comparison, and a unary minus over a compound or negated operand (`-(a + b)` was shown as `-a + b`). Parentheses that change nothing are still left out (`a - b - c`, `a * b * c`, `(a + b) * 2`). + ## [0.1.0] - 2026-07-30 Initial release. Extracted from the constraint evaluator in `lib/sim`, with the evaluation core rewritten over [mathjs](https://mathjs.org) so that units participate in the comparison. diff --git a/src/format.ts b/src/format.ts index 6383107..f842b0a 100644 --- a/src/format.ts +++ b/src/format.ts @@ -29,6 +29,9 @@ const PRECEDENCE: Partial> = { "/": 5, }; +/** Operators whose right operand cannot shed parentheses at the same level. */ +const NON_ASSOCIATIVE = new Set(["-", "/"]); + /** * Render an expression as a readable string. * @@ -36,6 +39,7 @@ const PRECEDENCE: Partial> = { * - `totalMass ≤ 5 kg` * - `operatingTemp ≥ -10 and operatingTemp ≤ 80` * - `(a + b) * 2` + * - `thrust / (totalMass * 9.81) ≥ 2` */ export function exprToString(expr: ConstraintExpr): string { switch (expr.kind) { @@ -45,26 +49,44 @@ export function exprToString(expr: ConstraintExpr): string { return expr.featurePath.join("."); case "binary": { const symbol = OP_SYMBOLS[expr.op] ?? expr.op; - const precedence = PRECEDENCE[expr.op] ?? 0; - return `${maybeParen(expr.left, precedence)} ${symbol} ${ - maybeParen(expr.right, precedence) + return `${operand(expr.left, expr.op, "left")} ${symbol} ${ + operand(expr.right, expr.op, "right") }`; } - case "unary": - return expr.op === "not" - ? `not(${exprToString(expr.operand)})` - : `-${exprToString(expr.operand)}`; + case "unary": { + if (expr.op === "not") return `not(${exprToString(expr.operand)})`; + // A compound or negated operand keeps its parentheses: -(a + b), -(-a). + const rendered = exprToString(expr.operand); + return expr.operand.kind === "binary" || expr.operand.kind === "unary" + ? `-(${rendered})` + : `-${rendered}`; + } case "call": return `${expr.name}(${expr.args.map(exprToString).join(", ")})`; } } -/** Parenthesise a sub-expression that binds more loosely than its parent. */ -function maybeParen(expr: ConstraintExpr, parentPrecedence: number): string { +/** + * Render an operand of `parentOp`, parenthesised wherever the bare text would + * read as another expression: it binds more loosely than its parent; it sits + * right of `-` or `/` at the same level (`a - (b - c)`, `a / (b * c)`); it is + * a comparison inside a comparison; or it is a negation on the right + * (`a - (-b)`). + */ +function operand( + expr: ConstraintExpr, + parentOp: BinaryOp, + side: "left" | "right", +): string { const rendered = exprToString(expr); - if (expr.kind === "binary") { - const own = PRECEDENCE[expr.op] ?? 0; - if (own < parentPrecedence) return `(${rendered})`; + if (expr.kind === "unary" && expr.op === "-") { + return side === "right" ? `(${rendered})` : rendered; } - return rendered; + if (expr.kind !== "binary") return rendered; + const own = PRECEDENCE[expr.op] ?? 0; + const parent = PRECEDENCE[parentOp] ?? 0; + const sameLevelNeedsParens = own === parent && + (parent === PRECEDENCE["<"] || + (side === "right" && NON_ASSOCIATIVE.has(parentOp))); + return own < parent || sameLevelNeedsParens ? `(${rendered})` : rendered; } diff --git a/tests/format_test.ts b/tests/format_test.ts new file mode 100644 index 0000000..e5aee39 --- /dev/null +++ b/tests/format_test.ts @@ -0,0 +1,104 @@ +/** + * Formatting tests + * + * The rendered expression is what an engineer reads next to the computed + * value: it must denote the same expression, with no parentheses dropped. + */ + +import { assertEquals } from "@std/assert"; +import { exprToString } from "../src/format.ts"; +import type { BinaryOp, ConstraintExpr } from "../src/types.ts"; + +const ref = (name: string): ConstraintExpr => ({ + kind: "ref", + featurePath: name.split("."), +}); +const num = (value: number, unit?: string): ConstraintExpr => ({ + kind: "literal", + value, + unit, +}); +const bin = ( + left: ConstraintExpr, + op: BinaryOp, + right: ConstraintExpr, +): ConstraintExpr => ({ kind: "binary", op, left, right }); +const neg = (operand: ConstraintExpr): ConstraintExpr => ({ + kind: "unary", + op: "-", + operand, +}); + +Deno.test("format — a right operand under / or - keeps its parentheses", () => { + // thrust / (totalMass * 9.81) >= 2 is not (thrust / totalMass) * 9.81 >= 2. + assertEquals( + exprToString( + bin( + bin(ref("thrust"), "/", bin(ref("totalMass"), "*", num(9.81))), + ">=", + num(2), + ), + ), + "thrust / (totalMass * 9.81) ≥ 2", + ); + assertEquals( + exprToString(bin(ref("a"), "-", bin(ref("b"), "-", ref("c")))), + "a - (b - c)", + ); + assertEquals( + exprToString(bin(ref("a"), "-", bin(ref("b"), "+", ref("c")))), + "a - (b + c)", + ); + assertEquals( + exprToString(bin(ref("a"), "/", bin(ref("b"), "/", ref("c")))), + "a / (b / c)", + ); +}); + +Deno.test("format — no parentheses where the reading is unchanged", () => { + assertEquals( + exprToString(bin(bin(ref("a"), "-", ref("b")), "-", ref("c"))), + "a - b - c", + ); + assertEquals( + exprToString(bin(ref("a"), "*", bin(ref("b"), "*", ref("c")))), + "a * b * c", + ); + assertEquals( + exprToString(bin(ref("a"), "+", bin(ref("b"), "-", ref("c")))), + "a + b - c", + ); + assertEquals( + exprToString(bin(bin(ref("a"), "+", ref("b")), "*", num(2))), + "(a + b) * 2", + ); + assertEquals( + exprToString( + bin(bin(ref("t"), ">=", num(-10)), "and", bin(ref("t"), "<=", num(80))), + ), + "t ≥ -10 and t ≤ 80", + ); + assertEquals( + exprToString(bin(ref("totalMass"), "<=", num(5, "kg"))), + "totalMass ≤ 5 kg", + ); +}); + +Deno.test("format — comparisons nested in comparisons are parenthesised", () => { + assertEquals( + exprToString( + bin(bin(ref("a"), "<", ref("b")), "==", bin(ref("c"), "<", ref("d"))), + ), + "(a < b) == (c < d)", + ); +}); + +Deno.test("format — unary minus keeps the parentheses of a compound operand", () => { + assertEquals(exprToString(neg(bin(ref("a"), "+", ref("b")))), "-(a + b)"); + assertEquals(exprToString(neg(ref("a"))), "-a"); + assertEquals(exprToString(neg(neg(ref("a")))), "-(-a)"); + assertEquals( + exprToString(bin(ref("a"), "-", neg(ref("b")))), + "a - (-b)", + ); +}); From 38178cdc53d3942e698ebeecca4e856c5230f8ba Mon Sep 17 00:00:00 2001 From: Erwan Lee Pesle Date: Sun, 25 Oct 2026 12:06:49 +0800 Subject: [PATCH 2/2] fix(format): group only what changes the reading; release 0.1.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the parentheses fix: - a negation right of an operator is grouped only under `-` (`t ≥ -10`, not `t ≥ (-10)`); a negative literal there too (`a - (-5)`); - a negative under a unary minus is grouped (`-(-5)`, formerly `--5`); - a quantity right of `*` or `/` is grouped, since mathjs reads `5 / 2 kg` as `(5 / 2) kg`; - an operator outside the precedence table groups its compound operands. A 4.77M-tree round-trip fuzz finds no rendering that reads as another expression. Version 0.1.1. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 +-- deno.json | 2 +- src/format.ts | 40 +++++++++++++++++++++------- tests/format_test.ts | 62 +++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 94 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f8c2ca..0dbd9e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,11 @@ All notable changes to `@casys/constraint-solver` will be documented in this file. -## [Unreleased] +## [0.1.1] - 2026-10-25 ### Fixed -- **`exprToString` keeps the parentheses the reading depends on.** A right operand at the same level as `-` or `/` lost them, so `thrust / (totalMass * 9.81) ≥ 2` was shown as `thrust / totalMass * 9.81 ≥ 2`, another expression than the one evaluated. The same held for `a - (b - c)`, a comparison inside a comparison, and a unary minus over a compound or negated operand (`-(a + b)` was shown as `-a + b`). Parentheses that change nothing are still left out (`a - b - c`, `a * b * c`, `(a + b) * 2`). +- **`exprToString` keeps the parentheses the reading depends on.** A right operand at the same level as `-` or `/` lost them, so `thrust / (totalMass * 9.81) ≥ 2` was shown as `thrust / totalMass * 9.81 ≥ 2`, another expression than the one evaluated. The same held for `a - (b - c)`, a comparison inside a comparison, a unary minus over a compound operand (`-(a + b)` was shown as `-a + b`), and an operator outside the precedence table (`a ^ (b + c)`). A quantity right of `*` or `/` is now grouped, since mathjs reads `5 / 2 kg` as `(5 / 2) kg`: `5 / (2 kg)`. A negative right of `-` is grouped (`a - (-5)`), and so is a negative under a unary minus (`-(-5)`, formerly `--5`). Parentheses that change no value are still left out: `a - b - c`, `a * b * c`, `t ≥ -10`. ## [0.1.0] - 2026-07-30 diff --git a/deno.json b/deno.json index 64add93..ef93413 100644 --- a/deno.json +++ b/deno.json @@ -1,6 +1,6 @@ { "name": "@casys/constraint-solver", - "version": "0.1.0", + "version": "0.1.1", "exports": { ".": "./mod.ts" }, diff --git a/src/format.ts b/src/format.ts index f842b0a..aad6077 100644 --- a/src/format.ts +++ b/src/format.ts @@ -55,9 +55,11 @@ export function exprToString(expr: ConstraintExpr): string { } case "unary": { if (expr.op === "not") return `not(${exprToString(expr.operand)})`; - // A compound or negated operand keeps its parentheses: -(a + b), -(-a). + // A compound or negative operand keeps its parentheses: -(a + b), + // -(-a), -(-5). const rendered = exprToString(expr.operand); - return expr.operand.kind === "binary" || expr.operand.kind === "unary" + return expr.operand.kind === "binary" || expr.operand.kind === "unary" || + isNegative(expr.operand) ? `-(${rendered})` : `-${rendered}`; } @@ -66,12 +68,22 @@ export function exprToString(expr: ConstraintExpr): string { } } +/** A negation, or a negative number. */ +function isNegative(expr: ConstraintExpr): boolean { + return (expr.kind === "unary" && expr.op === "-") || + (expr.kind === "literal" && expr.value < 0); +} + /** * Render an operand of `parentOp`, parenthesised wherever the bare text would - * read as another expression: it binds more loosely than its parent; it sits - * right of `-` or `/` at the same level (`a - (b - c)`, `a / (b * c)`); it is - * a comparison inside a comparison; or it is a negation on the right - * (`a - (-b)`). + * read as another expression or another value: + * - it binds more loosely than its parent; + * - it sits right of `-` or `/` at the same level (`a - (b - c)`, + * `a / (b * c)`); + * - it is a comparison inside a comparison; + * - it is a negative right of `-` (`a - (-b)`, `a - (-5)`); + * - it is a quantity right of `*` or `/` (`5 / (2 kg)` is not `(5 / 2) kg`); + * - its parent is an operator outside the precedence table. */ function operand( expr: ConstraintExpr, @@ -79,14 +91,22 @@ function operand( side: "left" | "right", ): string { const rendered = exprToString(expr); - if (expr.kind === "unary" && expr.op === "-") { - return side === "right" ? `(${rendered})` : rendered; + const grouped = `(${rendered})`; + const parent = PRECEDENCE[parentOp]; + if (parent === undefined) { + return expr.kind === "binary" ? grouped : rendered; + } + if ( + expr.kind === "literal" && expr.unit && side === "right" && + (parentOp === "*" || parentOp === "/") + ) return grouped; + if (isNegative(expr)) { + return side === "right" && parentOp === "-" ? grouped : rendered; } if (expr.kind !== "binary") return rendered; const own = PRECEDENCE[expr.op] ?? 0; - const parent = PRECEDENCE[parentOp] ?? 0; const sameLevelNeedsParens = own === parent && (parent === PRECEDENCE["<"] || (side === "right" && NON_ASSOCIATIVE.has(parentOp))); - return own < parent || sameLevelNeedsParens ? `(${rendered})` : rendered; + return own < parent || sameLevelNeedsParens ? grouped : rendered; } diff --git a/tests/format_test.ts b/tests/format_test.ts index e5aee39..0033fee 100644 --- a/tests/format_test.ts +++ b/tests/format_test.ts @@ -55,7 +55,7 @@ Deno.test("format — a right operand under / or - keeps its parentheses", () => ); }); -Deno.test("format — no parentheses where the reading is unchanged", () => { +Deno.test("format — leaves out parentheses that change no value", () => { assertEquals( exprToString(bin(bin(ref("a"), "-", ref("b")), "-", ref("c"))), "a - b - c", @@ -102,3 +102,63 @@ Deno.test("format — unary minus keeps the parentheses of a compound operand", "a - (-b)", ); }); + +Deno.test("format — a negative is grouped only right of a minus", () => { + assertEquals(exprToString(bin(ref("t"), ">=", neg(num(10)))), "t ≥ -10"); + assertEquals(exprToString(bin(ref("a"), "+", neg(ref("b")))), "a + -b"); + assertEquals(exprToString(bin(ref("a"), "*", neg(ref("b")))), "a * -b"); + assertEquals(exprToString(bin(neg(ref("a")), "*", ref("b"))), "-a * b"); + assertEquals(exprToString(bin(ref("a"), "-", num(-5))), "a - (-5)"); + assertEquals(exprToString(neg(num(-5))), "-(-5)"); + assertEquals(exprToString(neg(num(-5, "kg"))), "-(-5 kg)"); +}); + +Deno.test("format — a quantity right of * or / is grouped", () => { + // `5 / 2 kg` reads (5 / 2) kg; `d / 5 km/h` reads (d / 5 km) / h. + assertEquals(exprToString(bin(num(5), "/", num(2, "kg"))), "5 / (2 kg)"); + assertEquals( + exprToString(bin(ref("d"), "/", num(5, "km/h"))), + "d / (5 km/h)", + ); + assertEquals(exprToString(bin(num(2), "*", num(5, "kg"))), "2 * (5 kg)"); + assertEquals(exprToString(bin(num(5, "kg"), "/", num(2))), "5 kg / 2"); + assertEquals(exprToString(bin(ref("m"), "<=", num(5, "kg"))), "m ≤ 5 kg"); +}); + +Deno.test("format — logic, calls and operators outside the table", () => { + assertEquals( + exprToString(bin(bin(ref("a"), "or", ref("b")), "and", ref("c"))), + "(a or b) and c", + ); + assertEquals( + exprToString(bin(ref("a"), "or", bin(ref("b"), "and", ref("c")))), + "a or b and c", + ); + assertEquals( + exprToString({ + kind: "unary", + op: "not", + operand: bin(ref("a"), "<", ref("b")), + }), + "not(a < b)", + ); + assertEquals( + exprToString( + bin( + { + kind: "call", + name: "max", + args: [bin(ref("a"), "+", ref("b")), num(2)], + }, + "*", + num(3), + ), + ), + "max(a + b, 2) * 3", + ); + // An operator from untyped input has no known precedence: group operands. + assertEquals( + exprToString(bin(ref("a"), "^" as BinaryOp, bin(ref("b"), "+", ref("c")))), + "a ^ (b + c)", + ); +});