chore: release 3.2.0 — parenthesize operator operands, add Expr::Paren

CHANGELOG.md

@@ -1,5 +1,33 @@
 # Changelog
 
+## 3.2.0
+
+### Fixed
+- **Operator operands are now parenthesized when their own structure requires it.** Renderers emitted no grouping, so an operand that was itself an operator expression got re-associated by the engine's precedence rules — two different ASTs rendered to the same SQL. `Cast(JsonPathText(data, 'age'), "bigint")` produced `data->>'age'::bigint`, which PostgreSQL reads as `data ->> ('age'::bigint)` and rejects with `invalid input syntax for type bigint`. Worse, `Binary(Binary(1, +, 2), *, 3)` produced `1 + 2 * 3` and silently evaluated to 7 instead of 9, with no error from the database. Operands of `Binary`, `Unary`, `Cast`, `Collate`, `JsonPathText` and of both sides of a comparison are now bracketed when they are themselves `Binary`, `Unary`, `Collate`, `JsonPathText` or `Window`.
+
+  Bracketing is structural rather than driven by a precedence table, because precedence is dialect-specific: SQLite binds `||` tighter than `*`, PostgreSQL binds it looser than `+`, so the same AST needs different brackets per dialect.
+
+  The same defect reached through `Expr::Field`: a `FieldDef` carrying a `child` renders a JSON path chain (`"data"->'age'`), so `Cast(Field(data->age), "bigint")` produced `"data"->'age'::bigint` — bracketed now as well. Plain fields are bare identifiers and stay unbracketed.
+
+  Two further operand positions were missed on the first pass and are now covered: the right operand of PostgreSQL's `?|` / `?&` (which also feeds a trailing `::text[]`), and the right operand of SQLite's `IRegex` (the RHS of a `||` concatenation).
+
+### Added
+- **`Custom*` nodes now render themselves.** Every `Custom*` trait gained `render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx)`, and `CustomExpr` also gained `needs_operand_parens()`. The node is handed the renderer, so it recurses back through it for its own sub-expressions — nested expressions, identifier quoting and parameter numbering all go through the same dialect and the same `RenderCtx`. Both default to an error / `true`, so nothing existing breaks and an unrenderable node still fails loudly.
+
+  This makes the extension story actually work. Previously `Expr::Custom` and `ConditionNode::Custom` returned "must be handled by a wrapping renderer", but such a wrapper could not be written: `delegate_renderer!` emits every trait method, so combining it with an override is a duplicate definition (`E0201`), and even a hand-written wrapper never saw the call — a dialect's `render_query` recurses through its own concrete `self`, never back out to the wrapper. Teaching qcraft a new syntax now needs no wrapper at all: implement `render` on the node. qcraft's own `PgVectorOp` (`<->`, `<#>`, …) is now implemented exactly this way, through the same public mechanism users get.
+
+- `Expr::needs_operand_parens()` and the `Renderer::render_operand()` default method, which together implement the rule above. A `CustomExpr` author calls `renderer.render_operand()` for its own operands and answers `needs_operand_parens()` for itself, so a user-defined infix node (`x AT TIME ZONE 'UTC'`) is bracketed correctly under a cast.
+- `Renderer::needs_operand_parens()` — the extension point a dialect overrides when its own rendering of a node already delimits it (SQLite renders `Power` as `power(l, r)` and `BitwiseXor` as a bracketed composite, so neither takes a second pair of brackets). Overriding the predicate rather than `render_operand` keeps the bracketing logic single-sourced.
+- `delegate_renderer!` forwards `needs_operand_parens`, so a wrapping renderer keeps the inner dialect's rule instead of falling back to the core default.
+
+### Changed
+- Generated SQL now carries brackets where operand structure demands them (values are unchanged; only the SQL text differs). Snapshot tests that compare rendered SQL byte-for-byte may need updating — for example a `COLLATE` operand in a comparison now renders as `("users"."name" COLLATE "C") = $1`.
+- Unary operators render via `keyword()` instead of `write()`, so `SELECT- x` is now `SELECT - x`.
+
+### Notes
+- `Expr::Raw` and `Expr::Custom` are **never** bracketed automatically: their contents are opaque and need not be an expression at all. A caller who needs grouping writes it into the fragment itself — `Expr::cast(Expr::raw("(price * qty)"), "numeric")` renders `(price * qty)::numeric`, while `Expr::raw("price * qty")` renders `price * qty::numeric` (the cast lands on `qty`).
+- No new `Expr` variant is added, so exhaustive `match` over `Expr` still compiles.
+
 ## 3.1.0
 
 ### Added

Cargo.toml

@@ -9,7 +9,7 @@ members = [
 ]
 
 [workspace.package]
-version = "3.1.0"
+version = "3.2.0"
 edition = "2024"
 rust-version = "1.85"
 license = "MIT OR Apache-2.0"
@@ -19,7 +19,7 @@ categories = ["database"]
 readme = "README.md"
 
 [workspace.dependencies]
-qcraft-core = { path = "crates/qcraft-core", version = "3.1.0" }
-qcraft-postgres = { path = "crates/qcraft-postgres", version = "3.1.0" }
-qcraft-sqlite = { path = "crates/qcraft-sqlite", version = "3.1.0" }
+qcraft-core = { path = "crates/qcraft-core", version = "3.2.0" }
+qcraft-postgres = { path = "crates/qcraft-postgres", version = "3.2.0" }
+qcraft-sqlite = { path = "crates/qcraft-sqlite", version = "3.2.0" }
 thiserror = "2"

crates/qcraft-core/src/ast/custom.rs

@@ -1,6 +1,10 @@
 use std::any::Any;
 use std::fmt::Debug;
 
+use crate::error::{RenderError, RenderResult};
+use crate::render::ctx::RenderCtx;
+use crate::render::renderer::Renderer;
+
 // ---------------------------------------------------------------------------
 // Macro to define Custom* traits with common boilerplate
 // ---------------------------------------------------------------------------
@@ -10,6 +14,25 @@ macro_rules! define_custom_trait {
         pub trait $trait_name: Debug + Send + Sync {
             fn as_any(&self) -> &dyn Any;
             fn clone_box(&self) -> Box<dyn $trait_name>;
+
+            /// Render this node. The extension lives in the node itself, not in a renderer
+            /// wrapped around the dialect: rendering recurses back through `renderer`, so
+            /// nested expressions, identifier quoting and parameter numbering all go
+            /// through the same dialect and the same [`RenderCtx`] as everything else.
+            ///
+            /// The default is an error, keeping the contract that an unrenderable custom
+            /// node fails loudly rather than silently dropping SQL.
+            fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> {
+                let _ = (renderer, ctx);
+                Err(RenderError::unsupported(
+                    stringify!($trait_name),
+                    concat!(
+                        "implement ",
+                        stringify!($trait_name),
+                        "::render to teach the renderer this node"
+                    ),
+                ))
+            }
         }
 
         impl Clone for Box<dyn $trait_name> {
@@ -20,7 +43,6 @@ macro_rules! define_custom_trait {
     };
 }
 
-define_custom_trait!(CustomExpr);
 define_custom_trait!(CustomCondition);
 define_custom_trait!(CustomCompareOp);
 define_custom_trait!(CustomTableSource);
@@ -30,3 +52,40 @@ define_custom_trait!(CustomFieldType);
 define_custom_trait!(CustomBinaryOp);
 define_custom_trait!(CustomConstraint);
 define_custom_trait!(CustomTransaction);
+
+/// A user-defined expression node.
+///
+/// Defined by hand rather than through `define_custom_trait!` because, unlike the other
+/// custom nodes, an expression can appear as the operand of an operator and therefore has
+/// to answer whether it needs brackets there.
+pub trait CustomExpr: Debug + Send + Sync {
+    fn as_any(&self) -> &dyn Any;
+    fn clone_box(&self) -> Box<dyn CustomExpr>;
+
+    /// Render this node. See [`CustomCondition::render`] — same contract: the node renders
+    /// itself and recurses back through `renderer` for any sub-expression it holds.
+    fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> {
+        let _ = (renderer, ctx);
+        Err(RenderError::unsupported(
+            "CustomExpr",
+            "implement CustomExpr::render to teach the renderer this node",
+        ))
+    }
+
+    /// Whether this node needs brackets when it is the operand of an operator
+    /// (`+`, `::`, `COLLATE`, a comparison, …). Only the author knows the shape it renders.
+    ///
+    /// Defaults to `true` — conservative, because a node rendering an infix form
+    /// (`x AT TIME ZONE 'UTC'`) would otherwise be re-associated by the engine's
+    /// precedence, which is the exact class of bug operand bracketing exists to prevent.
+    /// A node rendering a self-delimiting form (`my_func(x)`) should return `false`.
+    fn needs_operand_parens(&self) -> bool {
+        true
+    }
+}
+
+impl Clone for Box<dyn CustomExpr> {
+    fn clone(&self) -> Self {
+        self.clone_box()
+    }
+}

crates/qcraft-core/src/ast/expr.rs

@@ -273,6 +273,42 @@ impl Expr {
         Expr::Now
     }
 
+    /// True if this expression must be parenthesized when it appears as the operand
+    /// of an operator (`+`, `::`, `COLLATE`, `->>`, a comparison, …).
+    ///
+    /// An operator expression carries its grouping in the tree, but SQL text carries
+    /// it in brackets: printed flat, `Binary(Binary(1, +, 2), *, 3)` becomes
+    /// `1 + 2 * 3`, which every engine reads back as `Binary(1, +, Binary(2, *, 3))`
+    /// — 7 instead of 9. Bracketing is structural rather than driven by a precedence
+    /// table because precedence is dialect-specific: SQLite binds `||` tighter than
+    /// `*`, PostgreSQL binds it looser than `+`.
+    ///
+    /// A [`Expr::Field`] whose [`FieldDef`](crate::ast::common::FieldDef) carries a
+    /// `child` renders as a JSON path chain (`"data"->'age'`) — an operator expression
+    /// like [`Expr::JsonPathText`], and bracketed for the same reason. A plain field
+    /// is a bare identifier and is not.
+    ///
+    /// Self-delimiting forms (literals, identifiers, function calls, `CAST(…)`,
+    /// `CASE … END`, subqueries, tuples) carry their own boundaries and render bare.
+    ///
+    /// `Raw` and `Custom` are opaque escape hatches whose contents need not be an
+    /// expression at all, so they are never bracketed automatically. A caller who needs
+    /// grouping writes it into the fragment itself — `Expr::raw("(price * qty)")` — and
+    /// a `CustomExpr` author controls their own rendering.
+    pub fn needs_operand_parens(&self) -> bool {
+        match self {
+            Expr::Binary { .. }
+            | Expr::Unary { .. }
+            | Expr::Collate { .. }
+            | Expr::JsonPathText { .. }
+            | Expr::Window(_) => true,
+            Expr::Field(field_ref) => field_ref.field.child.is_some(),
+            // Only the node's author knows the shape it renders.
+            Expr::Custom(custom) => custom.needs_operand_parens(),
+            _ => false,
+        }
+    }
+
     /// True if this expression tree contains an unbound `Expr::Param` placeholder.
     /// Used to reject double-render forms that would corrupt positional binding.
     /// Does not descend into subquery `QueryStmt`s (those are rejected separately).

crates/qcraft-core/src/render/renderer.rs

@@ -41,6 +41,32 @@ pub trait Renderer {
     // ── Expressions ──
 
     fn render_expr(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()>;
+
+    /// Whether `expr` needs brackets in an operand position. Defaults to
+    /// [`Expr::needs_operand_parens`]; a dialect overrides this — not
+    /// [`Renderer::render_operand`] — when its own rendering of a node already
+    /// delimits it (SQLite renders `Power` as `power(l, r)`, for instance).
+    fn needs_operand_parens(&self, expr: &Expr) -> bool {
+        expr.needs_operand_parens()
+    }
+
+    /// Render `expr` where it is the operand of an operator (`+`, `::`, `COLLATE`,
+    /// `->>`, a comparison, …), bracketing it when its own structure would otherwise
+    /// be re-associated by the engine's operator precedence.
+    ///
+    /// See [`Expr::needs_operand_parens`] for which forms are bracketed and why this
+    /// is structural rather than precedence-driven.
+    fn render_operand(&self, expr: &Expr, ctx: &mut RenderCtx) -> RenderResult<()> {
+        if self.needs_operand_parens(expr) {
+            ctx.paren_open();
+            self.render_expr(expr, ctx)?;
+            ctx.paren_close();
+            Ok(())
+        } else {
+            self.render_expr(expr, ctx)
+        }
+    }
+
     fn render_aggregate(&self, agg: &AggregationDef, ctx: &mut RenderCtx) -> RenderResult<()>;
     fn render_window(&self, win: &WindowDef, ctx: &mut RenderCtx) -> RenderResult<()>;
     fn render_case(&self, case: &CaseDef, ctx: &mut RenderCtx) -> RenderResult<()>;
@@ -72,14 +98,25 @@ pub trait Renderer {
     fn render_index_def(&self, idx: &IndexDef, ctx: &mut RenderCtx) -> RenderResult<()>;
 }
 
-/// Macro to delegate all Renderer methods to an inner renderer.
+/// Delegates every [`Renderer`] method to an inner renderer.
+///
+/// The macro emits **all** methods, so it cannot be combined with overriding one —
+/// that is a duplicate definition (`error[E0201]`). It is for wrapping a dialect
+/// wholesale, not for extending it.
+///
+/// To teach the renderer a syntax it does not know, do **not** wrap the renderer:
+/// implement [`CustomExpr::render`](crate::ast::custom::CustomExpr::render) on the node
+/// itself. The node is handed the renderer and recurses back through it, so nested
+/// expressions, quoting and parameter numbering stay consistent:
 ///
-/// Usage:
 /// ```ignore
-/// struct MyRenderer { inner: PostgresRenderer }
-/// impl Renderer for MyRenderer {
-///     fn render_cast(&self, ...) { /* custom */ }
-///     delegate_renderer!(self.inner);
+/// impl CustomExpr for AtTimeZone {
+///     fn render(&self, renderer: &dyn Renderer, ctx: &mut RenderCtx) -> RenderResult<()> {
+///         renderer.render_operand(&self.expr, ctx)?;   // brackets the operand if needed
+///         ctx.keyword("AT TIME ZONE").string_literal(&self.zone);
+///         Ok(())
+///     }
+///     fn needs_operand_parens(&self) -> bool { true }  // infix — bracket me as an operand
 /// }
 /// ```
 #[macro_export]
@@ -176,6 +213,9 @@ macro_rules! delegate_renderer {
         ) -> $crate::error::RenderResult<()> {
             $self.$inner.render_expr(expr, ctx)
         }
+        fn needs_operand_parens(&$self, expr: &$crate::ast::expr::Expr) -> bool {
+            $self.$inner.needs_operand_parens(expr)
+        }
         fn render_aggregate(
             &$self,
             agg: &$crate::ast::expr::AggregationDef,