fix(sqlite): align adaptation protocol with CPython #5964
Conversation
WalkthroughThe Changes
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
ever0de
force-pushed
the
fix/sqlite-adapt-none-handling
branch
from
fed2316 to
084eda7
Compare
ever0de
changed the title
None from __adapt__ method![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_sqlite3/test_types.pyis excluded by!Lib/**
📒 Files selected for processing (1)
stdlib/src/sqlite.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (2)
stdlib/src/sqlite.rs (2)
684-726: LGTM: Correctly implements CPython's SQLite adaptation protocol behavior.The changes properly handle the case where
__adapt__and__conform__methods returnNone, which in Python's adaptation protocol means "I don't know how to adapt this object." By continuing the adaptation process whenNoneis returned instead of immediately returning it, this aligns with CPython's behavior.The implementation correctly:
- Preserves error handling for non-TypeError exceptions
- Only adds minimal overhead with the
vm.is_none(&val)check- Maintains the logical flow of the adaptation protocol
- Follows the principle of trying multiple adaptation methods before falling back
712-716: Consistent implementation with__adapt__method handling.The None check for
__conform__return values mirrors the implementation for__adapt__, ensuring consistent behavior across both adaptation methods in the protocol.
| Ok(val) => { | ||
| if !vm.is_none(&val) { | ||
| return Ok(val); | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CPython compatibility with test coverage.
The None check for __adapt__ return values correctly implements the adaptation protocol, but comprehensive test coverage would help ensure this change doesn't introduce regressions.
🏁 Script executed:
#!/bin/bash
# Description: Check for existing tests related to the adaptation protocol and None handling
# Search for test files related to sqlite adaptation
fd -e py -e rs | xargs rg -l "adapt.*None|__adapt__|__conform__" | head -10
# Look for specific test cases that might cover this behavior
rg -A 5 -B 5 "__adapt__.*None|__conform__.*None" --type py --type rs
# Check for existing sqlite adaptation tests
rg -A 10 -B 5 "test.*adapt|adapt.*test" stdlib/src/sqlite.rs || echo "No adaptation tests found in this file"Length of output: 893
Add missing adaptation protocol tests for None handling in sqlite
There are currently no tests covering the case where __adapt__ (or __conform__) returns None in the SQLite module. Please add targeted test cases to prevent regressions:
• Location: stdlib/src/sqlite.rs (around lines 698–702)
• What to add:
- A Rust unit test (e.g. in a
#[cfg(test)] mod tests) that calls the adapter on a type which returnsNone, asserts thatvm.is_none(&val)is true and that the function short-circuits by returningOk(None). - If your test harness supports Python‐level tests, add a snippet in the Python testsuite that registers a dummy adapter returning
None, runs a query binding it, and verifies the value comes back asNone.
Example sketch in Rust:
#[cfg(test)]
mod tests {
use super::*;
use crate::vm::VirtualMachine;
#[test]
fn sqlite_adapt_returns_none() {
let vm = VirtualMachine::new();
// Define or fetch a value whose __adapt__ returns None
let dummy = DummyAdapterReturningNone::new();
let val = dummy.__adapt__(&vm).unwrap();
assert!(vm.is_none(&val), "Expected None from __adapt__");
// Confirm the sqlite path respects that None short-circuit
match sqlite_bind(&vm, val) {
Ok(v) => assert!(vm.is_none(&v)),
Err(e) => panic!("Binding failed: {}", e),
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In stdlib/src/sqlite.rs around lines 698 to 702, add missing tests for the case
when __adapt__ or __conform__ returns None. Create a Rust unit test in the
#[cfg(test)] mod tests that defines a dummy adapter returning None, calls the
adapter, asserts vm.is_none(&val) is true, and verifies the function returns
Ok(None) to ensure proper short-circuiting. Additionally, if possible, add a
Python-level test in the testsuite that registers a dummy adapter returning
None, executes a query binding it, and confirms the returned value is None to
prevent regressions.
Reference
Summary by CodeRabbit