Skip to content

csv: fix csv escape fieldsep#8260

Open
widehyo1 wants to merge 4 commits into
RustPython:mainfrom
widehyo1:fix-csv-escape-fieldsep
Open

csv: fix csv escape fieldsep#8260
widehyo1 wants to merge 4 commits into
RustPython:mainfrom
widehyo1:fix-csv-escape-fieldsep

Conversation

@widehyo1

@widehyo1 widehyo1 commented Jul 12, 2026

Copy link
Copy Markdown
  • This PR follows our AI policy.

Summary

csv.reader did not pass a dialect's escapechar into csv_core, so an
escape character worked only when supplied as an explicit keyword argument.
The first commit passes the dialect value through to csv_core, fixing escaped
delimiters inside quoted fields.

csv_core does not apply its escape setting to unquoted fields. The second
commit adds a small RustPython QUOTE_NONE parser path: an escape character
makes the following byte data, so an escaped delimiter does not split a field.

import csv

class EscapedExcel(csv.excel):
    quoting = csv.QUOTE_NONE
    escapechar = "\\"

list(csv.reader(["abc\\,def\\r\\n"], dialect=EscapedExcel()))
# before: [["abc\\", "def"]]
# after:  [["abc,def"]]  # matches CPython

Summary by CodeRabbit

  • Bug Fixes
    • Improved CSV parsing for dialects and defaults with custom escapechar, ensuring consistent behavior across dialect resolution paths.
    • Enhanced QUOTE_NONE handling with escapechar and skipinitialspace, including correct escaped separators and whitespace processing.
    • Added stricter input validation (record termination, field size limits, and UTF-8 decoding) and ensured trailing escapes are handled gracefully.
  • Tests
    • Added a regression test for QUOTE_NONE with escapechar and skipinitialspace to verify correct field parsing.

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 29564fcc-2fa3-417b-9090-db958ff0bb8c

📥 Commits

Reviewing files that changed from the base of the PR and between 93ee871 and 0ef9af9.

⛔ 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
🚧 Files skipped from review as they are similar to previous changes (2)
  • extra_tests/snippets/stdlib_csv.py
  • crates/stdlib/src/csv.rs

📝 Walkthrough

Walkthrough

CSV dialect escape characters are now passed to reader builders. Quote-none dialects with an escape character use dedicated byte-level parsing for delimiters, spaces, newlines, field limits, trailing escapes, and UTF-8 conversion.

Changes

CSV escapechar support

Layer / File(s) Summary
Dialect escape configuration
crates/stdlib/src/csv.rs
Reader builders now configure escapechar for registered dialects, provided dialect objects, and the default Excel dialect.
Quote-none record parsing
crates/stdlib/src/csv.rs, extra_tests/snippets/stdlib_csv.py
Quote-none iteration routes escaped input through a dedicated parser that handles delimiters, skipped initial spaces, newline validation, field limits, trailing escapes, UTF-8 conversion, and escaped-space coverage.

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

Sequence Diagram(s)

sequenceDiagram
  participant IterNext
  participant read_quote_none_record
  participant PythonRecord
  IterNext->>read_quote_none_record: parse raw input bytes
  read_quote_none_record->>PythonRecord: convert fields to Python strings
  read_quote_none_record-->>IterNext: return record list or CSV error
  IterNext-->>IterNext: increment line number
Loading

Possibly related PRs

Suggested reviewers: youknowone, ShaharNaveh

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and relevant to the CSV escapechar/delimiter parsing fix, even if it is a bit terse.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ 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: 25)

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: 1

🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)

799-839: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate duplicated dialect-resolution logic across the three branches.

Each branch (Str, Obj, default "excel") repeats the identical .delimiter(...).double_quote(...).escape(...) chain plus the quotechar check, differing only in which PyDialect is used. Since this PR is adding a third repeated line (.escape(...)) to all three branches, it's a good point to extract the differing value once.

♻️ Proposed refactor
 fn to_reader(&self) -> csv_core::Reader {
-    let mut builder = csv_core::ReaderBuilder::new();
-    let mut reader = match &self.dialect {
-        DialectItem::Str(name) => {
-            let g = GLOBAL_HASHMAP.lock();
-            if let Some(dialect) = g.get(name) {
-                let mut builder = builder
-                    .delimiter(dialect.delimiter)
-                    .double_quote(dialect.doublequote)
-                    .escape(dialect.escapechar);
-                if let Some(t) = dialect.quotechar {
-                    builder = builder.quote(t);
-                }
-                builder
-            } else {
-                &mut builder
-            }
-        }
-        DialectItem::Obj(obj) => {
-            let mut builder = builder
-                .delimiter(obj.delimiter)
-                .double_quote(obj.doublequote)
-                .escape(obj.escapechar);
-            if let Some(t) = obj.quotechar {
-                builder = builder.quote(t);
-            }
-            builder
-        }
-        _ => {
-            let name = "excel";
-            let g = GLOBAL_HASHMAP.lock();
-            let dialect = g.get(name).unwrap();
-            let mut builder = builder
-                .delimiter(dialect.delimiter)
-                .double_quote(dialect.doublequote)
-                .escape(dialect.escapechar);
-            if let Some(quotechar) = dialect.quotechar {
-                builder = builder.quote(quotechar);
-            }
-            builder
-        }
-    };
+    let resolved = match &self.dialect {
+        DialectItem::Str(name) => GLOBAL_HASHMAP.lock().get(name).copied(),
+        DialectItem::Obj(obj) => Some(*obj),
+        _ => GLOBAL_HASHMAP.lock().get("excel").copied(),
+    };
+
+    let mut builder = csv_core::ReaderBuilder::new();
+    let mut reader = if let Some(dialect) = resolved {
+        let mut b = builder
+            .delimiter(dialect.delimiter)
+            .double_quote(dialect.doublequote)
+            .escape(dialect.escapechar);
+        if let Some(t) = dialect.quotechar {
+            b = b.quote(t);
+        }
+        b
+    } else {
+        &mut builder
+    };

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."

🤖 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 799 - 839, Refactor to_reader so the
DialectItem::Str, DialectItem::Obj, and default "excel" branches first resolve a
single PyDialect reference, then apply delimiter, double_quote, escape, and
optional quotechar configuration once. Preserve the existing named-dialect
lookup and fallback behavior while eliminating the duplicated ReaderBuilder
configuration chains.

Source: Coding guidelines

🤖 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 978-1032: Update read_quote_none_record to honor
dialect.skipinitialspace by removing leading spaces only when starting a field
immediately after a delimiter, while preserving spaces elsewhere and escaped
spaces. Add a regression test covering QUOTE_NONE with escapechar and
skipinitialspace for escaped-space input.

---

Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 799-839: Refactor to_reader so the DialectItem::Str,
DialectItem::Obj, and default "excel" branches first resolve a single PyDialect
reference, then apply delimiter, double_quote, escape, and optional quotechar
configuration once. Preserve the existing named-dialect lookup and fallback
behavior while eliminating the duplicated ReaderBuilder configuration chains.
🪄 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: 60b0182f-6a3a-4c1d-9c6e-ea201467be5f

📥 Commits

Reviewing files that changed from the base of the PR and between 9c064c1 and 8ce78f9.

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

Comment thread crates/stdlib/src/csv.rs
@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 12, 2026
widehyo1 added 4 commits July 12, 2026 21:49
ReaderBuilder only received escapechar when it was passed directly as a
keyword argument. A dialect object's or registered dialect's escapechar was
visible through reader.dialect but not used by csv_core.

Pass escapechar through for named, object, and default dialect paths. This
makes escaped delimiters inside quoted fields follow the configured dialect,
for example '"abc\,def"' reads as 'abc,def'.

Unmark TestQuotedEscapedExcel.test_read_escape_fieldsep.

Assisted-by: Codex
csv_core applies escapechar while reading quoted fields, but its unquoted
field state treats an escaped delimiter as a delimiter. Consequently,
QUOTE_NONE with escapechar split 'abc\,def' into 'abc\' and 'def'.

Add a small QUOTE_NONE record parser for dialects with escapechar. It keeps
the byte following an escape character as field data, so escaped delimiters
do not end a field while ordinary delimiters and record terminators retain
their usual meaning.

Unmark TestEscapedExcel.test_read_escape_fieldsep.

Assisted-by: Codex
The QUOTE_NONE parser returned before the normal reader's whitespace
preprocessing, so it ignored skipinitialspace when escapechar was set.

Track whether parsing has just followed a delimiter and skip only ordinary
spaces in that position. Escaped spaces remain field data.

Add a stdlib_csv snippet regression test for QUOTE_NONE with escapechar and
skipinitialspace.

Assisted-by: Codex:gpt-5.6-terra
to_reader configured ReaderBuilder separately for named, object, and default
dialects even though the configuration was identical.

Resolve the PyDialect once, then apply delimiter, double_quote, escapechar,
and quotechar in one place. Preserve the existing unregistered-name fallback
and required excel dialect lookup behavior.

Assisted-by: Codex:gpt-5.6-terra
@widehyo1 widehyo1 force-pushed the fix-csv-escape-fieldsep branch from 93ee871 to 0ef9af9 Compare July 12, 2026 12:49
@moreal moreal self-requested a review July 12, 2026 13:10

@ShaharNaveh ShaharNaveh 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.

tysm!

@widehyo1

Copy link
Copy Markdown
Author

thank you for the approval and comment 😊!

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.

3 participants