Support database queries and executes in return expressions - #561
Conversation
`return query <db> with <sql> and parameters [...]` (and the `execute` form) failed with "Unexpected token in expression: KeywordParameters" because the database query form only existed as the value side of a `store` statement. Without parameters it silently parsed as a concatenation, which failed at runtime. Add an Expression::DatabaseQuery AST variant, share its parsing with the existing `store ... as query/execute` path, and recognize the form in return position using the same strict lookahead. The interpreter evaluates it via logic extracted from DatabaseQueryStatement, and the typechecker gives it the same row typing as the statement form. The analyzer walks its sub-expressions and the JS transpiler rejects it like the database statements. Closes #559 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVZX79bHtePpTnX6mH4jeZ
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a new ChangesDatabase query as expression
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Action as WFL Action
participant Parser
participant Interpreter
participant DBPool as Database Pool
Action->>Parser: return query db with sql and parameters [...]
Parser->>Parser: parse_database_query_expression()
Parser-->>Interpreter: Expression::DatabaseQuery
Interpreter->>Interpreter: evaluate_database_query()
Interpreter->>DBPool: run_query / run_execute
DBPool-->>Interpreter: rows / affected_rows
Interpreter-->>Action: returned Value
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds first-class support for using query / execute database operations in expression position—most notably in return/give back statements—so actions can directly return database results without an intermediate store ... as.
Changes:
- Extended the AST with
Expression::DatabaseQueryand wired it through parser, analyzer, type checker, and interpreter evaluation. - Refactored interpreter database execution into a shared
evaluate_database_query()helper used by both statement and expression forms. - Added parser + interpreter tests, updated SQLite E2E program, and documented the new return pattern (JS transpilation explicitly errors for these expressions).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/parser/stmt/database.rs |
Extracts DB query/execute parsing into an expression parser and reuses it for statement parsing. |
src/parser/stmt/actions.rs |
Enables return/give back to parse DB query/execute forms as expressions. |
src/parser/ast.rs |
Adds Expression::DatabaseQuery AST variant. |
src/analyzer/mod.rs |
Analyzes sub-expressions inside Expression::DatabaseQuery. |
src/typechecker/mod.rs |
Type-checks Expression::DatabaseQuery and assigns correct result types for query vs execute. |
src/interpreter/mod.rs |
Adds evaluation for Expression::DatabaseQuery and shares logic with statement execution. |
src/transpiler/javascript.rs |
Rejects DB expressions during JS transpilation with a clear error. |
src/parser/stmt/io.rs |
Updates display parsing match to account for Expression::DatabaseQuery. |
tests/database_parser_test.rs |
Adds parser-level coverage for return-position DB query/execute forms and compatibility cases. |
tests/database_test.rs |
Adds interpreter end-to-end tests for returning DB query/execute results from actions. |
TestPrograms/database_sqlite_test.wfl |
Adds E2E scenarios validating return-position query/execute. |
Docs/04-advanced-features/databases.md |
Documents returning query/execute results directly from actions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let Expression::DatabaseQuery { | ||
| db, | ||
| sql, | ||
| parameters, | ||
| kind, | ||
| .. | ||
| } = self.parse_database_query_expression(kind, line, column)? | ||
| else { | ||
| unreachable!("parse_database_query_expression always returns DatabaseQuery"); | ||
| }; | ||
|
|
||
| Ok(Statement::DatabaseQueryStatement { | ||
| db: *db, | ||
| sql: *sql, | ||
| parameters: parameters.map(|p| *p), | ||
| variable_name: name, | ||
| kind, | ||
| line, | ||
| column, | ||
| }) |
| other => panic!("Expected DatabaseQuery expression, got {other:?}"), | ||
| } | ||
| } | ||
|
|
| let rows = expect_list(&get_global(&interpreter, "rows")); | ||
| assert_eq!(expect_number(&expect_object_key(&rows[0], "id")), 42.0); | ||
| } | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/typechecker/mod.rs (1)
3324-3380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffType inference and result typing here correctly mirror the
DatabaseQueryStatementarm (Lines 1057-1119) and match the runtime result shapes exercised by the tests. Logic is sound.Optional: the db/sql/parameters validation block (Lines 3332-3371) is a near-verbatim copy of the statement arm at Lines 1066-1105. Consider extracting a small helper (e.g.
check_database_query_operands(db, sql, parameters, line, column)) plus a shareddatabase_result_type(kind)so the two call sites can't silently drift apart. Not blocking.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/typechecker/mod.rs` around lines 3324 - 3380, The DatabaseQuery expression validation is duplicated with the DatabaseQueryStatement arm, so the two paths can drift over time. Extract the shared db/sql/parameters checks from the `Expression::DatabaseQuery` branch into a helper such as `check_database_query_operands`, and centralize the result-shape mapping into a small `database_result_type(kind)` helper so both `infer_expression_type` call sites use the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/typechecker/mod.rs`:
- Around line 3324-3380: The DatabaseQuery expression validation is duplicated
with the DatabaseQueryStatement arm, so the two paths can drift over time.
Extract the shared db/sql/parameters checks from the `Expression::DatabaseQuery`
branch into a helper such as `check_database_query_operands`, and centralize the
result-shape mapping into a small `database_result_type(kind)` helper so both
`infer_expression_type` call sites use the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b795c8df-09e9-4fd0-8c52-e0a581425a0c
📒 Files selected for processing (12)
Docs/04-advanced-features/databases.mdTestPrograms/database_sqlite_test.wflsrc/analyzer/mod.rssrc/interpreter/mod.rssrc/parser/ast.rssrc/parser/stmt/actions.rssrc/parser/stmt/database.rssrc/parser/stmt/io.rssrc/transpiler/javascript.rssrc/typechecker/mod.rstests/database_parser_test.rstests/database_test.rs
- Drop the shadowed `kind` binding when destructuring the parsed database query expression (Copilot). - Add parser and interpreter tests for the no-parameters `return execute` form (Copilot). - Extract shared typechecker helpers `check_database_query_operands` and `database_result_type` so the statement and expression arms cannot drift apart (CodeRabbit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVZX79bHtePpTnX6mH4jeZ
| } else if let Some(kind) = self.peek_database_query_kind() { | ||
| // Database forms: `return query/execute <db> with <sql> | ||
| // [and parameters <list>]`, mirroring the `store ... as` value side. | ||
| Some(self.parse_database_query_expression( | ||
| kind, | ||
| return_token.line, | ||
| return_token.column, | ||
| )?) |
Pass the query/execute token position (not the return keyword) to parse_database_query_expression so runtime/type errors highlight the actual database operation (Copilot review feedback). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVZX79bHtePpTnX6mH4jeZ
test_constant_time_measures exceeded its 150% coefficient-of-variation threshold (164.57%) on a shared runner; unrelated to this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVZX79bHtePpTnX6mH4jeZ
test_constant_time_measures asserted on a single 50-iteration timing round, so one scheduling hiccup on a shared CI runner (e.g. 164.57% observed) failed the build. Run 100 rounds instead: log any round whose coefficient of variation exceeds 150%, but fail only when the mean across all rounds does — a descheduled runner skews one round, a real timing leak skews the mean. The 150% threshold and 10ms completion check are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVZX79bHtePpTnX6mH4jeZ
| let rounds = 100; | ||
| let iterations = 50; | ||
| let threshold = 1.5; // 150% coefficient of variation |
There was a problem hiding this comment.
Measured: the full wflhash_security_test file runs in ~0.3s including the 5,000 hash calls (the input is a short string, so each hash is sub-microsecond). The 100-round count was the maintainer's explicit request, and averaging over more rounds is what makes the mean assertion robust against scheduler noise, so keeping it as-is.
Generated by Claude Code
| // Timing tests are inherently unreliable in CI environments with | ||
| // shared resources: a single measurement round can spike well past any | ||
| // reasonable threshold when the runner gets descheduled. Instead of | ||
| // asserting on one round, run many rounds, log any round that exceeds | ||
| // the threshold, and fail only if the MEAN variation across all rounds | ||
| // does — a scheduling hiccup skews one round, a real timing leak skews | ||
| // the mean. |
There was a problem hiding this comment.
This change was explicitly requested by the maintainer after this test flaked on this PR's CI run (164.57% on a single round, unrelated to the database change), so it stays in this PR rather than being split out. It's now called out in its own "Out-of-Scope Change" section in the PR description.
Generated by Claude Code
There was a problem hiding this comment.
Maintainer note on why this is fixed here rather than split out: the flake surfaced on this PR's own CI run, and deferring it to a separate issue/PR would have left it failing builds — this one and any other open PR — in the meantime. The working policy on this repo is that when a flake pops up, it gets fixed where it pops up; that's how CI stays green. The change is test-methodology only (mean over 100 rounds instead of a single-round assertion), the 150% threshold and the hash implementation are untouched, and it's called out in the PR description for independent review.
Generated by Claude Code
Summary
Adds support for using
queryandexecutedatabase operations directly in return expressions (e.g.,return query conn with "SELECT ..." and parameters [...]), enabling concise data-access helper actions. Previously, database operations could only be used in statement form withstore ... as.Key Changes
Parser Enhancement (
src/parser/stmt/database.rs):parse_database_query_expression()methodparse_database_query_statement()to reuse the expression parserquery/executein return position with full parameter supportAST Extension (
src/parser/ast.rs):Expression::DatabaseQueryvariant to represent database operations as expressionsInterpreter Support (
src/interpreter/mod.rs):evaluate_database_query()methodDatabaseQueryStatementhandler to use the shared methodExpression::DatabaseQuerycase inevaluate_expression()to support return expressionsexpr_type()helper for debuggingType Checker (
src/typechecker/mod.rs):Expression::DatabaseQueryDatabasetypeTexttypeListcheck_database_query_operands()/database_result_type()helpers keep the statement and expression arms in syncAnalyzer (
src/analyzer/mod.rs):Expression::DatabaseQueryAction Parser (
src/parser/stmt/actions.rs):return query/execute ...as valid return expressionsquery/executeexpression, not thereturnkeywordJavaScript Transpiler (
src/transpiler/javascript.rs):Expression::DatabaseQuery(not supported in JS)I/O Parser (
src/parser/stmt/io.rs):Tests & Documentation
Parser Tests (
tests/database_parser_test.rs):test_return_query_with_parameters()- Validates parsing of parameterized queriestest_return_query_without_parameters()- Validates parsing of simple queriestest_return_execute_with_parameters()- Validates parsing of parameterized executestest_return_execute_without_parameters()- Validates parsing of simple executestest_give_back_query_with_parameters()- Validatesgive backvarianttest_return_variable_named_query_still_works()- Ensures backward compatibilitytest_return_query_concatenation_still_works()- Ensures string concatenation still worksInterpreter Tests (
tests/database_test.rs):test_return_query_with_parameters_from_action()- End-to-end test with parameterized SELECTtest_return_query_without_parameters_from_action()- End-to-end test with simple SELECTtest_return_execute_with_parameters_from_action()- End-to-end test with parameterized INSERTtest_return_execute_without_parameters_from_action()- End-to-end test with plain DELETEE2E Test (
TestPrograms/database_sqlite_test.wfl):Documentation (
Docs/04-advanced-features/databases.md):Out-of-Scope Change: WFLHASH Timing-Test Deflake (Maintainer-Requested)
tests/wflhash_security_test.rs::test_constant_time_measuresfailed this PR's CI run with a timing variation of 164.57% on a shared runner — unrelated to the database change. At the maintainer's request, the test methodology was changed in this PR to be statistical: it now runs 100 measurement rounds of 50 iterations each, logs any single round whose coefficient of variation exceeds 150%, and fails only if the mean variation across all rounds exceeds 150%. The 150% threshold and the 10ms mean-completion-time assertion are unchanged. Measured runtime for the full test file remains ~0.3s.Implementation Details
evaluate_database_query()method handles all database operation logic: evaluating expressions, type validation, parameter conversion, pool retrieval, and query/execute executionreturn query(without a handle) still parses as a variable referencehttps://claude.ai/code/session_01AVZX79bHtePpTnX6mH4jeZ
Summary by CodeRabbit
New Features
return queryandreturn executewith optional parameters in action bodies.Bug Fixes
Tests