Apply suggestion from PR, and increase the recursion limit thanks to reduction on Err type

Author: mamcx

crates/core/src/error.rs

@@ -82,8 +82,6 @@ pub enum DatabaseError {
     DatabasedOpened(PathBuf, anyhow::Error),
 }
 
-// FIXME: reduce type size
-#[expect(clippy::large_enum_variant)]
 #[derive(Error, Debug, EnumAsInner)]
 pub enum DBError {
     #[error("LibError: {0}")]

crates/core/src/host/module_host.rs

@@ -539,8 +539,6 @@ pub enum InitDatabaseError {
     Other(anyhow::Error),
 }
 
-// FIXME: reduce type size
-#[expect(clippy::large_enum_variant)]
 #[derive(thiserror::Error, Debug)]
 pub enum ClientConnectedError {
     #[error(transparent)]

crates/core/src/sql/execute.rs

@@ -1127,8 +1127,10 @@ pub(crate) mod tests {
     }
 
     /// Test we are protected against recursion when:
-    /// 1. The query is too large
+    /// 1. The query is too large (too many characters)
     /// 2. The AST is too deep
+    ///
+    /// Exercise the limit [`recursion::MAX_RECURSION_EXPR`]
     #[test]
     fn test_large_query_no_panic() -> ResultTest<()> {
         let db = TestDB::durable()?;
@@ -1143,13 +1145,11 @@ pub(crate) mod tests {
 
         let build_query = |total| {
             let mut sql = "select * from test where ".to_string();
-            for x in 0..total {
-                for y in 0..total {
-                    let fragment = format!("((x = {x}) and (y = {y})) or ");
-                    sql.push_str(&fragment);
-                }
+            for x in 1..total {
+                let fragment = format!("x = {x} or ");
+                sql.push_str(&fragment.repeat((total - 1) as usize));
             }
-            sql.push_str("((x = 1000) and (y = 1000))");
+            sql.push_str("(y = 0)");
             sql
         };
         let run = |db: &RelationalDB, sep: char, sql_text: &str| {
@@ -1161,16 +1161,15 @@ pub(crate) mod tests {
             Err("SQL query exceeds maximum allowed length".to_string())
         );
 
-        // Exercise the limit [recursion::MAX_RECURSION_EXPR] && [recursion::MAX_RECURSION_TYP_EXPR]
-        let sql = build_query(8);
+        let sql = build_query(41); // This causes stack overflow without the limit
         assert_eq!(run(&db, ',', &sql), Err("Recursion limit exceeded".to_string()));
 
-        let sql = build_query(7);
+        let sql = build_query(40); // The max we can with the current limit
         assert!(run(&db, ',', &sql).is_ok(), "Expected query to run without panic");
 
         // Check no overflow with lot of joins
         let mut sql = "SELECT test.* FROM test ".to_string();
-        // We could pust up to 700 joins without overflow as long we don't have any conditions,
+        // We could push up to 700 joins without overflow as long we don't have any conditions,
         // but here execution become too slow.
         // TODO: Move this test to the `Plan`
         for i in 0..200 {

crates/expr/src/check.rs

@@ -99,7 +99,7 @@ pub trait TypeChecker {
                     vars.insert(rhs.alias.clone(), rhs.schema.clone());
 
                     if let Some(on) = on {
-                        if let Expr::BinOp(BinOp::Eq, a, b) = type_expr(vars, on, Some(&AlgebraicType::Bool), &mut 0)? {
+                        if let Expr::BinOp(BinOp::Eq, a, b) = type_expr(vars, on, Some(&AlgebraicType::Bool))? {
                             if let (Expr::Field(a), Expr::Field(b)) = (*a, *b) {
                                 join = RelExpr::EqJoin(LeftDeepJoin { lhs, rhs }, a, b);
                                 continue;

crates/expr/src/errors.rs

@@ -122,8 +122,6 @@ pub struct DuplicateName(pub String);
 #[error("`filter!` does not support column projections; Must return table rows")]
 pub struct FilterReturnType;
 
-// FIXME: reduce type size
-#[expect(clippy::large_enum_variant)]
 #[derive(Error, Debug)]
 pub enum TypingError {
     #[error(transparent)]

crates/expr/src/lib.rs

@@ -31,7 +31,7 @@ pub mod statement;
 pub(crate) fn type_select(input: RelExpr, expr: SqlExpr, vars: &Relvars) -> TypingResult<RelExpr> {
     Ok(RelExpr::Select(
         Box::new(input),
-        type_expr(vars, expr, Some(&AlgebraicType::Bool), &mut 0)?,
+        type_expr(vars, expr, Some(&AlgebraicType::Bool))?,
     ))
 }
 
@@ -69,7 +69,7 @@ pub(crate) fn type_proj(input: RelExpr, proj: ast::Project, vars: &Relvars) -> T
                     return Err(DuplicateName(alias.into_string()).into());
                 }
 
-                if let Expr::Field(p) = type_expr(vars, expr.into(), None, &mut 0)? {
+                if let Expr::Field(p) = type_expr(vars, expr.into(), None)? {
                     projections.push((alias, p));
                 }
             }
@@ -79,13 +79,13 @@ pub(crate) fn type_proj(input: RelExpr, proj: ast::Project, vars: &Relvars) -> T
     }
 }
 
-/// Type check and lower a [SqlExpr] into a logical [Expr].
-pub(crate) fn type_expr(
-    vars: &Relvars,
-    expr: SqlExpr,
-    expected: Option<&AlgebraicType>,
-    depth: &mut usize,
-) -> TypingResult<Expr> {
+// Assert we will not blow the stack with recursion:
+const _: () = assert!(size_of::<Relvars>() == 48);
+const _: () = assert!(size_of::<TypingResult<Expr>>() == 64);
+const _: () = assert!(size_of::<SqlExpr>() == 40);
+const _: () = assert!(size_of::<Option<&AlgebraicType>>() == 8);
+
+fn _type_expr(vars: &Relvars, expr: SqlExpr, expected: Option<&AlgebraicType>, depth: usize) -> TypingResult<Expr> {
     recursion::guard(depth, recursion::MAX_RECURSION_TYP_EXPR, "expr::type_expr")?;
 
     match (expr, expected) {
@@ -125,21 +125,21 @@ pub(crate) fn type_expr(
             }))
         }
         (SqlExpr::Log(a, b, op), None | Some(AlgebraicType::Bool)) => {
-            let a = type_expr(vars, *a, Some(&AlgebraicType::Bool), depth)?;
-            let b = type_expr(vars, *b, Some(&AlgebraicType::Bool), depth)?;
+            let a = _type_expr(vars, *a, Some(&AlgebraicType::Bool), depth + 1)?;
+            let b = _type_expr(vars, *b, Some(&AlgebraicType::Bool), depth + 1)?;
             Ok(Expr::LogOp(op, Box::new(a), Box::new(b)))
         }
         (SqlExpr::Bin(a, b, op), None | Some(AlgebraicType::Bool)) if matches!(&*a, SqlExpr::Lit(_)) => {
-            let b = type_expr(vars, *b, None, depth)?;
-            let a = type_expr(vars, *a, Some(b.ty()), depth)?;
+            let b = _type_expr(vars, *b, None, depth + 1)?;
+            let a = _type_expr(vars, *a, Some(b.ty()), depth + 1)?;
             if !op_supports_type(op, a.ty()) {
                 return Err(InvalidOp::new(op, a.ty()).into());
             }
             Ok(Expr::BinOp(op, Box::new(a), Box::new(b)))
         }
         (SqlExpr::Bin(a, b, op), None | Some(AlgebraicType::Bool)) => {
-            let a = type_expr(vars, *a, None, depth)?;
-            let b = type_expr(vars, *b, Some(a.ty()), depth)?;
+            let a = _type_expr(vars, *a, None, depth + 1)?;
+            let b = _type_expr(vars, *b, Some(a.ty()), depth + 1)?;
             if !op_supports_type(op, a.ty()) {
                 return Err(InvalidOp::new(op, a.ty()).into());
             }
@@ -152,6 +152,11 @@ pub(crate) fn type_expr(
     }
 }
 
+/// Type check and lower a [SqlExpr] into a logical [Expr].
+pub(crate) fn type_expr(vars: &Relvars, expr: SqlExpr, expected: Option<&AlgebraicType>) -> TypingResult<Expr> {
+    _type_expr(vars, expr, expected, 0)
+}
+
 /// Is this type compatible with this binary operator?
 fn op_supports_type(_op: BinOp, t: &AlgebraicType) -> bool {
     t.is_bool()

crates/expr/src/statement.rs

@@ -162,7 +162,7 @@ pub fn type_delete(delete: SqlDelete, tx: &impl SchemaView) -> TypingResult<Tabl
     let mut vars = Relvars::default();
     vars.insert(table_name.clone(), from.clone());
     let expr = filter
-        .map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool), &mut 0))
+        .map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool)))
         .transpose()?;
     Ok(TableDelete {
         table: from,
@@ -216,7 +216,7 @@ pub fn type_update(update: SqlUpdate, tx: &impl SchemaView) -> TypingResult<Tabl
     vars.insert(table_name.clone(), schema.clone());
     let values = values.into_boxed_slice();
     let filter = filter
-        .map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool), &mut 0))
+        .map(|expr| type_expr(&vars, expr, Some(&AlgebraicType::Bool)))
         .transpose()?;
     Ok(TableUpdate {
         table: schema,
@@ -450,15 +450,16 @@ pub fn compile_sql_stmt<'a>(sql: &'a str, tx: &impl SchemaView, auth: &AuthCtx)
 
 #[cfg(test)]
 mod tests {
-    use spacetimedb_lib::{identity::AuthCtx, AlgebraicType, ProductType};
-    use spacetimedb_schema::def::ModuleDef;
-
+    use super::Statement;
+    use crate::ast::LogOp;
     use crate::check::{
         test_utils::{build_module_def, SchemaViewer},
-        SchemaView, TypingResult,
+        Relvars, SchemaView, TypingResult,
     };
-
-    use super::Statement;
+    use crate::type_expr;
+    use spacetimedb_lib::{identity::AuthCtx, AlgebraicType, ProductType};
+    use spacetimedb_schema::def::ModuleDef;
+    use spacetimedb_sql_parser::ast::{SqlExpr, SqlLiteral};
 
     fn module_def() -> ModuleDef {
         build_module_def(vec![
@@ -519,4 +520,27 @@ mod tests {
             assert!(result.is_err());
         }
     }
+
+    // Manually build the AST for a recursive query,
+    // because we limit the length of the query to prevent stack overflow on parsing.
+    /// Exercise the limit [`recursion::MAX_RECURSION_TYP_EXPR`]
+    #[test]
+    fn typing_recursion() {
+        let build_query = |total, sep: char| {
+            let mut expr = SqlExpr::Lit(SqlLiteral::Bool(true));
+            for _ in 1..total {
+                let next = SqlExpr::Log(
+                    Box::new(SqlExpr::Lit(SqlLiteral::Bool(true))),
+                    Box::new(SqlExpr::Lit(SqlLiteral::Bool(false))),
+                    LogOp::And,
+                );
+                expr = SqlExpr::Log(Box::new(expr), Box::new(next), LogOp::And);
+            }
+            type_expr(&Relvars::default(), expr, Some(&AlgebraicType::Bool))
+                .map_err(|e| e.to_string().split(sep).next().unwrap_or_default().to_string())
+        };
+        assert_eq!(build_query(2_501, ','), Err("Recursion limit exceeded".to_string()));
+
+        assert!(build_query(2_500, ',').is_ok());
+    }
 }

crates/sql-parser/src/parser/errors.rs

@@ -50,7 +50,7 @@ pub enum SqlUnsupported {
     #[error("Unsupported FROM expression: {0}")]
     From(TableFactor),
     #[error("Unsupported set operation: {0}")]
-    SetOp(SetExpr),
+    SetOp(Box<SetExpr>),
     #[error("Unsupported INSERT expression: {0}")]
     Insert(Query),
     #[error("Unsupported INSERT value: {0}")]
@@ -94,22 +94,33 @@ pub enum SqlRequired {
 }
 
 #[derive(Error, Debug)]
-#[error("Recursion limit exceeded, `{message}` limit: {limit}")]
+#[error("Recursion limit exceeded, `{source_}`")]
 pub struct RecursionError {
-    pub(crate) limit: usize,
-    pub(crate) message: String,
+    pub(crate) source_: &'static str,
 }
 
 #[derive(Error, Debug)]
 pub enum SqlParseError {
     #[error(transparent)]
-    SqlUnsupported(#[from] SqlUnsupported),
+    SqlUnsupported(#[from] Box<SqlUnsupported>),
     #[error(transparent)]
-    SubscriptionUnsupported(#[from] SubscriptionUnsupported),
+    SubscriptionUnsupported(#[from] Box<SubscriptionUnsupported>),
     #[error(transparent)]
     SqlRequired(#[from] SqlRequired),
     #[error(transparent)]
     ParserError(#[from] ParserError),
     #[error(transparent)]
     Recursion(#[from] RecursionError),
 }
+
+impl From<SubscriptionUnsupported> for SqlParseError {
+    fn from(value: SubscriptionUnsupported) -> Self {
+        SqlParseError::SubscriptionUnsupported(Box::new(value))
+    }
+}
+
+impl From<SqlUnsupported> for SqlParseError {
+    fn from(value: SqlUnsupported) -> Self {
+        SqlParseError::SqlUnsupported(Box::new(value))
+    }
+}

crates/sql-parser/src/parser/mod.rs

@@ -68,7 +68,7 @@ trait RelParser {
                             op: BinaryOperator::Eq,
                             right,
                         },
-                        &mut 0,
+                        0,
                     )?),
                 })
             }
@@ -207,17 +207,21 @@ pub(crate) fn parse_proj(expr: Expr) -> SqlParseResult<ProjectExpr> {
     }
 }
 
+// Assert we will not blow the stack with recursion:
+const _: () = assert!(size_of::<Expr>() == 168);
+const _: () = assert!(size_of::<SqlParseResult<SqlExpr>>() == 40);
+
 /// Parse a scalar expression
-pub(crate) fn parse_expr(expr: Expr, depth: &mut usize) -> SqlParseResult<SqlExpr> {
-    fn signed_num(sign: impl Into<String>, expr: Expr) -> Result<SqlExpr, SqlUnsupported> {
+fn parse_expr(expr: Expr, depth: usize) -> SqlParseResult<SqlExpr> {
+    fn signed_num(sign: impl Into<String>, expr: Expr) -> Result<SqlExpr, Box<SqlUnsupported>> {
         match expr {
             Expr::Value(Value::Number(n, _)) => Ok(SqlExpr::Lit(SqlLiteral::Num((sign.into() + &n).into_boxed_str()))),
-            expr => Err(SqlUnsupported::Expr(expr)),
+            expr => Err(SqlUnsupported::Expr(expr).into()),
         }
     }
     recursion::guard(depth, recursion::MAX_RECURSION_EXPR, "sql-parser::parse_expr")?;
     match expr {
-        Expr::Nested(expr) => parse_expr(*expr, depth),
+        Expr::Nested(expr) => parse_expr(*expr, depth + 1),
         Expr::Value(Value::Placeholder(param)) if &param == ":sender" => Ok(SqlExpr::Param(Parameter::Sender)),
         Expr::Value(v) => Ok(SqlExpr::Lit(parse_literal(v)?)),
         Expr::UnaryOp {
@@ -243,31 +247,31 @@ pub(crate) fn parse_expr(expr: Expr, depth: &mut usize) -> SqlParseResult<SqlExp
             op: BinaryOperator::And,
             right,
         } => {
-            let l = parse_expr(*left, depth)?;
-            let r = parse_expr(*right, depth)?;
+            let l = parse_expr(*left, depth + 1)?;
+            let r = parse_expr(*right, depth + 1)?;
             Ok(SqlExpr::Log(Box::new(l), Box::new(r), LogOp::And))
         }
         Expr::BinaryOp {
             left,
             op: BinaryOperator::Or,
             right,
         } => {
-            let l = parse_expr(*left, depth)?;
-            let r = parse_expr(*right, depth)?;
+            let l = parse_expr(*left, depth + 1)?;
+            let r = parse_expr(*right, depth + 1)?;
             Ok(SqlExpr::Log(Box::new(l), Box::new(r), LogOp::Or))
         }
         Expr::BinaryOp { left, op, right } => {
-            let l = parse_expr(*left, depth)?;
-            let r = parse_expr(*right, depth)?;
+            let l = parse_expr(*left, depth + 1)?;
+            let r = parse_expr(*right, depth + 1)?;
             Ok(SqlExpr::Bin(Box::new(l), Box::new(r), parse_binop(op)?))
         }
         _ => Err(SqlUnsupported::Expr(expr).into()),
     }
 }
 
 /// Parse an optional scalar expression
-pub(crate) fn parse_expr_opt(opt: Option<Expr>, depth: &mut usize) -> SqlParseResult<Option<SqlExpr>> {
-    opt.map(|expr| parse_expr(expr, depth)).transpose()
+pub(crate) fn parse_expr_opt(opt: Option<Expr>) -> SqlParseResult<Option<SqlExpr>> {
+    opt.map(|expr| parse_expr(expr, 0)).transpose()
 }
 
 /// Parse a scalar binary operator

crates/sql-parser/src/parser/recursion.rs

@@ -5,29 +5,23 @@
 //! Removing one could allow the others to be higher, but depending on how the `SQL` is structured, it could lead to a `stack overflow`
 //! if is not guarded against, so is incorrect to assume that a limit is sufficient for the next part of the parser.
 use crate::parser::errors::{RecursionError, SqlParseError};
-use std::fmt::Display;
 
 /// A conservative limit for recursion depth on `parse_expr`.
-pub const MAX_RECURSION_EXPR: usize = 700;
+pub const MAX_RECURSION_EXPR: usize = 1_600;
 /// A conservative limit for recursion depth on `type_expr`.
-pub const MAX_RECURSION_TYP_EXPR: usize = 5_000;
+pub const MAX_RECURSION_TYP_EXPR: usize = 2_500;
 
 /// A utility for guarding against excessive recursion depth.
 ///
 /// **Usage:**
 /// ```
 /// use spacetimedb_sql_parser::parser::recursion;
 /// let mut depth = 0;
-/// assert!(recursion::guard(&mut depth, 10, "test").is_ok());
+/// assert!(recursion::guard(depth, 10, "test").is_ok());
 /// ```
-pub fn guard(depth: &mut usize, limit: usize, msg: impl Display) -> Result<(), SqlParseError> {
-    *depth += 1;
-    if *depth > limit {
-        Err(RecursionError {
-            limit,
-            message: msg.to_string(),
-        }
-        .into())
+pub fn guard(depth: usize, limit: usize, source: &'static str) -> Result<(), SqlParseError> {
+    if depth > limit {
+        Err(RecursionError { source_: source }.into())
     } else {
         Ok(())
     }