csv: support multi-character lineterminator in writer - #8328
Conversation
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/stdlib/src/csv.rs (1)
885-935: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRedundant sentinel-terminator setup duplicated across both dialect branches.
.terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL))is set inside both theDialectItem::Str(Line 894) andDialectItem::Obj(Line 911) branches, and then unconditionally overwritten again with the identical value at Line 934. The two branch-local calls are dead code since Line 934 always re-applies the same sentinel regardless of dialect source — this is exactly the value-differs/logic-shared duplication the repo guideline calls out.As per coding guidelines, "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
♻️ Proposed fix removing the redundant branch-local terminator calls
if let Some(dialect) = g.get(name) { let mut builder = builder .delimiter(dialect.delimiter) - .double_quote(dialect.doublequote) - .terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)); + .double_quote(dialect.doublequote);DialectItem::Obj(obj) => { let mut builder = builder .delimiter(obj.delimiter) - .double_quote(obj.doublequote) - .terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)); + .double_quote(obj.doublequote);🤖 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/stdlib/src/csv.rs` around lines 885 - 935, Remove the redundant .terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)) calls from both DialectItem::Str and DialectItem::Obj builder branches in to_writer. Keep the existing unconditional terminator assignment after the dialect-specific and override handling so the sentinel is applied once for every dialect source.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)
1488-1496: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCorrectness-critical sentinel invariant relies only on
debug_assert_eq!, which is compiled out in release builds.If the assumption that
buffer[buffer_offset - 1] == CSV_CORE_TERMINATOR_SENTINELever breaks (e.g. future change toto_writer/csv-core usage), release builds would silently drop the wrong byte and splice the real terminator into corrupted output instead of failing loudly. Given this is currently guaranteed by how csv-core'sterminator()is used, the risk is low today, but the guard should hold in release builds too.🛡️ Proposed fix to keep the check in release builds
- debug_assert_eq!(buffer[buffer_offset - 1], CSV_CORE_TERMINATOR_SENTINEL); + assert_eq!(buffer[buffer_offset - 1], CSV_CORE_TERMINATOR_SENTINEL);🤖 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/stdlib/src/csv.rs` around lines 1488 - 1496, Replace the debug-only assertion in the output construction path with a release-enforced check that validates buffer[buffer_offset - 1] is CSV_CORE_TERMINATOR_SENTINEL before removing it; fail loudly if the invariant is violated, while preserving the existing terminator replacement behavior for valid output.
🤖 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/stdlib/src/csv.rs`:
- Around line 623-632: The empty-lineterminator validation is duplicated with
inconsistent exception types and messages. Update FormatOptions::from_args at
crates/stdlib/src/csv.rs:623-632 and prase_lineterminator_from_obj at
crates/stdlib/src/csv.rs:223-236 to share one validation path, using the same
exception type and wording that describe the actual non-empty constraint;
preserve acceptance of non-empty strings, including multi-character values.
---
Outside diff comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 885-935: Remove the redundant
.terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)) calls from both
DialectItem::Str and DialectItem::Obj builder branches in to_writer. Keep the
existing unconditional terminator assignment after the dialect-specific and
override handling so the sentinel is applied once for every dialect source.
---
Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 1488-1496: Replace the debug-only assertion in the output
construction path with a release-enforced check that validates
buffer[buffer_offset - 1] is CSV_CORE_TERMINATOR_SENTINEL before removing it;
fail loudly if the invariant is violated, while preserving the existing
terminator replacement behavior for valid output.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 79e824cb-2025-42fe-a920-917fb6699cf3
⛔ Files ignored due to path filters (1)
Lib/test/test_csv.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/src/csv.rsextra_tests/snippets/stdlib_csv.py
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
Legend:
|
c6565d0 to
5659610
Compare
|
Since #8304 was merged, the read_quote_none_record function has been removed, so this branch now has a merge conflict. That wasn't intentional—could you take a look? |
9e3fad7 to
f0f1e8c
Compare
|
@widehyo1 sure. I will! thank you for catching this |
There was a problem hiding this comment.
Hi @jinmay
Lineterminator accepts non-ASCII strings, but this byte-wise check can split UTF-8 fields.
Please reject them or handle them character-wise.
There was a problem hiding this comment.
Thanks @doma17, good catch.
I deferred non-ASCII terminators on purpose since doing it properly is a much bigger change — delimiter/quotechar/escapechar are still single bytes, so it only really makes sense together with full Unicode dialect support (#8310). But I only wrote that down in the issue and PR description and never enforced it in code, so the parser accepted non-ASCII and then handled it byte-wise. Didn't think to guard it there.
I'll reject non-ASCII terminators for now (csv.Error) and leave full support to the #8310 follow-up.
There was a problem hiding this comment.
@jinmay A golden rule when you know caveat is leaving a comment here is a caveat. please add a comment about future TODO about this.
There was a problem hiding this comment.
Sorry, I should have put that in the code in the first place rather than only in the PR description. Added a TODO at this line and on the validation helper, both pointing at #8310 for the code-point-wise handling.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Looks good in general, please also fix build failure
Replace the csv-core reader and per-item quote scanner with one Rust implementation of CPython's nine-state reader parser. Keep parser state across iterator items and distinguish virtual item boundaries from true iterator exhaustion. Centralize field completion so quote provenance, empty-field None conversion, float conversion, strict parsing, field limits, blank rows, and escaped or quoted newlines share one path. Preserve the reentrant-iterator generation guard and the post-RustPython#8328 writer behavior. Remove the expected-failure markers from the eleven reader tests that now pass. Assisted-by: Codex:gpt-5.6-sol
f0f1e8c to
7c5a962
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
extra_tests/snippets/stdlib_csv.py (1)
232-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winWiden QUOTE_NONE escaping coverage to all terminator bytes.
This only verifies that
!(one byte of the three-byte terminator!@#) gets escaped. A field containing all three bytes together (e.g."a!@#b") would better validate that the per-byte escaping loop correctly handles a multi-character terminator end-to-end.♻️ Suggested broader case
none = io.StringIO() csv.writer( none, lineterminator="!@#", quoting=csv.QUOTE_NONE, escapechar="\\" ).writerow(["a!b", "x"]) assert none.getvalue() == "a\\!b,x!@#", none.getvalue() + + none_all = io.StringIO() + csv.writer( + none_all, lineterminator="!@#", quoting=csv.QUOTE_NONE, escapechar="\\" + ).writerow(["a!@#b", "x"]) + assert none_all.getvalue() == "a\\!\\@\\`#b`,x!@#", none_all.getvalue()🤖 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 `@extra_tests/snippets/stdlib_csv.py` around lines 232 - 237, Expand the QUOTE_NONE test around the csv.writer call to use a field containing all terminator bytes together, such as “a!@#b”, and update the expected escaped output accordingly so the assertion verifies each byte in the multi-character lineterminator is escaped.
🤖 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 `@extra_tests/snippets/stdlib_csv.py`:
- Around line 232-237: Expand the QUOTE_NONE test around the csv.writer call to
use a field containing all terminator bytes together, such as “a!@#b”, and
update the expected escaped output accordingly so the assertion verifies each
byte in the multi-character lineterminator is escaped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b770ed4d-605d-4c30-915e-7466e37f6a5c
⛔ Files ignored due to path filters (1)
Lib/test/test_csv.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/stdlib/src/csv.rsextra_tests/snippets/stdlib_csv.py
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/csv.rs
Store the dialect line terminator as an owned String instead of the single-byte csv_core::Terminator, dropping PyDialect's Copy derive. The manual writer paths emit the full terminator; the csv-core-backed QUOTE_ALL/QUOTE_NONNUMERIC paths emit a sentinel byte (preserving csv-core's quote/empty-record bookkeeping) and append the real terminator. field_needs_quotes/escape now quote a field containing any terminator byte. The reader ignores lineterminator and always uses CRLF, matching CPython and avoiding mid-UTF-8 record splits.
- Remove the redundant per-branch sentinel terminator setup in to_writer; the unconditional terminator call after the match is the single source. - Reword the empty-lineterminator errors to "must not be empty" (the constraint is non-empty, not single-character) on both entry points. - Promote the sentinel invariant check in writerow from debug_assert_eq! to assert_eq! so it also guards release builds. - Add a snippet case for a field containing a line-break byte to ensure the csv-core path drops only the trailing terminator.
The writer decides what to quote and escape by comparing raw bytes, so a non-ASCII terminator quoted a field that merely shared a UTF-8 lead byte, and QUOTE_NONE escaped individual bytes of the terminator and then failed to decode the record back to a string. Reject non-ASCII terminators, including lone surrogates, as csv.Error when the dialect is parsed, and leave code-point-wise handling to a follow-up (RustPython#8310). The non-string error message now matches CPython as well.
7c5a962 to
61e9782
Compare
4 participants
Summary
csv.writerrejected anylineterminatorthat was not a single character — including the default'\r\n'when passed explicitly — because the dialect stored it as a single-bytecsv_core::Terminator. CPython accepts an arbitrary-length terminator.The dialect now stores the line terminator as an owned
String(droppingPyDialect'sCopyderive). The hand-written writer paths (QUOTE_MINIMAL/QUOTE_NONE/QUOTE_STRINGS/QUOTE_NOTNULL) emit the full terminator directly. Thecsv_core-backedQUOTE_ALL/QUOTE_NONNUMERICpaths still rely oncsv_corefor its quoting bookkeeping, so they emit a one-byte sentinel terminator (keepingcsv_core's closing-quote / empty-record handling intact) and then replace that sentinel with the real terminator.field_needs_quotes/field_needs_escapenow quote/escape a field containing any byte of the terminator, matching CPython.The reader now ignores
lineterminatorand always splits on\r/\n/\r\n, matching CPython (csv.readernever honored the dialect terminator) and avoiding a mid-UTF-8 record split that a multi-byte terminator would otherwise cause.Scoped to ASCII terminators; non-ASCII/surrogate terminators and empty-terminator acceptance are left for a follow-up.
Test plan
test_write_lineterminator(@unittest.expectedFailurebefore).cargo run --release -- -m test test_csv: 128 run, 7 skipped, SUCCESS.extra_tests/snippets/stdlib_csv.py: passes (addedtest_multichar_lineterminatorcovering multi-char terminators, terminator-byte quoting,QUOTE_ALL/QUOTE_NONNUMERIC,QUOTE_NONEescaping,register_dialectround-trip, and reader behavior; verified on both RustPython and CPython).cargo fmt --check/cargo clippy: clean (no new warnings).Assisted-by: Claude Code:claude-opus-4-8
Summary by CodeRabbit
lineterminatorhandling to support arbitrary non-empty multi-character terminators.QUOTE_NONEwithescapechar.\r\nwhile keeping other terminator text inside fields (CPython-aligned).lineterminatorvalues and reject non-ASCII inputs.lineterminatorbehavior across quoting modes and reader/writer round-tripping.lineterminatorvalues.