Align SQL comment parsing with CPython #5996
Conversation
WalkthroughThe internal implementation of the 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-lstrip-sql
branch
from
bdfa3f3 to
3403b9b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
stdlib/src/sqlite.rs (1)
3070-3118: Good refactoring with improved bounds safety.The refactored implementation has several positive aspects:
- Explicit bounds checking: Using
sql.get(pos)provides clear, safe bounds checking compared to the previous slicing approach- Clearer control flow: The index-based iteration makes the logic more explicit and easier to follow
- Memory safety: Eliminates potential issues with pointer arithmetic and slicing
Minor performance consideration: The multiple
sql.get(pos)calls might have slight overhead compared to slicing, but the improved safety and clarity are worth the trade-off for this parsing function.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_sqlite3/test_dbapi.pyis excluded by!Lib/**
📒 Files selected for processing (1)
stdlib/src/sqlite.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to Lib/test/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to tests in `Lib/test`
⏰ 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). (9)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
| fn lstrip_sql(sql: &[u8]) -> Option<&[u8]> { | ||
| let mut pos = sql; | ||
| loop { | ||
| match pos.first()? { | ||
| let mut pos = 0; | ||
|
|
||
| // This loop is borrowed from the SQLite source code. | ||
| while let Some(t_char) = sql.get(pos) { | ||
| match t_char { | ||
| b' ' | b'\t' | b'\x0c' | b'\n' | b'\r' => { | ||
| pos = &pos[1..]; | ||
| // Skip whitespace. | ||
| pos += 1; | ||
| } | ||
| b'-' => { | ||
| if *pos.get(1)? == b'-' { | ||
| // line comments | ||
| pos = &pos[2..]; | ||
| while *pos.first()? != b'\n' { | ||
| pos = &pos[1..]; | ||
| // Skip line comments. | ||
| if sql.get(pos + 1) == Some(&b'-') { | ||
| pos += 2; | ||
| while let Some(&ch) = sql.get(pos) { | ||
| if ch == b'\n' { | ||
| break; | ||
| } | ||
| pos += 1; | ||
| } | ||
| let _ = sql.get(pos)?; | ||
| } else { | ||
| return Some(pos); | ||
| return Some(&sql[pos..]); | ||
| } | ||
| } | ||
| b'/' => { | ||
| if *pos.get(1)? == b'*' { | ||
| // c style comments | ||
| pos = &pos[2..]; | ||
| while *pos.first()? != b'*' || *pos.get(1)? != b'/' { | ||
| pos = &pos[1..]; | ||
| // Skip C style comments. | ||
| if sql.get(pos + 1) == Some(&b'*') { | ||
| pos += 2; | ||
| while let Some(&ch) = sql.get(pos) { | ||
| if ch == b'*' && sql.get(pos + 1) == Some(&b'/') { | ||
| break; | ||
| } | ||
| pos += 1; | ||
| } | ||
| let _ = sql.get(pos)?; | ||
| pos += 2; | ||
| } else { | ||
| return Some(pos); | ||
| return Some(&sql[pos..]); | ||
| } | ||
| } | ||
| _ => return Some(pos), | ||
| _ => { | ||
| return Some(&sql[pos..]); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
Review the bounds checking logic in comment handling.
The refactored index-based approach looks good overall, but there are potential logical issues in the comment handling sections:
-
Line 3090:
let _ = sql.get(pos)?;- This line appears to be checking ifposis still valid after processing a line comment, but if it fails, the function returnsNone. This might not be the intended behavior. If we've reached the end of input after a line comment, we should returnNone(indicating everything was skipped), not fail. -
Line 3105: Similar issue with C-style comment handling.
The logic should be:
- If we reach the end of input while processing comments, return
None(everything was skipped) - Only return
Some(&sql[pos..])when we find non-skippable content
Consider this fix:
b'-' => {
// Skip line comments.
if sql.get(pos + 1) == Some(&b'-') {
pos += 2;
while let Some(&ch) = sql.get(pos) {
if ch == b'\n' {
break;
}
pos += 1;
}
- let _ = sql.get(pos)?;
} else {
return Some(&sql[pos..]);
}
}
b'/' => {
// Skip C style comments.
if sql.get(pos + 1) == Some(&b'*') {
pos += 2;
while let Some(&ch) = sql.get(pos) {
if ch == b'*' && sql.get(pos + 1) == Some(&b'/') {
break;
}
pos += 1;
}
- let _ = sql.get(pos)?;
- pos += 2;
+ if sql.get(pos).is_some() {
+ pos += 2;
+ }
} else {
return Some(&sql[pos..]);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn lstrip_sql(sql: &[u8]) -> Option<&[u8]> { | |
| let mut pos = sql; | |
| loop { | |
| match pos.first()? { | |
| let mut pos = 0; | |
| // This loop is borrowed from the SQLite source code. | |
| while let Some(t_char) = sql.get(pos) { | |
| match t_char { | |
| b' ' | b'\t' | b'\x0c' | b'\n' | b'\r' => { | |
| pos = &pos[1..]; | |
| // Skip whitespace. | |
| pos += 1; | |
| } | |
| b'-' => { | |
| if *pos.get(1)? == b'-' { | |
| // line comments | |
| pos = &pos[2..]; | |
| while *pos.first()? != b'\n' { | |
| pos = &pos[1..]; | |
| // Skip line comments. | |
| if sql.get(pos + 1) == Some(&b'-') { | |
| pos += 2; | |
| while let Some(&ch) = sql.get(pos) { | |
| if ch == b'\n' { | |
| break; | |
| } | |
| pos += 1; | |
| } | |
| let _ = sql.get(pos)?; | |
| } else { | |
| return Some(pos); | |
| return Some(&sql[pos..]); | |
| } | |
| } | |
| b'/' => { | |
| if *pos.get(1)? == b'*' { | |
| // c style comments | |
| pos = &pos[2..]; | |
| while *pos.first()? != b'*' || *pos.get(1)? != b'/' { | |
| pos = &pos[1..]; | |
| // Skip C style comments. | |
| if sql.get(pos + 1) == Some(&b'*') { | |
| pos += 2; | |
| while let Some(&ch) = sql.get(pos) { | |
| if ch == b'*' && sql.get(pos + 1) == Some(&b'/') { | |
| break; | |
| } | |
| pos += 1; | |
| } | |
| let _ = sql.get(pos)?; | |
| pos += 2; | |
| } else { | |
| return Some(pos); | |
| return Some(&sql[pos..]); | |
| } | |
| } | |
| _ => return Some(pos), | |
| _ => { | |
| return Some(&sql[pos..]); | |
| } | |
| } | |
| } | |
| None | |
| } | |
| fn lstrip_sql(sql: &[u8]) -> Option<&[u8]> { | |
| let mut pos = 0; | |
| // This loop is borrowed from the SQLite source code. | |
| while let Some(t_char) = sql.get(pos) { | |
| match t_char { | |
| b' ' | b'\t' | b'\x0c' | b'\n' | b'\r' => { | |
| // Skip whitespace. | |
| pos += 1; | |
| } | |
| b'-' => { | |
| // Skip line comments. | |
| if sql.get(pos + 1) == Some(&b'-') { | |
| pos += 2; | |
| while let Some(&ch) = sql.get(pos) { | |
| if ch == b'\n' { | |
| break; | |
| } | |
| pos += 1; | |
| } | |
| } else { | |
| return Some(&sql[pos..]); | |
| } | |
| } | |
| b'/' => { | |
| // Skip C style comments. | |
| if sql.get(pos + 1) == Some(&b'*') { | |
| pos += 2; | |
| while let Some(&ch) = sql.get(pos) { | |
| if ch == b'*' && sql.get(pos + 1) == Some(&b'/') { | |
| break; | |
| } | |
| pos += 1; | |
| } | |
| if sql.get(pos).is_some() { | |
| pos += 2; | |
| } | |
| } else { | |
| return Some(&sql[pos..]); | |
| } | |
| } | |
| _ => { | |
| return Some(&sql[pos..]); | |
| } | |
| } | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
In stdlib/src/sqlite.rs between lines 3070 and 3118, the bounds checking after
processing line and C-style comments uses sql.get(pos)? which returns None if
pos is out of bounds, causing the function to return None prematurely. To fix
this, replace these checks with explicit bounds checks: if pos is at or beyond
the input length after skipping comments, return None to indicate all input was
skipped; otherwise, continue processing. This ensures the function correctly
returns None only when all content is skipped and returns Some with the
remaining slice when non-skippable content is found.
💡 Verification agent
🧩 Analysis chain
Verify alignment with CPython's SQL comment parsing behavior.
Since the PR objective is to "align SQL comment parsing with CPython," please verify that this implementation correctly matches CPython's behavior, particularly:
- Edge cases: How CPython handles malformed comments (e.g.,
/*without closing*/,--at end of input) - Comment nesting: Whether CPython supports nested C-style comments
- Whitespace handling: Exact whitespace characters that CPython skips
Run this script to compare behavior with CPython:
🏁 Script executed:
#!/bin/bash
# Description: Test SQL comment parsing edge cases against CPython behavior
# Expected: Verify that RustPython matches CPython's sqlite3 behavior
# Test cases for edge cases
python3 -c "
import sqlite3
test_cases = [
'/* incomplete comment',
'-- comment at end',
'/* nested /* comment */ */',
' \t\n SELECT 1',
'-- comment\nSELECT 1',
'/* comment */SELECT 1'
]
for case in test_cases:
try:
result = sqlite3.complete_statement(case)
print(f'{case!r}: {result}')
except Exception as e:
print(f'{case!r}: ERROR - {e}')
"Length of output: 633
🏁 Script executed:
#!/bin/bash
# Check how CPython's sqlite3.complete_statement handles various ASCII whitespace characters
python3 - <<'PYCODE'
import sqlite3
cases = [
('space', ' SELECT 1;'),
('tab', '\tSELECT 1;'),
('newline', '\nSELECT 1;'),
('carriage_return', '\rSELECT 1;'),
('form_feed', '\fSELECT 1;'),
('vertical_tab', '\vSELECT 1;'),
]
print("Whitespace Skip Tests (should be True if whitespace is skipped):")
for name, stmt in cases:
result = sqlite3.complete_statement(stmt)
print(f"{name:15s}: {result}")
PYCODELength of output: 208
Include ASCII vertical tab in whitespace skip and document comment nesting
CPython’s sqlite3.complete_statement skips all standard ASCII whitespace—including vertical tab (\v)—and does not support nested C-style comments. To fully align RustPython’s parsing:
- Update the whitespace match in
lstrip_sqlto includeb'\x0b'(vertical tab). - Add a note in the code comment that nested
/*…*/comments aren’t supported, matching CPython behavior. - Add tests covering nested comments (with and without trailing semicolon) to lock in this behavior.
Suggested diff for stdlib/src/sqlite.rs around line 3073:
- while let Some(t_char) = sql.get(pos) {
- match t_char {
- b' ' | b'\t' | b'\x0c' | b'\n' | b'\r' => {
+ while let Some(t_char) = sql.get(pos) {
+ match t_char {
+ // Skip all ASCII whitespace (space, tab, form-feed, newline, carriage-return, vertical-tab)
+ b' ' | b'\t' | b'\x0c' | b'\n' | b'\r' | b'\x0b' => {
pos += 1;
}And above the C-style comment logic, add:
// Note: Nested C-style comments (e.g., /* /* … */ */) are not supported,
// mirroring CPython’s sqlite3.complete_statement behavior.🤖 Prompt for AI Agents
In stdlib/src/sqlite.rs around lines 3070 to 3118, update the whitespace
matching in the lstrip_sql function to include the ASCII vertical tab character
(b'\x0b') so it skips this whitespace as well. Add a comment above the C-style
comment handling code to document that nested C-style comments are not
supported, matching CPython's sqlite3.complete_statement behavior. Additionally,
create tests that cover nested C-style comments with and without trailing
semicolons to ensure this behavior is maintained.
Reference
Summary by CodeRabbit