Skip to content

csv: validate dialect options - #8402

Open
hyoinandout wants to merge 7 commits into
RustPython:mainfrom
hyoinandout:fix/csv-dialect-validation
Open

csv: validate dialect options#8402
hyoinandout wants to merge 7 commits into
RustPython:mainfrom
hyoinandout:fix/csv-dialect-validation

Conversation

@hyoinandout

@hyoinandout hyoinandout commented Jul 27, 2026

Copy link
Copy Markdown

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

  • Bug Fixes
    • Improved CSV dialect validation for delimiters, quote characters, escape characters, spaces, and line terminators.
    • Added clearer handling and error reporting for invalid, conflicting, duplicate, or incorrectly sized character settings.
    • Improved support for CRLF and multi-character line terminators.
    • Fixed handling of non-ASCII characters in CSV configuration and content.
    • Ensured CSV readers and writers consistently apply validated dialect settings, including correct quoting and escaping.

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
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

CSV dialect parsing now uses shared WTF-8 character helpers. Dialect validation runs during construction, registration, and option resolution. Readers and writers reuse resolved dialects, and escaping recognizes complete UTF-8 line-terminator characters.

Changes

CSV dialect behavior

Layer / File(s) Summary
Character parsing and argument handling
crates/stdlib/src/csv.rs
Delimiter, quotechar, escapechar, and lineterminator values use shared WTF-8 parsing. Explicit None and invalid keyword types are handled correctly.
Dialect resolution and validation
crates/stdlib/src/csv.rs
Dialect construction and resolution reject conflicting line breaks, spaces, duplicate characters, and characters shared with the line terminator.
Reader, writer, and escaping integration
crates/stdlib/src/csv.rs
Readers and writers reuse resolved dialects. csv-core receives the resolved settings, and escaping detects complete UTF-8 terminator characters.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: validating CSV dialect options.
Linked Issues check ✅ Passed The changes reject invalid escapechar values and validate resolved dialect options for readers and writers, addressing issue #8284.
Out of Scope Changes check ✅ Passed The reviewed changes remain focused on CSV dialect parsing, validation, resolution, and reader/writer integration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ 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.

@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

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 win

Validate dialect attributes during direct _csv.Dialect(...) construction.

PyDialect::try_from_object currently succeeds for invalid attributes even though validate_dialect is only called later from register_dialect and FormatOptions::result; direct dialect construction / subclass initialization diverges from CPython. Add validate_dialect(vm, &dialect)? before returning the constructed PyDialect.

🤖 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 win

Use the dialect attribute validation helpers for escapechar and quotechar kwargs.

The delimiter kwarg already delegates to parse_delimiter_from_obj. escapechar/quotechar should use the corresponding helpers instead of inlining checks, so invalid types/lengths use the correct TypeError text and accept PyNone where 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

📥 Commits

Reviewing files that changed from the base of the PR and between 59e903d and d49a37f.

⛔ 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 Outdated
Comment thread crates/stdlib/src/csv.rs Outdated
@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

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

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

dependencies:

  • csv (native: _csv)
    • io (native: _io, _thread, errno, msvcrt, sys)
    • re, types

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

@youknowone youknowone added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 28, 2026
@moreal

moreal commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

@hyoinandout Could you resolve conflicts?

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
crates/stdlib/src/csv.rs (3)

782-812: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate every character in lineterminator.

exactly_one() discards multi-character terminators before collision checking. A dialect with delimiter='|' and lineterminator="\n|" passes validation even though the delimiter occurs in the terminator. Check each ASCII byte of lineterminator against delimiter, quotechar, and escapechar.

🤖 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 782 - 812, Update the validation
around `line_terminator` and the `values` collision loop to inspect every ASCII
byte in `dialect.lineterminator`, rather than reducing it with `exactly_one()`.
Reject the dialect when any terminator byte matches `delimiter`, `quotechar`, or
`escapechar`, while preserving the existing duplicate-character validation and
error behavior.

489-497: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Configure csv-core from the resolved dialect.

Writer stores the resolved dialect, but FormatOptions::to_writer() configures csv-core from raw options. An inherited escapechar from a dialect or object option can stay in Writer.dialect without reaching .escape(), so QUOTE_ALL and QUOTE_NONNUMERIC output may use csv-core quoting instead of stored quoting. Build the csv-core writer from options.result(vm)? while still preserving explicit escapechar=None behavior.

Also applies to: to_writer() at lines 891-946

🤖 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 489 - 497, Update the Writer
construction and FormatOptions::to_writer() to configure csv-core from the
resolved dialect returned by options.result(vm), rather than raw options.
Preserve explicit escapechar=None semantics while ensuring inherited
dialect/object escape and quoting settings reach csv-core, including QUOTE_ALL
and QUOTE_NONNUMERIC behavior.

1048-1050: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Preserve WTF-8 when decoding and emitting CSV fields.

PyStr can contain WTF-8 for lone surrogates. These paths pass raw PyStr bytes through the parser or writer, then reject them with from_utf8. Valid Python string values can therefore fail with UnicodeDecodeError.

  • crates/stdlib/src/csv.rs#L1048-L1050: construct the parsed field from WTF-8 instead of from_utf8.
  • crates/stdlib/src/csv.rs#L1451-L1453: construct quoted-string output from WTF-8.
  • crates/stdlib/src/csv.rs#L1497-L1500: construct QUOTE_NONE output from WTF-8.
  • crates/stdlib/src/csv.rs#L1546-L1549: construct QUOTE_MINIMAL output from WTF-8.
  • crates/stdlib/src/csv.rs#L1631-L1634: construct csv-core output from WTF-8.
🤖 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 1048 - 1050, Replace UTF-8-only
decoding with WTF-8 construction throughout the CSV parser and writer: update
the parsed-field conversion at crates/stdlib/src/csv.rs:1048-1050 and the
quoted-string, QUOTE_NONE, QUOTE_MINIMAL, and csv-core output paths at
crates/stdlib/src/csv.rs:1451-1453, 1497-1500, 1546-1549, and 1631-1634.
Preserve lone-surrogate PyStr values instead of propagating UnicodeDecodeError,
using the existing WTF-8 string construction APIs.
🤖 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.

Outside diff comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 782-812: Update the validation around `line_terminator` and the
`values` collision loop to inspect every ASCII byte in `dialect.lineterminator`,
rather than reducing it with `exactly_one()`. Reject the dialect when any
terminator byte matches `delimiter`, `quotechar`, or `escapechar`, while
preserving the existing duplicate-character validation and error behavior.
- Around line 489-497: Update the Writer construction and
FormatOptions::to_writer() to configure csv-core from the resolved dialect
returned by options.result(vm), rather than raw options. Preserve explicit
escapechar=None semantics while ensuring inherited dialect/object escape and
quoting settings reach csv-core, including QUOTE_ALL and QUOTE_NONNUMERIC
behavior.
- Around line 1048-1050: Replace UTF-8-only decoding with WTF-8 construction
throughout the CSV parser and writer: update the parsed-field conversion at
crates/stdlib/src/csv.rs:1048-1050 and the quoted-string, QUOTE_NONE,
QUOTE_MINIMAL, and csv-core output paths at crates/stdlib/src/csv.rs:1451-1453,
1497-1500, 1546-1549, and 1631-1634. Preserve lone-surrogate PyStr values
instead of propagating UnicodeDecodeError, using the existing WTF-8 string
construction APIs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 946f5a98-7949-4579-8ce1-4cf10adf36ef

📥 Commits

Reviewing files that changed from the base of the PR and between d49a37f and 831deb0.

📒 Files selected for processing (1)
  • crates/stdlib/src/csv.rs

@hyoinandout
hyoinandout force-pushed the fix/csv-dialect-validation branch from 831deb0 to d49a37f Compare August 5, 2026 14:41
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

AI-Assisted-By: Codex
@youknowone

Copy link
Copy Markdown
Member

@widehyo1 could you please review this changes?

Comment thread crates/stdlib/src/csv.rs
Comment on lines 314 to 341
impl TryFromObject for PyDialect {
fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult<Self> {
let delimiter = parse_delimiter_from_obj(vm, &obj)?;
let quotechar = parse_quotechar_from_obj(vm, &obj)?;
let escapechar = parse_escapechar_from_obj(vm, &obj)?;
let doublequote = obj.get_attr("doublequote", vm)?.try_to_bool(vm)?;
let skipinitialspace = obj.get_attr("skipinitialspace", vm)?.try_to_bool(vm)?;
let lineterminator = prase_lineterminator_from_obj(vm, &obj)?;
let quoting = prase_quoting_from_obj(vm, &obj)?;

let strict = if let Ok(t) = obj.get_attr("strict", vm) {
t.try_to_bool(vm).unwrap_or(false)
} else {
false
};

Ok(Self {
delimiter,
quotechar,
escapechar,
doublequote,
skipinitialspace,
lineterminator,
quoting,
strict,
})
}
}

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.

@coderabbitai Is the "Validate dialect attributes during direct _csv.Dialect(...) construction" comment still valid?

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.

@fanninpm Yes. The comment is still valid.

Direct _csv.Dialect(...) construction calls PyDialect::try_from_object through PyDialect::py_new. That path parses the attributes but does not call validate_dialect.

register_dialect and FormatOptions::result call validate_dialect, but a directly constructed _csv.Dialect can bypass those paths. Add this before py_new returns:

let dialect = Self::try_from_object(vm, ctx)?;
validate_dialect(vm, &dialect)?;
Ok(dialect)

You are interacting with an AI system.

Comment thread crates/stdlib/src/csv.rs
Comment on lines 662 to 673
if let Some(escapechar) = args.kwargs.swap_remove("escapechar") {
res.escapechar = match_class!(match escapechar {
s @ PyStr =>
Some(s.as_bytes().iter().copied().exactly_one().map_err(|_| {
vm.new_type_error(r#""escapechar" must be a 1-character string"#)
})?),
_ => None,
s @ PyStr => Some(parse_single_char(&s, |_| {
vm.new_type_error(r#""escapechar" must be a 1-character string"#)
})?),
_ => {
return Err(ArgumentError::Exception(
vm.new_type_error(r#""escapechar" must be a 1-character string"#),
));
}
})
};

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.

@coderabbitai Is the "Use the dialect attribute validation helpers for escapechar and quotechar kwargs" comment still valid?

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.

@fanninpm, the comment is still partially valid.

The new parse_single_char call fixes validation for string values. It rejects invalid lengths and non-byte Unicode characters.

However, escapechar=None is still rejected because the keyword branch accepts only PyStr. The dialect attribute helper accepts None. Also, FormatOptions::escapechar: Option<u8> cannot represent an explicit None override that clears an escapechar inherited from a dialect.

The comment should be narrowed to preserve None handling and explicit override behavior.

You are interacting with an AI system.

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.

The comment in question is #8402 (review) authored by @coderabbitai.

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.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"

echo "== Review =="
gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" \
  --jq '{id, user: .user.login, state, body, submitted_at, commit_id}'

echo
echo "== Inline comments in review =="
gh api --paginate "repos/$repo/pulls/$pr_number/comments?per_page=100" \
  --jq ".[] | select(.pull_request_review_id == $review_id) | {id, path, line, side, body, html_url}"

Length of output: 32674


@fanninpm, thank you for the correction. I reviewed that original CodeRabbit review.

The original comment is partially valid.

  • The string validation part is resolved. escapechar now uses parse_single_char, and quotechar already validates a single character.
  • The remaining issue is escapechar=None. The escapechar keyword branch rejects None, although dialect attributes accept it.
  • FormatOptions::escapechar: Option<u8> also cannot represent an explicit None override for an escapechar inherited from a base dialect.
  • quotechar already has Option<Option<u8>>, so it can represent that override.

The original wording should be narrowed. It should request consistent escapechar=None handling and an Option<Option<u8>> representation for FormatOptions::escapechar. It should not require direct use of parse_escapechar_from_obj or parse_quotechar_from_obj, because those helpers read attributes from a dialect object rather than parse keyword values.

You are interacting with an AI system.

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.

Can @coderabbitai edit @coderabbitai's comment from earlier to that effect?

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.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"

gh api --paginate "repos/$repo/pulls/$pr_number/comments?per_page=100" \
  --jq ".[] | select(.pull_request_review_id == $review_id and .user.login == \"coderabbitai\") | {id, path, line, side, body, html_url}"

Length of output: 450


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
review_id="4788540642"

gh api --paginate "repos/$repo/pulls/reviews/$review_id/comments?per_page=100" \
  --jq '.[] | {id, user: .user.login, path, line, side, body, html_url}'

Length of output: 676


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"

gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" \
  --jq '{id, user: .user.login, state, body, submitted_at, commit_id, html_url}'

Length of output: 7318


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"
body_file="$(mktemp)"
updated_body_file="$(mktemp)"
trap 'rm -f "$body_file" "$updated_body_file"' EXIT

gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" --jq '.body' > "$body_file"

python3 - "$body_file" "$updated_body_file" <<'PY'
from pathlib import Path
import sys

source = Path(sys.argv[1]).read_text()
old_finding = """**Use the dialect attribute validation helpers for `escapechar` and `quotechar` kwargs.**

The `delimiter` kwarg already delegates to `parse_delimiter_from_obj`. `escapechar`/`quotechar` should use the corresponding helpers instead of inlining checks, so invalid types/lengths use the correct `TypeError` text and accept `PyNone` where that helper supports it. This closes the remaining keyword-argument path for issue `#8284`."""
new_finding = """**Support `escapechar=None` as a keyword override.**

The string validation is now correct. However, the `escapechar` keyword branch rejects `None`, while a dialect attribute accepts it. Also, `FormatOptions::escapechar: Option<u8>` cannot represent an explicit `None` override that clears an `escapechar` inherited from a base dialect. Change it to represent three states: omitted, a character, and explicit `None`. Keep `quotechar` behavior unchanged because `Option<Option<u8>>` already represents these states."""

old_prompt = """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."""
new_prompt = """In `@crates/stdlib/src/csv.rs` around lines 632 - 757, Update
FormatOptions so the `escapechar` keyword accepts `None` and can explicitly
clear an `escapechar` inherited from a base dialect. Use a representation that
distinguishes an omitted keyword, a one-character string, and explicit `None`.
Keep the existing single-character validation. Do not change `quotechar`
handling because it already distinguishes these states."""

if source.count(old_finding) != 1 or source.count(old_prompt) != 2:
    raise SystemExit("The expected review text was not found in the expected count.")

updated = source.replace(old_finding, new_finding).replace(old_prompt, new_prompt)
Path(sys.argv[2]).write_text(updated)
PY

gh api \
  --method PATCH \
  "repos/$repo/pulls/$pr_number/reviews/$review_id" \
  --raw-field "body=$(cat "$updated_body_file")" \
  --jq '{id, state, html_url}'

Length of output: 3032


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"
body_file="$(mktemp)"
trap 'rm -f "$body_file"' EXIT

gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" --jq '.body' > "$body_file"

rg -n -C 4 'Use the dialect attribute|FormatOptions::from_args|escapechar and quotechar|escapechar kwargs' "$body_file"

Length of output: 2244


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"
body_file="$(mktemp)"
updated_body_file="$(mktemp)"
trap 'rm -f "$body_file" "$updated_body_file"' EXIT

gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" --jq '.body' > "$body_file"

python3 - "$body_file" "$updated_body_file" <<'PY'
from pathlib import Path
import sys

source = Path(sys.argv[1]).read_text()

replacements = {
"""**Use the dialect attribute validation helpers for `escapechar` and `quotechar` kwargs.**

The `delimiter` kwarg already delegates to `parse_delimiter_from_obj`. `escapechar`/`quotechar` should use the corresponding helpers instead of inlining checks, so invalid types/lengths use the correct `TypeError` text and accept `PyNone` where that helper supports it. This closes the remaining keyword-argument path for issue `#8284`."""
:
"""**Support `escapechar=None` as a keyword override.**

The string validation is now correct. However, the `escapechar` keyword branch rejects `None`, while a dialect attribute accepts it. Also, `FormatOptions::escapechar: Option<u8>` cannot represent an explicit `None` override that clears an `escapechar` inherited from a base dialect. Change it to represent omitted, character, and explicit-`None` states. Keep `quotechar` behavior unchanged because `Option<Option<u8>>` already represents these states."""
,
"""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."""
:
"""In `@crates/stdlib/src/csv.rs` around lines 632 - 757, Update
FormatOptions so the `escapechar` keyword accepts `None` and can explicitly
clear an `escapechar` inherited from a base dialect. Use a representation that
distinguishes an omitted keyword, a one-character string, and explicit `None`.
Keep the existing single-character validation. Do not change `quotechar`
handling because it already distinguishes these states."""
,
"""- 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."""
:
"""- Around line 632-757: Update FormatOptions so the `escapechar` keyword
accepts `None` and can explicitly clear an inherited `escapechar`. Use a
representation that distinguishes an omitted keyword, a one-character string,
and explicit `None`. Keep the existing single-character validation. Do not
change `quotechar` handling because it already distinguishes these states."""
}

updated = source
for old, new in replacements.items():
    count = updated.count(old)
    if count != 1:
        raise SystemExit(f"Expected exactly one occurrence, found {count}: {old[:72]!r}")
    updated = updated.replace(old, new)

Path(sys.argv[2]).write_text(updated)
PY

gh api \
  --method PATCH \
  "repos/$repo/pulls/$pr_number/reviews/$review_id" \
  --raw-field "body=$(cat "$updated_body_file")" \
  --jq '{id, state, html_url}'

Length of output: 3757


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"
body_file="$(mktemp)"
trap 'rm -f "$body_file"' EXIT

gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" --jq '.body' > "$body_file"

sed -n '41,59p;99,104p' "$body_file" | cat -vet

Length of output: 1937


🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr_number="8402"
review_id="4788540642"
body_file="$(mktemp)"
updated_body_file="$(mktemp)"
trap 'rm -f "$body_file" "$updated_body_file"' EXIT

gh api "repos/$repo/pulls/$pr_number/reviews/$review_id" --jq '.body' > "$body_file"

python3 - "$body_file" "$updated_body_file" <<'PY'
from pathlib import Path
import sys

source = Path(sys.argv[1]).read_text()

replacements = {
"""> **Use the dialect attribute validation helpers for `escapechar` and `quotechar` kwargs.**
> 
> The `delimiter` kwarg already delegates to `parse_delimiter_from_obj`. `escapechar`/`quotechar` should use the corresponding helpers instead of inlining checks, so invalid types/lengths use the correct `TypeError` text and accept `PyNone` where that helper supports it. This closes the remaining keyword-argument path for issue `#8284`."""
:
"""> **Support `escapechar=None` as a keyword override.**
> 
> The string validation is now correct. However, the `escapechar` keyword branch rejects `None`, while a dialect attribute accepts it. Also, `FormatOptions::escapechar: Option<u8>` cannot represent an explicit `None` override that clears an `escapechar` inherited from a base dialect. Change it to represent omitted, character, and explicit-`None` states. Keep `quotechar` behavior unchanged because `Option<Option<u8>>` already represents these states."""
,
"""> 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."""
:
"""> In `@crates/stdlib/src/csv.rs` around lines 632 - 757, Update
> FormatOptions so the `escapechar` keyword accepts `None` and can explicitly
> clear an `escapechar` inherited from a base dialect. Use a representation that
> distinguishes an omitted keyword, a one-character string, and explicit `None`.
> Keep the existing single-character validation. Do not change `quotechar`
> handling because it already distinguishes these states."""
,
"""- 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."""
:
"""- Around line 632-757: Update FormatOptions so the `escapechar` keyword
accepts `None` and can explicitly clear an inherited `escapechar`. Use a
representation that distinguishes an omitted keyword, a one-character string,
and explicit `None`. Keep the existing single-character validation. Do not
change `quotechar` handling because it already distinguishes these states."""
}

updated = source
for old, new in replacements.items():
    count = updated.count(old)
    if count != 1:
        raise SystemExit(f"Expected exactly one occurrence, found {count}: {old[:72]!r}")
    updated = updated.replace(old, new)

Path(sys.argv[2]).write_text(updated)
PY

gh api \
  --method PATCH \
  "repos/$repo/pulls/$pr_number/reviews/$review_id" \
  --raw-field "body=$(cat "$updated_body_file")" \
  --jq '{id, state, html_url}'

Length of output: 3805


@fanninpm, I could not edit the earlier review. GitHub rejected the update request with HTTP 404 for the submitted review. The review text remains unchanged.

The narrowed finding is:

Support escapechar=None as a keyword override. The string validation is correct. However, the escapechar keyword rejects None, and FormatOptions::escapechar: Option<u8> cannot represent an explicit None override that clears an inherited escapechar.

You are interacting with an AI system.

Comment thread crates/stdlib/src/csv.rs
Comment thread crates/stdlib/src/csv.rs
Comment on lines +489 to +497
let dialect = options.result(vm)?;

Ok(Writer {
write,
state: PyMutex::new(WriteState {
buffer: vec![0; 1024],
writer: options.to_writer(),
}),
dialect: options.result(vm)?,
dialect,

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.

@coderabbitai Is the "Configure csv-core from the resolved dialect" comment still valid?

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.

@fanninpm Yes. The comment is still valid.

writer resolves dialect with options.result(vm), but it configures csv_core::Writer with options.to_writer(). These two paths can produce different settings.

For example, FormatOptions::to_writer() applies escapechar only from self.escapechar. It does not apply an escapechar inherited from the resolved PyDialect. The stored Writer::dialect uses that inherited value, but the csv_core::Writer does not.

Configure csv_core::Writer directly from the resolved dialect to keep both writer paths consistent.

You are interacting with an AI system.

Comment thread crates/stdlib/src/csv.rs Outdated
Comment on lines 889 to 946

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.

@coderabbitai Is the "Configure csv-core from the resolved dialect" comment still valid?

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.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

gh api \
  "repos/RustPython/RustPython/pulls/8402/comments?per_page=100" \
  --paginate \
  --jq '.[] | select(.id == 3722348588) | {id, body, path, line, original_line, in_reply_to_id, user: .user.login}'

Length of output: 599


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '=== Matching review comments ==='
gh api \
  "repos/RustPython/RustPython/pulls/8402/comments?per_page=100" \
  --paginate \
  --jq '.[] | select((.body | test("Configure csv-core from the resolved dialect|resolved dialect|csv-core"; "i")) or (.user.login == "coderabbitai")) | {id, body, path, line, original_line, in_reply_to_id, user: .user.login}'

echo '=== Current writer construction and csv-core configuration ==='
ast-grep outline crates/stdlib/src/csv.rs --items all --type function --match 'writer|to_writer|result' --view expanded
sed -n '470,510p;800,930p' crates/stdlib/src/csv.rs

Length of output: 7609


@fanninpm Yes. The comment is still valid.

writer resolves the final dialect with options.result(vm), but it builds csv_core::Writer with options.to_writer().

FormatOptions::to_writer reconstructs configuration from the source dialect and explicit options. For example, it configures escapechar only from self.escapechar. An escapechar inherited from a registered dialect is present in the resolved PyDialect, but it is not passed to csv_core::Writer.

to_writer should accept the resolved PyDialect and configure csv_core::Writer directly from it.

You are interacting with an AI system.

Comment thread crates/stdlib/src/csv.rs
Comment on lines 1046 to 1048
let value = core::str::from_utf8(&self.field)
.map_err(|e| new_not_utf8_error(vm, &self.field, e))?;
let field: PyObjectRef = vm.ctx.new_str(value).into();

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.

@coderabbitai Is the "Preserve WTF-8 when decoding and emitting CSV fields" comment still valid?

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.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="RustPython/RustPython"
pr="8402"

gh api --paginate "repos/${repo}/pulls/${pr}/comments?per_page=100" \
  --jq '.[] | select(.user.login == "coderabbitai" or (.body | test("WTF-8|WTF8|Wtf8"; "i"))) | {
    id,
    node_id,
    path,
    line,
    original_line,
    body,
    html_url,
    user: .user.login
  }'

Length of output: 820


🏁 Script executed:

#!/bin/bash
set -euo pipefail

gh api graphql \
  -F owner="RustPython" \
  -F name="RustPython" \
  -F number=8402 \
  -f query='
query($owner: String!, $name: String!, $number: Int!) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      reviewThreads(first: 100) {
        nodes {
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 100) {
            nodes {
              databaseId
              author { login }
              body
              url
              createdAt
            }
          }
        }
      }
    }
  }
}' \
  --jq '
    .data.repository.pullRequest.reviewThreads.nodes[]
    | select(any(.comments.nodes[]; .databaseId == 3722375710))
  '

Length of output: 1318


@fanninpm Yes. The comment is still valid.

The new parse_single_char function correctly uses as_wtf8().code_points() for dialect options. It does not change CSV field handling.

CsvParser::save_field still calls core::str::from_utf8(&self.field). The writer paths also call core::str::from_utf8(&output) before they invoke write. A Python str with a lone surrogate uses WTF-8 in RustPython. These UTF-8 conversions reject that value and raise UnicodeDecodeError.

The CSV reader and writer should use WTF-8-aware conversion and construction for field data. Tests should cover a field that contains a lone surrogate in both reader input and writer output.

You are interacting with an AI system.

@widehyo1

widehyo1 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@widehyo1 could you please review this changes?

yes i will.

@widehyo1

widehyo1 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

I think it is a great work! As above comments shows, I found some CPython incompatibilities
in the new dialect-validation paths. Could you please take a look?

  • Direct construction of _csv.Dialect avoids dialect validation
  • Dialect escapechar propagation does not reach to the writer backend used by writerow().
import _csv

BadDialect = type(
    "BadDialect",
    (),
    {
        "delimiter": "\n",
        "quotechar": '"',
        "escapechar": None,
        "doublequote": True,
        "skipinitialspace": False,
        "lineterminator": "\r\n",
        "quoting": 0,
    },
)

dialect = _csv.Dialect(BadDialect)
print(f"RustPython accepted invalid delimiter: {dialect.delimiter!r}")

# CPython raises: ValueError: bad delimiter value
# RustPython should also reject construction, rather than exposing this object.
import csv
from io import StringIO

csv.register_dialect(
    "inherited_escape",
    delimiter=",",
    quotechar='"',
    doublequote=False,
    escapechar="^",
    quoting=csv.QUOTE_ALL,
    lineterminator="\n",
)

output = StringIO()
writer = csv.writer(output, dialect="inherited_escape")

print(f"Stored escapechar: {writer.dialect.escapechar!r}")
writer.writerow(['a"b'])
print(f"Output: {output.getvalue()!r}")

# CPython output: '"a^"b"\n'
# Current RustPython output: '"a\\"b"\n'
# The stored dialect has '^', but the csv-core writer uses its default '\\'.

I think the first is related to py_new and the second is related to missing inheritance of escapechar via options.to_writer().

- Reject dialect characters that overlap any lineterminator character
- Validate Dialect construction consistently and allow non-ASCII terminators
- Unskip csv invalid character coverage now that validation matches behavior

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

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

1286-1332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Make field_needs_escape take the current byte explicitly.

field_needs_escape reads data[0] without a bounds check. The only caller guarantees a non-empty slice through split_first, so there is no current defect. A future caller can pass an empty slice and cause a panic. Pass the already-extracted byte and the remaining slice instead.

♻️ Proposed refactor
-        let mut data = data;
-        while let Some((&byte, rest)) = data.split_first() {
-            if field_needs_escape(data, dialect) {
+        let mut data = data;
+        while let Some((&byte, rest)) = data.split_first() {
+            if field_needs_escape(byte, data, dialect) {
                 let escapechar = dialect
                     .escapechar
                     .ok_or_else(|| new_csv_error(vm, "need to escape, but no escapechar set"))?;
                 output.push(escapechar);
             }
             output.push(byte);
             data = rest;
         }
-    fn field_needs_escape(data: &[u8], dialect: &PyDialect) -> bool {
-        let byte = data[0];
+    fn field_needs_escape(byte: u8, data: &[u8], dialect: &PyDialect) -> bool {
         byte == dialect.delimiter
🤖 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 1286 - 1332, Update field_needs_escape
to accept the already-extracted current byte and the remaining data slice as
separate arguments, removing its direct data[0] access. Adjust the caller in the
byte-writing loop to pass byte and rest while preserving all existing escape
checks, including data_starts_with_lineterminator_char on the remaining slice.
🤖 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 `@crates/stdlib/src/csv.rs`:
- Around line 1286-1332: Update field_needs_escape to accept the
already-extracted current byte and the remaining data slice as separate
arguments, removing its direct data[0] access. Adjust the caller in the
byte-writing loop to pass byte and rest while preserving all existing escape
checks, including data_starts_with_lineterminator_char on the remaining slice.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: dd5e54e5-cb35-4415-9fe8-2f8530f60cae

📥 Commits

Reviewing files that changed from the base of the PR and between 5e2da0c and 70ebe18.

⛔ 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

Removes a RustPython-only assertion that contradicts the
CPython-compatible dialect validation added by this branch.

Scope: test-only
AI-Assisted-By: Codex
csv.writer(io.StringIO(), lineterminator="é")

with assert_raises(csv.Error):
csv.writer(io.StringIO(), lineterminator="\x85")

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

There comes a contradict.

CPython’s test_writer_arg_valid requires no exception.
But here, it expects csv.Error, as described in the comments.

I am not aware of why the extra_tests exists for testing incompatibility between rustpython and Cpython.

@hyoinandout hyoinandout Aug 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Does it make sense to temporarily comment the testcase?
I think we can remove it since RustPython's csv module support unicode after all.

Comment thread Lib/test/test_csv.py
ctor(arg, delimiter='\x85')
ctor(arg, escapechar='\x85')
ctor(arg, quotechar='\x85')
ctor(arg, lineterminator='\x85')

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

CPython’s test_writer_arg_valid requires no exception.

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: reader allows escapechar=1

5 participants