fix(sqlite): complete Node 26 parity - #6896
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR extends Node.js SQLite compatibility with serialization, deserialization, typed-array binding, limits properties, DQS configuration, iterator invalidation, and validation changes. It also makes loop GC safepoint analysis include control expressions and updates HTTP dispatch feature gates. Changesnode:sqlite parity
Loop GC safepoints
HTTP dispatch gate cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DatabaseSync
participant SQLiteDispatch
participant DatabaseSerialize
participant SQLite
DatabaseSync->>SQLiteDispatch: call serialize(schema)
SQLiteDispatch->>DatabaseSerialize: dispatch database handle and schema
DatabaseSerialize->>SQLite: serialize selected database
SQLite-->>DatabaseSerialize: return serialized image
DatabaseSerialize-->>DatabaseSync: return Uint8Array-like buffer
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-stdlib/src/sqlite/bind.rs (1)
251-260: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t treat bare
TypedArray/DataViewpositional values as named-parameter objects.In
bind_node_sqlite_params, this predicate is checked for only the first argument. BareTypedArrayvalues belong on the direct positional bind path becausebind_node_sqlite_valuehandles them withis_registered_buffer/TypedArrayHeaderchecks separately. Sinceis_named_parameter_objectonly excludes registered buffers and symbols, a loneUint8Array/DataViewis misclassified asnamed_paramsand its numeric keys are bound as named parameters instead of as parameter 1.🐛 Proposed fix
pub(crate) fn is_named_parameter_object(value: f64) -> bool { let js = value_from_f64(value); if !js.is_pointer() { return false; } let raw = raw_addr_from_value(value); raw >= 0x1000 && !is_registered_buffer(raw) + && perry_runtime::typedarray::lookup_typed_array_kind(raw).is_none() && unsafe { perry_runtime::symbol::js_is_symbol(value) == 0 } }🤖 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 `@crates/perry-stdlib/src/sqlite/bind.rs` around lines 251 - 260, Update is_named_parameter_object to exclude all TypedArray and DataView values, not only registered buffers and symbols. Reuse the existing TypedArrayHeader or related type-detection logic used by bind_node_sqlite_value, so bare binary values remain on the direct positional bind path while ordinary objects retain named-parameter handling.
🤖 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.
Outside diff comments:
In `@crates/perry-stdlib/src/sqlite/bind.rs`:
- Around line 251-260: Update is_named_parameter_object to exclude all
TypedArray and DataView values, not only registered buffers and symbols. Reuse
the existing TypedArrayHeader or related type-detection logic used by
bind_node_sqlite_value, so bare binary values remain on the direct positional
bind path while ordinary objects retain named-parameter handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 214fce9f-4d27-4dbd-9cdb-45fbcb1bc121
📒 Files selected for processing (11)
crates/perry-codegen/src/lower_call/native_table/databases.rscrates/perry-runtime/src/array/iter_object.rscrates/perry-stdlib/src/common/dispatch/init.rscrates/perry-stdlib/src/common/dispatch/method_dispatch.rscrates/perry-stdlib/src/sqlite.rscrates/perry-stdlib/src/sqlite/bind.rscrates/perry-stdlib/src/sqlite/connection.rscrates/perry-stdlib/src/sqlite/dispatch.rscrates/perry-stdlib/src/sqlite/node_db.rscrates/perry-stdlib/src/sqlite/node_stmt_session.rscrates/perry-stdlib/src/sqlite/options.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/perry-codegen/src/loop_purity.rs (2)
128-130: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not classify generic
IndexUpdateas allocation-free.Unlike the typed-array write variants, this arm does not prove a fixed-size receiver or an in-bounds index. A plain array expression such as
arr[i]++can grow/reallocate wheniis out of bounds, causingbody_may_allocateto suppress the required moving-GC safepoint. Remove this generic arm or restrict it to a statically proven fixed-size/bounds-safe case.🤖 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 `@crates/perry-codegen/src/loop_purity.rs` around lines 128 - 130, Remove the generic Expr::IndexUpdate arm from the allocation-free classification in loop_purity.rs, or restrict it only to cases with statically proven fixed-size receivers and in-bounds indices. Do not treat expr_alloc_free(object) and expr_alloc_free(index) alone as sufficient; preserve the existing typed-array write handling.
65-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winInclude loop control expressions in safepoint eligibility checks.
emit_gc_loop_safepointonly receivesbody, butlower_for/lower_while/lower_do_whileinvoke it before loweringinit/update/condition. An allocatingforinitializer/update orwhile/do-whilecondition can therefore make the loop-body lowerings skipjs_gc_loop_safepoint; pass the loop control state through or make the callee checkstmt_alloc_free/expr_alloc_freefor that control.🤖 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 `@crates/perry-codegen/src/loop_purity.rs` around lines 65 - 67, Update loop safepoint eligibility around body_may_allocate and emit_gc_loop_safepoint to account for control expressions and statements, not only the loop body: include for initializers and updates plus while/do-while conditions when checking allocation, and pass or inspect that control state before lowering. Preserve safepoints whenever any loop control component may allocate.
🤖 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.
Outside diff comments:
In `@crates/perry-codegen/src/loop_purity.rs`:
- Around line 128-130: Remove the generic Expr::IndexUpdate arm from the
allocation-free classification in loop_purity.rs, or restrict it only to cases
with statically proven fixed-size receivers and in-bounds indices. Do not treat
expr_alloc_free(object) and expr_alloc_free(index) alone as sufficient; preserve
the existing typed-array write handling.
- Around line 65-67: Update loop safepoint eligibility around body_may_allocate
and emit_gc_loop_safepoint to account for control expressions and statements,
not only the loop body: include for initializers and updates plus while/do-while
conditions when checking allocation, and pass or inspect that control state
before lowering. Preserve safepoints whenever any loop control component may
allocate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4987572-5194-4b23-b1e5-43cbfb7c83d0
📒 Files selected for processing (1)
crates/perry-codegen/src/loop_purity.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-codegen/src/stmt/loops.rs`:
- Around line 5142-5143: Move the safepoint emission in the loop-control flow so
it runs after the update and condition expressions execute, immediately before
re-entering the body; retain controls only for eligibility analysis. Apply the
same ordering in crates/perry-codegen/src/stmt/loops.rs at lines 5142-5143,
7048, and 7106: emit after condition evaluation and before branching to the body
or back to the body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ddcb5c7-755d-4dfc-918e-dcc090c9219d
📒 Files selected for processing (2)
crates/perry-codegen/src/loop_purity.rscrates/perry-codegen/src/stmt/loops.rs
# Conflicts: # crates/perry-codegen/src/stmt/loops.rs
2 participants
Summary
Complete
node:sqliteparity with Node.js 26.5.0.Changes
Related issue
Closes #6776
Test plan
cargo build --release -p perrycargo test -p perry-stdlib symbols_are_not_named_parameter_objects --libPATH=/opt/homebrew/Cellar/node/26.5.0/bin:$PATH ./run_parity_tests.sh --suite node-suite --module sqliteChecklist
feat:/fix:/docs:/chore:prefix convention used in the logSummary by CodeRabbit
New Features
Bug Fixes