fix: parenthesise an arithmetic expression passed as an argument

.changeset/arith-argument-parens.md

@@ -0,0 +1,5 @@
+---
+"@maple-dev/effect-orm": patch
+---
+
+An arithmetic expression passed as an argument keeps its parentheses: `$.a.sub($.b.add(1))` compiles to `a - (b + 1)`, not `a - b + 1`. Chains still follow SQL precedence as documented (`$.a.sub(1).div(2)` is `a - 1 / 2`); passing the group as an argument is how to group.

docs/expressions.md

@@ -164,6 +164,12 @@ decodes that `null` as `NaN`. `sum`, `sumIf`, and `toFloat64OrZero` behave the s
 
 _(Backed by `docs/expressions.md > Arithmetic does not parenthesise`.)_
 
+An expression passed as an *argument* keeps its boundary, so that is how to group:
+`$.DurationMs.sub($.Offset.add(1))` is `DurationMs - (Offset + 1)`, and
+`CH.lit(0.5).mul($.DurationMs.sub(1))` is `0.5 * (DurationMs - 1)`.
+
+_(Backed by `docs/expressions.md > An arithmetic argument keeps its parentheses`.)_
+
 ### Division can produce a NULL
 
 `.div()` and `.mod()` decode nullably, whatever their operands are. ClickHouse renders `1 / 0` as

docs/testing.md

@@ -124,8 +124,8 @@ A failure prints the shrunk input, the SQL it compiled to, the database's reply,
 
 Bugs found but not yet fixed live in `tests/fuzz/known-issues.ts`. Each entry recognises
 the inputs that reach its bug and gives a reason, so the suite stays green while the rest of
-the input space keeps being fuzzed. After fixing a bug, delete its entry and run with
-`FUZZ_STRICT=1`, which ignores every entry, to confirm nothing else matched it. Differences
+the input space keeps being fuzzed. `FUZZ_STRICT=<id>` re-enables one entry's inputs, to check a
+fix before deleting the entry; `FUZZ_STRICT=1` re-enables them all. Differences
 between the databases that are not bugs are encoded in the model instead, each with the
 difference it represents.
 

src/ch/expr.ts

@@ -391,8 +391,16 @@ const inCond = (
 const acceptsNull = (schema: Schema.Codec<any, any> | undefined): boolean =>
 	schema !== undefined && Result.isSuccess(Schema.decodeUnknownResult(schema)(null))
 
+/** Expressions `arith` built: an infix operator a caller may pass as an argument. */
+const infix = new WeakSet<object>()
+
 /** Numeric promotion and SQL NULL propagation share one runtime codec. The
- *  result type is the caller's claim — see {@link Quotient} for `/` and `%`. */
+ *  result type is the caller's claim — see {@link Quotient} for `/` and `%`.
+ *
+ *  The receiver is not parenthesised: a chain follows SQL precedence, as
+ *  docs/expressions.md says. An argument is, when it is itself arithmetic —
+ *  `$.a.sub($.b.add(1))` means `a - (b + 1)`, and no call order could say so
+ *  if the argument's boundary were dropped. */
 const arith = <Result>(
 	lhs: SqlFragment,
 	op: string,
@@ -411,10 +419,16 @@ const arith = <Result>(
 		acceptsNull(rhsSchema)
 	// `+`, `-`, `*` can overflow a Float64 to `inf`, sent as JSON null: NaN.
 	const overflows = op === "+" || op === "-" || op === "*"
-	return makeExpr(
-		known(() => `${compile(lhs)} ${op} ${compile(toFragment(rhs))}`),
+	const grouped = typeof rhs === "object" && rhs !== null && infix.has(rhs)
+	const expr = makeExpr(
+		known(() => {
+			const right = compile(toFragment(rhs))
+			return `${compile(lhs)} ${op} ${grouped ? `(${right})` : right}`
+		}),
 		(nullable ? Schema.NullOr(CHNumber) : overflows ? CHFloatResult : CHNumber) as Schema.Codec<Result, any>,
 	) as Expr<Result, any>
+	infix.add(expr)
+	return expr
 }
 
 /**

src/docs-examples.test.ts

@@ -302,6 +302,17 @@ describe("docs/expressions.md", () => {
 		expect(oneLine(compileCHUnsafe(query, {}).sql)).toContain("DurationMs - 1 / 2 AS ratio")
 	})
 
+	it("An arithmetic argument keeps its parentheses", () => {
+		const query = CH.from(Events).select(($) => ({
+			left: $.DurationMs.sub($.DurationMs.add(1)),
+			half: CH.lit(0.5).mul($.DurationMs.sub(1)),
+		}))
+
+		const sql = oneLine(compileCHUnsafe(query, {}).sql)
+		expect(sql).toContain("events.DurationMs - (events.DurationMs + 1) AS left")
+		expect(sql).toContain("0.5 * (events.DurationMs - 1) AS half")
+	})
+
 	it("Combining conditions with and/or", () => {
 		const query = CH.from(Events)
 			.select(($) => ({ name: $.Name }))

tests/fuzz/compile.fuzz.test.ts

@@ -16,7 +16,7 @@ import * as PG from "@maple-dev/effect-orm/postgres"
 import type { CoreContext } from "../core-cases"
 import { scopeContexts as contexts } from "./contexts"
 import { compilePlan } from "./build"
-import { config, FuzzFailure, property, skipKnown } from "./harness"
+import { FuzzFailure, property, skipKnown } from "./harness"
 import { knownHostileIssue, knownPlanIssue } from "./known-issues"
 import { nastyOptions, ORDER_COLUMNS, plan, type OrderColumn, type Plan } from "./plan"
 
@@ -26,10 +26,8 @@ const stringsIn = (value: unknown): Array<string> =>
 const serialize = (value: unknown) => JSON.stringify(value, (_, v) => (typeof v === "bigint" ? `${v}n` : v instanceof Date ? `Date(${v.getTime()})` : v))
 
 const compiles = (ctx: CoreContext, input: Plan) => {
-	if (!config.strict) {
-		const known = knownPlanIssue({ target: ctx.dialect, plan: input })
-		if (known !== undefined) skipKnown(known)
-	}
+	const known = knownPlanIssue({ target: ctx.dialect, plan: input })
+	if (known !== undefined) skipKnown(known)
 	const first = compilePlan(ctx, input)
 	if (first._tag !== "Compiled") throw new FuzzFailure(`a valid plan was not compiled (${first._tag})`, { error: first.error })
 	const { compiled } = first
@@ -113,10 +111,8 @@ const hostileInput = (dialect: "clickhouse" | "postgres"): fc.Arbitrary<HostileI
 	})
 
 const neverThrows = (ctx: CoreContext, input: HostileInput) => {
-	if (!config.strict) {
-		const known = knownHostileIssue(input)
-		if (known !== undefined) skipKnown(known)
-	}
+	const known = knownHostileIssue(input)
+	if (known !== undefined) skipKnown(known)
 	let exit: Exit.Exit<unknown, unknown>
 	try {
 		const { query, params } = hostileQuery(ctx, input)

tests/fuzz/harness.ts

@@ -9,8 +9,8 @@
 //     command that replays it (`FUZZ_SEED`, `FUZZ_PATH`);
 //   - inputs pinned in `regressions.ts` are replayed before any random one, so
 //     a counterexample stays a test after the bug is fixed;
-//   - `known` issues are skipped by name (see `known-issues.ts`), and
-//     `FUZZ_STRICT=1` turns them back on to check whether one is fixed.
+//   - `known` issues are skipped by name (see `known-issues.ts`);
+//     `FUZZ_STRICT=1` turns them all back on, `FUZZ_STRICT=<id>,<id>` just those.
 import fc from "fast-check"
 import { pinnedFor } from "./regressions"
 
@@ -25,7 +25,8 @@ export const config = {
 	runs: int("FUZZ_RUNS"),
 	seed: int("FUZZ_SEED"),
 	path: process.env.FUZZ_PATH || undefined,
-	strict: process.env.FUZZ_STRICT === "1",
+	/** `1`: ignore every known issue. A comma list of ids: ignore just those (to verify one fix). */
+	strict: new Set((process.env.FUZZ_STRICT ?? "").split(",").filter(Boolean)),
 	verbose: process.env.FUZZ_VERBOSE === "1",
 }
 

tests/fuzz/known-issues.ts

@@ -4,6 +4,7 @@
 // while it is open and the rest of the input space keeps being fuzzed. This file
 // is the backlog: fix the bug, delete its entry, and run with `FUZZ_STRICT=1`
 // to confirm nothing else matched it. An entry needs a reason a person can act on.
+import { config } from "./harness"
 import type { LiteralType } from "./values"
 
 export interface KnownIssue<A> {
@@ -12,6 +13,10 @@ export interface KnownIssue<A> {
 	readonly matches: (input: A) => boolean
 }
 
+/** The first open issue `input` reaches, unless FUZZ_STRICT re-enabled it. */
+const firstMatch = <A>(issues: ReadonlyArray<KnownIssue<A>>, input: A): string | undefined =>
+	issues.find((issue) => !config.strict.has("1") && !config.strict.has(issue.id) && issue.matches(input))?.id
+
 export type LiteralPath = "compare" | "select"
 export interface LiteralInput {
 	readonly literal: LiteralType
@@ -47,7 +52,7 @@ const literalIssues: ReadonlyArray<KnownIssue<LiteralInput>> = [
 
 /** The id of the open issue this input would hit, if any. */
 export const knownLiteralIssue = (input: LiteralInput): string | undefined =>
-	literalIssues.find((issue) => issue.matches(input))?.id
+	firstMatch(literalIssues, input)
 
 // Hostile values (compile.fuzz.test.ts)
 
@@ -101,7 +106,7 @@ const hostileIssues: ReadonlyArray<KnownIssue<HostileInput>> = [
 ]
 
 export const knownHostileIssue = (input: HostileInput): string | undefined =>
-	hostileIssues.find((issue) => issue.matches(input))?.id
+	firstMatch(hostileIssues, input)
 
 // Query plans (queries.fuzz.test.ts)
 
@@ -121,8 +126,6 @@ const nodes = function* (value: unknown): Generator<Record<string, unknown>> {
 		for (const item of Object.values(value)) yield* nodes(item)
 	}
 }
-const precedence = (op: unknown) => (op === "add" || op === "sub" ? 1 : 2)
-const isArith = (e: unknown): e is { op: string; l: unknown; r: unknown } => (e as any)?.k === "arith"
 
 /** The conjuncts a WHERE list ANDs together, flattening nested ANDs. */
 const conjuncts = (conds: ReadonlyArray<unknown>): Array<unknown> =>
@@ -160,18 +163,6 @@ const planIssues: ReadonlyArray<KnownIssue<PlanInput>> = [
 			target === "postgres" &&
 			[...nodes(plan)].some((node) => node.k === "value" && typeof node.v === "number" && !Number.isInteger(node.v)),
 	},
-	{
-		id: "arith-argument-not-parenthesised",
-		reason:
-			"A compound expression passed as an arithmetic argument loses its boundary: `$.a.sub($.b.add(1))` compiles to " +
-			"`a - b + 1`, `$.a.div($.b.mul(2))` to `a / b * 2`. docs/expressions.md documents that *chains* follow SQL " +
-			"precedence (the model applies that), but an argument is not a chain, and no call order expresses " +
-			"`a - (b + 1)`. Parenthesising a compound right operand would fix it without changing chained SQL.",
-		matches: ({ plan }) =>
-			[...nodes(plan)].some(
-				(node) => isArith(node) && isArith(node.r) && precedence(node.r.op) <= precedence(node.op),
-			),
-	},
 	{
 		id: "fromquery-select-all",
 		reason:
@@ -181,4 +172,4 @@ const planIssues: ReadonlyArray<KnownIssue<PlanInput>> = [
 	},
 ]
 
-export const knownPlanIssue = (input: PlanInput): string | undefined => planIssues.find((issue) => issue.matches(input))?.id
+export const knownPlanIssue = (input: PlanInput): string | undefined => firstMatch(planIssues, input)

tests/fuzz/literals.fuzz.test.ts

@@ -18,7 +18,7 @@ import { Effect, Exit } from "effect"
 import * as CH from "@maple-dev/effect-orm/clickhouse"
 import * as PG from "@maple-dev/effect-orm/postgres"
 import { clickhouseEndpoint, closePostgres, run } from "./db"
-import { config, FuzzFailure, property, skip, skipKnown, withContext } from "./harness"
+import { FuzzFailure, property, skip, skipKnown, withContext } from "./harness"
 import { knownLiteralIssue, type LiteralPath } from "./known-issues"
 import { literalTypes, normalize, referenceJson, wellFormed, type LiteralType } from "./values"
 
@@ -35,10 +35,8 @@ const build = (literal: LiteralType, value: unknown, path: LiteralPath) => {
 const roundTrip = async (literal: LiteralType, value: unknown, path: LiteralPath): Promise<void> => {
 	// The reference row goes through JSON, which cannot carry a lone surrogate the way SQL text does.
 	if (!wellFormed(value)) skip("lone surrogate")
-	if (!config.strict) {
-		const known = knownLiteralIssue({ literal, value, path })
-		if (known !== undefined) skipKnown(known)
-	}
+	const known = knownLiteralIssue({ literal, value, path })
+	if (known !== undefined) skipKnown(known)
 	const M = literal.dialect === "clickhouse" ? CH : PG
 	const exit = Effect.runSyncExit((M.compile as any)(build(literal, value, path), { p: value }))
 	if (Exit.isFailure(exit)) throw new FuzzFailure("the builder refused a valid value", { value, cause: String(exit.cause) })

tests/fuzz/model.ts

@@ -90,15 +90,16 @@ const chKind = (e: Expr | number): ChKind => {
 // Expressions
 
 /**
- * Arithmetic as the SQL text parses it. The builder writes `${lhs} ${op} ${rhs}`
- * without parentheses — documented in docs/expressions.md ("Arithmetic does not
- * parenthesise"): a chain follows SQL precedence, not call order. So the model
- * flattens a tree to the token sequence the SQL contains and re-parses it with
- * `*` `/` `%` binding tighter than `+` `-`, both left-associative.
+ * Arithmetic as the SQL text parses it. A chain is not parenthesised —
+ * documented in docs/expressions.md ("Arithmetic does not parenthesise"), it
+ * follows SQL precedence, not call order — while an argument keeps its own
+ * parentheses. So the model flattens the receiver chain to the token sequence
+ * the SQL contains, keeps each argument whole, and re-parses with `*` `/` `%`
+ * binding tighter than `+` `-`, both left-associative.
  */
 const parsed = new WeakSet<Expr>()
 const flatten = (e: Expr): Array<Expr | ArithOp> =>
-	e.k === "arith" ? [...flatten(e.l), e.op, ...(typeof e.r === "number" ? [{ k: "num", v: e.r } as Expr] : flatten(e.r))] : [e]
+	e.k === "arith" ? [...flatten(e.l), e.op, typeof e.r === "number" ? ({ k: "num", v: e.r } as Expr) : e.r] : [e]
 const asParsed = (e: Expr & { k: "arith" }): Expr => {
 	const tokens = flatten(e)
 	const node = (op: ArithOp, l: Expr, r: Expr): Expr => {

tests/fuzz/queries.fuzz.test.ts

@@ -12,7 +12,7 @@ import type { CoreContext } from "../core-cases"
 import { contexts, scopeContexts } from "./contexts"
 import { compilePlan } from "./build"
 import { clickhouseEndpoint, closePostgres, run, type Target } from "./db"
-import { config, FuzzFailure, property, skipKnown, withContext } from "./harness"
+import { FuzzFailure, property, skipKnown, withContext } from "./harness"
 import { knownPlanIssue } from "./known-issues"
 import { evaluate } from "./model"
 import { nastyOptions, plainOptions, plan, type Plan } from "./plan"
@@ -30,10 +30,8 @@ const canonical = (rows: ReadonlyArray<unknown>): Array<unknown> =>
 const sorted = (rows: Array<unknown>) => rows.map((r) => JSON.stringify(r)).sort()
 
 const matchesModel = async (ctx: CoreContext, target: Target, input: Plan) => {
-	if (!config.strict) {
-		const known = knownPlanIssue({ target, plan: input })
-		if (known !== undefined) skipKnown(known)
-	}
+	const known = knownPlanIssue({ target, plan: input })
+	if (known !== undefined) skipKnown(known)
 	const expected = evaluate(input, target)
 	const result = compilePlan(ctx, input)
 	if (result._tag !== "Compiled") throw new FuzzFailure(`a valid plan was not compiled (${result._tag})`, { error: result.error })