Skip to content

csv: fix csv escape fieldsep#8260

Merged
youknowone merged 4 commits into
RustPython:mainfrom
widehyo1:fix-csv-escape-fieldsep
Jul 13, 2026
Merged

csv: fix csv escape fieldsep#8260
youknowone merged 4 commits into
RustPython:mainfrom
widehyo1:fix-csv-escape-fieldsep

Conversation

@widehyo1

@widehyo1 widehyo1 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor
  • 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
Contributor Author

thank you for the approval and comment 😊!

@youknowone
youknowone merged commit 67ddc1a into RustPython:main Jul 13, 2026
27 checks passed
@youknowone

Copy link
Copy Markdown
Member

Thank you! and welcome to RustPython project

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