csv: validate dialect options - #8402
Conversation
Resolve each dialect once and validate the merged options before constructing readers and writers. Handle Unicode character parsing consistently and enable the corresponding CPython CSV tests. Assisted-by: Tau:gpt-5.6-luna
📝 WalkthroughWalkthroughCSV character parsing is centralized through Unicode-aware helpers. Dialect validation now runs during registration and resolution, while readers and writers consume the resolved dialect for their settings. ChangesCSV dialect behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FormatOptions
participant validate_dialect
participant ReaderWriter
Caller->>FormatOptions: parse dialect arguments
FormatOptions->>validate_dialect: validate resolved dialect
validate_dialect-->>FormatOptions: validated dialect
FormatOptions-->>ReaderWriter: provide dialect settings
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/stdlib/src/csv.rs (2)
299-326: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate dialect attributes during direct
_csv.Dialect(...)construction.
PyDialect::try_from_objectcurrently succeeds for invalid attributes even thoughvalidate_dialectis only called later fromregister_dialectandFormatOptions::result; direct dialect construction / subclass initialization diverges from CPython. Addvalidate_dialect(vm, &dialect)?before returning the constructedPyDialect.🤖 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 299 - 326, Update PyDialect::try_from_object to construct the dialect value first, call validate_dialect(vm, &dialect)? on it, and return it only after validation succeeds. Preserve the existing attribute parsing and strict-default behavior, while ensuring direct _csv.Dialect construction and subclass initialization reject invalid attributes.
632-757: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the dialect attribute validation helpers for
escapecharandquotecharkwargs.The
delimiterkwarg already delegates toparse_delimiter_from_obj.escapechar/quotecharshould use the corresponding helpers instead of inlining checks, so invalid types/lengths use the correctTypeErrortext and acceptPyNonewhere that helper supports it. This closes the remaining keyword-argument path for issue#8284.🤖 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 632 - 757, Update FormatOptions::from_args to parse the escapechar and quotechar kwargs through the existing dialect attribute validation helpers, matching the delimiter path. Remove the inline match-based validation and preserve each helper’s handling of invalid types, character length, and PyNone support.
🤖 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 259-282: Update parse_single_char and parse_first_char so
char_len() is used only for empty or multi-code-point length errors, while
conversion failures use a distinct error path and message. Remove the u8-only
restriction for valid single Unicode code points such as €, and update the
functions’ return type and their callers as needed to preserve the full code
point for CSV dialect attributes.
- Around line 792-807: Update the duplicate-character validation around the
values collection and iteration so dialect_check_chars compares only delimiter,
quotechar, and escapechar. Keep lineterminator validation in the separate
dialect_check_char path, preserving CPython’s acceptance of dialects where it
matches another character setting.
---
Outside diff comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 299-326: Update PyDialect::try_from_object to construct the
dialect value first, call validate_dialect(vm, &dialect)? on it, and return it
only after validation succeeds. Preserve the existing attribute parsing and
strict-default behavior, while ensuring direct _csv.Dialect construction and
subclass initialization reject invalid attributes.
- Around line 632-757: Update FormatOptions::from_args to parse the escapechar
and quotechar kwargs through the existing dialect attribute validation helpers,
matching the delimiter path. Remove the inline match-based validation and
preserve each helper’s handling of invalid types, character length, and PyNone
support.
🪄 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 Plus
Run ID: aa3d9483-5785-4d03-bc09-408322fa6732
⛔ Files ignored due to path filters (1)
Lib/test/test_csv.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/stdlib/src/csv.rs
| fn parse_single_char( | ||
| s: &Py<PyStr>, | ||
| error: impl Fn(usize) -> PyBaseExceptionRef, | ||
| ) -> PyResult<u8> { | ||
| let ch = s | ||
| .as_wtf8() | ||
| .code_points() | ||
| .exactly_one() | ||
| .map_err(|_| error(s.char_len()))?; | ||
| u8::try_from(ch.to_u32()).map_err(|_| error(s.char_len())) | ||
| } | ||
|
|
||
| fn parse_first_char( | ||
| s: &Py<PyStr>, | ||
| error: impl Fn(usize) -> PyBaseExceptionRef, | ||
| ) -> PyResult<u8> { | ||
| let ch = s | ||
| .as_wtf8() | ||
| .code_points() | ||
| .next() | ||
| .ok_or_else(|| error(s.char_len()))?; | ||
| u8::try_from(ch.to_u32()).map_err(|_| error(s.char_len())) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)csv\.rs$|csv' || true
echo "== outline =="
ast-grep outline crates/stdlib/src/csv.rs --view compact || true
echo "== relevant csv lines =="
sed -n '1,380p' crates/stdlib/src/csv.rs | cat -n | sed -n '220,320p'
echo "== search parse_single_char/usages =="
rg -n "parse_single_char|parse_first_char|not a string of length|dialection|Delimiter|QuoteChar|DoubleQuote|error\\(" crates/stdlib/src/csv.rs crates -g '*.rs' || trueRepository: RustPython/RustPython
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== csv.rs relevant section =="
sed -n '220,320p' crates/stdlib/src/csv.rs | cat -n
echo "== parse_single_char/parse_first_char and nearby dialect fields =="
rg -n "pub struct PyDialect|Delimiter|QuoteChar|DoubleQuoteChars|escapechar|lineterminator|skipinitialspace|parse_single_char|parse_first_char|error\\(" crates/stdlib/src/csv.rs -C 3
echo "== focused tests/usages =="
rg -n "sniffer|PyDialect|dialect|delimiter|quotechar|doublequote|escapechar|skipinitialspace|lineterminator|ParserError" crates -g '*.rs' --glob '!crates/vm/src/**' | head -n 200
echo "== CPython docs/source snippets online =="
python3 - <<'PY'
# Behavioral probe: model Rust string semantics around the parsing pattern.
s = "€"
print("single unicode char", len(s), "bytes", len(s.encode('utf-8')))
code_point = ord("€")
print("code point", code_point, "fits in u8", code_point <= 0xff)
s_multi = "€1"
print("multi char chars", len(s_multi))
PYRepository: RustPython/RustPython
Length of output: 50377
🌐 Web query:
Python csv module Dialect delimiter quotechar lineterminator escapechar stored as Py_UCS4 accepts single Unicode characters longer than ASCII
💡 Result:
The Python csv module does not accept Unicode strings longer than a single character for the delimiter, quotechar, or escapechar parameters [1][2][3]. While the internal implementation of the CPython csv module (located in Modules/_csv.c) uses the Py_UCS4 type to store these parameters [4][5][6], this type is used specifically to hold a single Unicode code point [3]. Validation logic within the csv module explicitly ensures that these parameters are one-character strings [7][3]. If a string with more than one character (or an empty string, where disallowed) is provided, the module will raise a TypeError [5][7]. Specifically, the _csv module's internal dialect validation routines reject any attempt to assign a multi-character string to these attributes [5][3]. The lineterminator parameter, by contrast, is stored as a PyObject (a string) and is not restricted to a single character [4][5][6].
Citations:
- 1: https://docs.python.org/3/library/csv.html
- 2: https://github.com/python/cpython/blob/main/Lib/csv.py
- 3: https://gopy.tamnd.com/docs/annotations/modules/module_csv_c_detail
- 4: https://github.com/python/cpython/blob/main/Modules/_csv.c
- 5: https://github.com/python/cpython/blob/3.9/Modules/_csv.c
- 6: https://raw.githubusercontent.com/python/cpython/master/Modules/_csv.c
- 7: https://github.com/python/cpython/blob/3.7/Modules/_csv.c
🌐 Web query:
grep "_csv.c" Py_UCS4 unicode code point validate single character csv delimiter quotechar
💡 Result:
In Python's csv module, the validation of parameters such as the delimiter, quotechar, and escapechar occurs within the C-based implementation found in Modules/_csv.c [1][2][3]. The dialect object stores these characters as Py_UCS4 type values, which are used to handle Unicode code points [1][4]. When these parameters are set, the _csv module verifies that they are either None (where applicable) or a string of exactly one character [1][2][3]. The specific validation process is handled by internal helper functions, typically named dialect_check_char or similar, which are invoked during the initialization of a Dialect object [2][5][3]. These functions enforce that: 1. The provided input is a string [1][6]. 2. The length of the string is exactly 1 (checked via PyUnicode_GetLength) [1][4]. 3. If an invalid type (like an integer) or a string with a length other than 1 is provided, the module raises a TypeError with a message such as "must be a 1-character string" [1][7][8]. Additionally, the code ensures that the delimiter, quotechar, and escapechar do not conflict (e.g., they cannot be the same character), which is verified by functions like dialect_check_chars [2][5][3]. If quoting is enabled, the module also explicitly checks that the quotechar is properly set [2][5][3].
Citations:
- 1: https://github.com/python/cpython/blob/3.10/Modules/_csv.c
- 2: https://raw.githubusercontent.com/python/cpython/master/Modules/_csv.c
- 3: https://github.com/python/cpython/blob/v3.14.2/Modules/_csv.c
- 4: https://hg.python.org/cpython/rev/f2adbb1065eb
- 5: https://github.com/python/cpython/blob/main/Modules/_csv.c
- 6: hugovk/cpython@34bbc87
- 7: https://github.com/python/cpython/blob/3.9/Modules/_csv.c
- 8: https://github.com/python/cpython/blob/3.7/Modules/_csv.c
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import csv
for attr, names in [
("delimiter", ("excel",)),
("quotechar", ("excel",)),
("escapechar", ("excel",)),
("lineterminator", ("excel",)),
]:
dialect = csv.get_dialect(names[0])
print(f"csv dialect {attr}: {getattr(dialect, attr)!r!s!U} type={type(getattr(dialect, attr)).__name__!r}")
for value, param in [
("€", attr),
("€1", attr[:-1] if attr == "lineterminator" else attr),
("", attr),
("💙", attr),
("💙1", attr),
]:
d = csv.registered_dialects[1]
try:
d.update(**{param: value})
print(f" OK {attr}={value!r!s!U} len_chars={len(value)}")
except Exception as e:
print(f" {type(e).__name__}: {e}")
print("Python unicode len examples:")
for s in ["€", "💙", "🩵", "€1", "💙1"]:
print(repr(s), "chars=", len(s), "bytes=", len(s.encode("utf-8")), "ord=", ord(s[0]))
PYRepository: RustPython/RustPython
Length of output: 445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import csv
for attr, names in [
("delimiter", ("excel",)),
("quotechar", ("excel",)),
("escapechar", ("excel",)),
("lineterminator", ("excel",)),
]:
dialect = csv.get_dialect(names[0])
val = getattr(dialect, attr)
print(f"csv dialect {attr}: {val!r} type={type(val).__name__}")
for value, param in [
("€", attr),
("€1", attr if attr != "lineterminator" else attr[:-1]),
("", attr),
("💙", attr),
("💙1", attr if attr != "lineterminator" else attr[:-1]),
]:
try:
dialect.update(**{param: value})
print(f" OK {attr}={value!r} chars={len(value)}")
except Exception as e:
print(f" {type(e).__name__}: {e}")
print("Python unicode len examples:")
for s in ["€", "💙", "🩵", "€1", "💙1"]:
print(repr(s), "chars=", len(s), "bytes=", len(s.encode("utf-8")), "ord=", ord(s[0]))
PYRepository: RustPython/RustPython
Length of output: 1866
🌐 Web query:
Python csv.register_dialect Dialect delimiter quotechar escapechar lineterminator non-ASCII character accepts
💡 Result:
In Python's csv module, the delimiter, quotechar, and escapechar formatting parameters are strictly required to be one-character strings [1]. Regarding non-ASCII characters: 1. Python 3: You can use non-ASCII characters (such as Unicode characters) as a delimiter, quotechar, or escapechar, provided the character is a single Unicode character [2]. 2. Python 2: The csv module typically enforces that these parameters must be 1-character strings. While it may accept certain characters depending on the environment, using non-ASCII characters often leads to compatibility issues or TypeError exceptions [2]. The lineterminator parameter is not restricted to a single character and can be a string of any length [1]. To register a dialect with these parameters, you use the csv.register_dialect function [3][4]: import csv csv.register_dialect('my_dialect', delimiter='ę', quotechar='æ', escapechar='\', lineterminator='\r\n') When defining dialects, keyword arguments (such as delimiter or quotechar) override any parameters inherited from a passed Dialect class [3][4].
Citations:
- 1: https://github.com/python/cpython/blob/main/Lib/csv.py
- 2: https://stackoverflow.com/questions/17325895/can-a-non-ascii-character-be-used-as-a-delimiter-with-python-2s-csv-module
- 3: https://docs.python.org/3/library/csv.html
- 4: https://github.com/python/cpython/blob/main/Doc/library/csv.rst
Separate character-length errors from UTF8/u8 conversion errors.
parse_single_char() and parse_first_char() currently reuse s.char_len() for both empty/multi-character strings and invalid UTF8/u8 conversion failures. For a valid single Unicode code point above u8’s range, this produces the misleading “not a string of length 1” message. Also, RustPython still rejects single non-ASCII code points like € here, unlike CPython csv, which accepts any single Unicode code point for dialect character attributes.
🤖 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 259 - 282, Update parse_single_char
and parse_first_char so char_len() is used only for empty or multi-code-point
length errors, while conversion failures use a distinct error path and message.
Remove the u8-only restriction for valid single Unicode code points such as €,
and update the functions’ return type and their callers as needed to preserve
the full code point for CSV dialect attributes.
There was a problem hiding this comment.
Fixing this requires a broader Unicode support change.
There was a problem hiding this comment.
@hyoinandout, understood—this is outside the scope of the dialect-validation work in #8402. The finding can remain as a follow-up for broader Unicode representation in the CSV parser rather than blocking this PR.
Would you like me to open a GitHub issue to track support for single non-ASCII Unicode dialect characters (for example, €)?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="RustPython/RustPython"
body=$(cat <<'EOF'
## Summary
Add support for single non-ASCII Unicode code points in CSV dialect character attributes, matching CPython behavior.
## Rationale
`csv` dialect options such as `delimiter`, `quotechar`, and `escapechar` accept exactly one Unicode character in CPython. RustPython currently stores these attributes as `u8` in `crates/stdlib/src/csv.rs`, so valid one-character values outside the byte range (for example, `€`) are rejected.
## Affected area
- `crates/stdlib/src/csv.rs`
- CSV dialect parsing, validation, reader, and writer configuration
## Required changes
Replace the byte-only representation and parsing path for dialect characters with a representation that preserves a full Unicode code point, then propagate that support through the CSV reader and writer implementation. Keep existing validation that rejects empty and multi-character values, while reporting conversion/representation failures accurately.
## Acceptance criteria
- `delimiter`, `quotechar`, and `escapechar` accept a single non-ASCII Unicode code point such as `€`.
- Empty and multi-character strings remain rejected according to CPython-compatible behavior.
- Reader and writer behavior correctly handles the supported Unicode dialect characters.
- Relevant RustPython CSV tests cover successful non-ASCII dialect-character usage and invalid lengths.
## Backlinks
- Follow-up from PR `#8402`: https://github.com/RustPython/RustPython/pull/8402
- Review discussion: https://github.com/RustPython/RustPython/pull/8402#discussion_r3658464157
Requested by `@hyoinandout`.
EOF
)
gh issue create \
--repo "$repo" \
--title "csv: support non-ASCII Unicode dialect characters" \
--body "$body" \
--assignee "hyoinandout"Length of output: 1983
@hyoinandout, created and assigned the follow-up: #8408 — csv: support non-ASCII Unicode dialect characters.
🐇✨
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_binascii.py (TODO: 4) dependencies: dependent tests: (95 tests)
[x] lib: cpython/Lib/sqlite3 dependencies:
dependent tests: (2 tests)
[x] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
Legend:
|
|
@hyoinandout Could you resolve conflicts? |
Summary
Resolve each dialect once and validate the merged options before constructing readers and writers. Handle Unicode character parsing consistently and enable the corresponding CPython CSV tests.
Assisted-by: Tau:gpt-5.6-luna
Summary by CodeRabbit