Skip to content

csv: quote embedded CR/LF under QUOTE_MINIMAL#8315

Merged
youknowone merged 2 commits into
RustPython:mainfrom
jinmay:fix-csv-quote-crlf-lineterminator
Jul 19, 2026
Merged

csv: quote embedded CR/LF under QUOTE_MINIMAL#8315
youknowone merged 2 commits into
RustPython:mainfrom
jinmay:fix-csv-quote-crlf-lineterminator

Conversation

@jinmay

@jinmay jinmay commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Under QUOTE_MINIMAL, a field containing \r or \n must be quoted regardless of the lineterminator — CPython quotes them unconditionally. RustPython serialized QUOTE_MINIMAL rows with csv_core, which decides quoting from the bytes it registered for the terminator, so with a terminator other than CR/LF (e.g. '!' or '\0') embedded \r/\n were left unquoted.

QUOTE_MINIMAL now uses a hand-written path (like QUOTE_NONE / QUOTE_STRINGS) that decides quoting via the existing field_needs_quotes, which always treats \r/\n as needing quotes. QUOTE_ALL and QUOTE_NONNUMERIC still use csv_core.

This also fixes an empty row (writerow([])) emitting "" instead of only the terminator.

import csv, io
sio = io.StringIO()
csv.writer(sio, lineterminator="!").writerow(["\r", "\n"])
# before: '\r,\n!'
# after:  '"\r","\n"!'   (matches CPython)

Multi-character lineterminator (the !@# case in test_write_lineterminator) is still unsupported and left for a follow-up, so that test stays marked.

Test plan

  • Unmarks test_write_iterable and test_write_empty_fields (both @unittest.expectedFailure before). cargo run --release -- -m test test_csv: 128 run, 7 skipped, SUCCESS.
  • extra_tests/snippets/stdlib_csv.py: passes (added writer cases for custom terminators and empty fields).
  • cargo fmt --check / cargo clippy: clean (no new warnings).
  • Compared against CPython 3.14.6 on the same machine; the QUOTE_MINIMAL writer outputs match.

Assisted-by: Claude Code:claude-opus-4-8

Summary by CodeRabbit

  • Bug Fixes

    • Fixed CSV writing with minimal quoting so fields are quoted only when required by delimiters, quotes, line breaks, or line terminators.
    • Correctly handles empty fields and single empty values when producing CSV output.
    • Ensured custom line terminators are serialized correctly.
  • Tests

    • Added coverage for minimal-quoting behavior, line terminators, and empty fields.

jinmay added 2 commits July 18, 2026 18:49
QUOTE_MINIMAL rows were serialized by csv-core, which only quotes bytes
registered for the configured terminator, so fields containing '\r' or
'\n' were left unquoted when the lineterminator was not CR/LF (e.g. '!'
or '\0'). Route QUOTE_MINIMAL through a hand-written path that uses
field_needs_quotes, which always treats '\r'/'\n' as needing quotes.
A single empty field is quoted and an empty row emits only the
terminator, matching CPython. QUOTE_ALL / QUOTE_NONNUMERIC still use
csv-core.

Assisted-by: Claude Code:claude-opus-4-8
test_write_iterable and test_write_empty_fields pass with the
QUOTE_MINIMAL writer fix (empty rows no longer emit an empty field).

Assisted-by: Claude Code:claude-opus-4-8
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The CSV writer now implements QuoteStyle::Minimal for single-row writes, applying per-field quoting rules and configured line terminators. Tests cover embedded line breaks, custom terminators, empty fields, None, and empty rows.

Changes

CSV minimal quoting

Layer / File(s) Summary
Minimal quoting write path
crates/stdlib/src/csv.rs, extra_tests/snippets/stdlib_csv.py
Writer applies per-field minimal quoting, dispatches QuoteStyle::Minimal to the new implementation, and tests line terminator, embedded newline, empty-field, None, and empty-row serialization.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: z-ca-2026

Suggested reviewers: widehyo1, shaharnaveh, youknowone

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The implementation and tests address #8302 by quoting CR/LF under QUOTE_MINIMAL even with custom line terminators.
Out of Scope Changes check ✅ Passed The changed code and tests stay focused on QUOTE_MINIMAL writer behavior and related edge cases.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: QUOTE_MINIMAL now quotes embedded CR/LF.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/csv.py
[x] test: cpython/Lib/test/test_csv.py (TODO: 20)

dependencies:

  • csv

dependent tests: (4 tests)

  • csv: test_csv test_genericalias
    • importlib.metadata: test_importlib test_zoneinfo

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
extra_tests/snippets/stdlib_csv.py (1)

155-155: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Specify newline="" in io.StringIO to prevent platform-dependent newline translation.

io.StringIO() defaults to translating \n to os.linesep when writing. On Windows, this translates the \r\n written by csv.writer into \r\r\n, which will cause your strict getvalue() assertions to fail. The Python CSV module documentation specifically recommends passing newline="" to avoid this gotcha. The shared root cause applies to all test instantiations:

  • extra_tests/snippets/stdlib_csv.py#L155-L155: add newline="" to io.StringIO instantiation.
  • extra_tests/snippets/stdlib_csv.py#L162-L162: add newline="" to io.StringIO instantiation.
  • extra_tests/snippets/stdlib_csv.py#L166-L166: add newline="" to io.StringIO instantiation.
  • extra_tests/snippets/stdlib_csv.py#L171-L171: add newline="" to io.StringIO instantiation.
  • extra_tests/snippets/stdlib_csv.py#L176-L176: add newline="" to io.StringIO instantiation.
  • extra_tests/snippets/stdlib_csv.py#L185-L185: add newline="" to io.StringIO instantiation.
🤖 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` at line 155, Update every io.StringIO
instantiation in extra_tests/snippets/stdlib_csv.py at lines 155-155, 162-162,
166-166, 171-171, 176-176, and 185-185 to pass newline="".
🤖 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 1383-1430: Consolidate the duplicated handwritten quoting logic by
replacing writerow_minimal, writerow_quote_none, and writerow_quoted_strings
with one writerow_custom method. Centralize row iteration, field conversion,
quoting-style selection for Strings, Notnull, Minimal, and None,
single-empty-field validation, output construction, and line-terminator writing
in writerow_custom, then update callers to use it and remove the obsolete
helpers.
- Around line 1433-1439: Update the writerow method’s quoting-style dispatch so
all handwritten quoting styles route through the unified writerow_custom helper
introduced by the refactor, while preserving the existing behavior for other
styles and the QuoteStyle matching structure.

---

Nitpick comments:
In `@extra_tests/snippets/stdlib_csv.py`:
- Line 155: Update every io.StringIO instantiation in
extra_tests/snippets/stdlib_csv.py at lines 155-155, 162-162, 166-166, 171-171,
176-176, and 185-185 to pass newline="".
🪄 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: 8ac39457-ccab-4cd2-ae56-f3b77bf26e4b

📥 Commits

Reviewing files that changed from the base of the PR and between 0bc109d and 071c799.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_csv.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/stdlib/src/csv.rs
  • extra_tests/snippets/stdlib_csv.py

Comment thread crates/stdlib/src/csv.rs
Comment on lines +1383 to +1430
fn writerow_minimal(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
let _state = self.state.lock();

let row: ArgIterable = ArgIterable::try_from_object(vm, row.clone()).map_err(|_e| {
new_csv_error(
vm,
format!("'{}' object is not iterable", row.class().name()),
)
})?;

let fields = row.iter(vm)?.collect::<PyResult<Vec<_>>>()?;
let single_field = fields.len() == 1;
let mut output = Vec::new();

for (index, field) in fields.into_iter().enumerate() {
if index > 0 {
output.push(self.dialect.delimiter);
}

let stringified;
let data: &[u8] = match_class!(match field {
ref s @ PyStr => s.as_bytes(),
crate::builtins::PyNone => b"",
ref obj => {
stringified = obj.str(vm)?;
stringified.as_bytes()
}
});

// CPython quotes a QUOTE_MINIMAL field if it contains the
// delimiter, the quote character, '\r', '\n', or the line
// terminator, regardless of which line terminator is
// configured. A row with a single empty field is also quoted
// so that it is not read back as an empty line.
if field_needs_quotes(data, self.dialect) || (single_field && data.is_empty()) {
write_quoted_field(&mut output, data, self.dialect, vm)?;
} else {
output.extend_from_slice(data);
}
}

write_lineterminator(&mut output, self.dialect.lineterminator);

let s = core::str::from_utf8(&output)
.map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?;

self.write.call((s,), vm)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Consolidate duplicated writerow helpers into a single method.

As per coding guidelines, when branches share common logic, the logic should be extracted to avoid duplication. The new writerow_minimal is largely a copy of writerow_quote_none and writerow_quoted_strings (duplicating row iteration, string conversion, and line-terminator logic).

Consider extracting a single writerow_custom method that handles all handwritten quoting styles and replacing all three redundant helpers.

♻️ Proposed unified implementation

Add this unified method, delete writerow_minimal, and safely remove the now-dead writerow_quote_none and writerow_quoted_strings methods:

        fn writerow_custom(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
            let _state = self.state.lock();

            let row: ArgIterable = ArgIterable::try_from_object(vm, row.clone()).map_err(|_e| {
                new_csv_error(
                    vm,
                    format!("'{}' object is not iterable", row.class().name()),
                )
            })?;

            let fields = row.iter(vm)?.collect::<PyResult<Vec<_>>>()?;
            let single_field = fields.len() == 1;
            let mut output = Vec::new();

            for (index, field) in fields.into_iter().enumerate() {
                if index > 0 {
                    output.push(self.dialect.delimiter);
                }

                let stringified;
                let (data, is_str, is_none): (&[u8], bool, bool) = match_class!(match field {
                    ref s @ PyStr => (s.as_bytes(), true, false),
                    crate::builtins::PyNone => (b"", false, true),
                    ref obj => {
                        stringified = obj.str(vm)?;
                        (stringified.as_bytes(), false, false)
                    }
                });

                let (should_quote, error_if_single_empty, use_unquoted) = match self.dialect.quoting {
                    QuoteStyle::Strings => (is_str || field_needs_quotes(data, self.dialect), true, false),
                    QuoteStyle::Notnull => (!is_none, true, false),
                    QuoteStyle::Minimal => (field_needs_quotes(data, self.dialect) || (single_field && data.is_empty()), false, false),
                    QuoteStyle::None => (false, true, true),
                    _ => unreachable!(),
                };

                if should_quote {
                    write_quoted_field(&mut output, data, self.dialect, vm)?;
                } else {
                    if error_if_single_empty && single_field && data.is_empty() {
                        return Err(new_csv_error(
                            vm,
                            "single empty field record must be quoted",
                        ));
                    }
                    if use_unquoted {
                        write_unquoted_field(&mut output, data, self.dialect, vm)?;
                    } else {
                        output.extend_from_slice(data);
                    }
                }
            }

            write_lineterminator(&mut output, self.dialect.lineterminator);

            let s = core::str::from_utf8(&output)
                .map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?;

            self.write.call((s,), vm)
        }
🤖 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 1383 - 1430, Consolidate the
duplicated handwritten quoting logic by replacing writerow_minimal,
writerow_quote_none, and writerow_quoted_strings with one writerow_custom
method. Centralize row iteration, field conversion, quoting-style selection for
Strings, Notnull, Minimal, and None, single-empty-field validation, output
construction, and line-terminator writing in writerow_custom, then update
callers to use it and remove the obsolete helpers.

Source: Coding guidelines

Comment thread crates/stdlib/src/csv.rs
Comment on lines 1433 to +1439
fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
match self.dialect.quoting {
QuoteStyle::None => return self.writerow_quote_none(row, vm),
QuoteStyle::Strings | QuoteStyle::Notnull => {
return self.writerow_quoted_strings(row, vm);
}
QuoteStyle::Minimal => return self.writerow_minimal(row, vm),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Update writerow dispatch to use the unified helper.

If you adopt the writerow_custom refactor, update this match arm to dispatch all handwritten styles to the unified method.

♻️ Proposed fix
-        #[pymethod]
-        fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
-            match self.dialect.quoting {
-                QuoteStyle::None => return self.writerow_quote_none(row, vm),
-                QuoteStyle::Strings | QuoteStyle::Notnull => {
-                    return self.writerow_quoted_strings(row, vm);
-                }
-                QuoteStyle::Minimal => return self.writerow_minimal(row, vm),
-                _ => {}
-            }
+        #[pymethod]
+        fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
+            if matches!(
+                self.dialect.quoting,
+                QuoteStyle::None | QuoteStyle::Strings | QuoteStyle::Notnull | QuoteStyle::Minimal
+            ) {
+                return self.writerow_custom(row, vm);
+            }
📝 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.

Suggested change
fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
match self.dialect.quoting {
QuoteStyle::None => return self.writerow_quote_none(row, vm),
QuoteStyle::Strings | QuoteStyle::Notnull => {
return self.writerow_quoted_strings(row, vm);
}
QuoteStyle::Minimal => return self.writerow_minimal(row, vm),
fn writerow(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
if matches!(
self.dialect.quoting,
QuoteStyle::None | QuoteStyle::Strings | QuoteStyle::Notnull | QuoteStyle::Minimal
) {
return self.writerow_custom(row, vm);
}
🤖 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 1433 - 1439, Update the writerow
method’s quoting-style dispatch so all handwritten quoting styles route through
the unified writerow_custom helper introduced by the refactor, while preserving
the existing behavior for other styles and the QuoteStyle matching structure.

@jinmay
jinmay marked this pull request as draft July 18, 2026 12:45
@jinmay

jinmay commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Re: CodeRabbit's suggestion to consolidate the hand-written writer paths (writerow_quote_none / writerow_quoted_strings / writerow_minimal) into one method.

The duplication is real. I'd like to keep this PR scoped to the QUOTE_MINIMAL fix, though: QUOTE_ALL and QUOTE_NONNUMERIC still go through csv_core, so a proper unification really comes down to whether the writer should keep csv_core at all — an architecture change that reaches beyond this bug. I'd rather agree on that direction first and do the consolidation as a follow-up once there's consensus.

@jinmay
jinmay marked this pull request as ready for review July 18, 2026 13:14
@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 18, 2026

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thank you!

@youknowone
youknowone merged commit 8754004 into RustPython:main Jul 19, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Development

Successfully merging this pull request may close these issues.

csv.writer: embedded \r/\n not quoted with a custom lineterminator (QUOTE_MINIMAL)

2 participants