iframe-proxy

doma17 · 2026-06-30T11:50:28Z

Closes csv.writer QUOTE_NONE with quotechar=None panics #8200
This PR follows our AI policy

Summary

Fixes csv.writer with quoting=csv.QUOTE_NONE and quotechar=None.

Problem

This case should match CPython:

'a\\,b,x"y\r\n'

RustPython accepted the dialect, but reached an internal todo!() path and panicked while writing the row.

Fixes

Skip csv-core quote character configuration when quotechar=None.
Add a QUOTE_NONE writer path that escapes fields without quoting them.
Match CPython behavior for missing escapechar and single empty fields.
Add regression coverage in extra_tests/snippets/stdlib_csv.py.

Testing

Verified the reproducer no longer panics.
Verified test_csv behavior locally.
Verified the new stdlib_csv regression.
Verified formatting, lint, pre-commit hooks, and workspace tests before opening the PR.

AI use

Assisted by Codex:gpt-5.5 for implementation, test drafting, verification. I reviewed and verified the changes locally.

Summary by CodeRabbit

Bug Fixes
- Improved CSV writing when using QUOTE_NONE with no quote character, including correct escaping of commas, quotes, backslashes, and line breaks.
- CSV output now handles empty fields more consistently and preserves expected validation errors for invalid writer settings.

Handle csv.writer rows with QUOTE_NONE through a RustPython-owned unquoted writer path so quotechar=None no longer reaches the unfinished csv_core configuration branch. Escape delimiter, newline, quotechar, and escapechar bytes according to the active dialect, and preserve CPython's single-empty-field error behavior. Constraint: Match CPython csv.writer behavior without changing Lib/ copied stdlib files. Rejected: Relying on csv_core QuoteStyle::Never | it does not model CPython QUOTE_NONE escaping with quotechar=None. Confidence: high Scope-risk: narrow Directive: Keep QUOTE_NONE writer behavior separate unless csv_core gains matching CPython semantics. Tested: prek run --all-files; cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher; cargo build --release --features sqlite; pytest -v in extra_tests Assisted-by: Codex:gpt-5.5

coderabbitai · 2026-06-30T11:51:03Z

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/stdlib/src/csv.rs (1)
1247-1292: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Consider extracting the shared row-writing boilerplate.

writerow_quote_none and writerow_quoted_strings are near-identical: both lock state, convert row to ArgIterable, compute single_field, run the same match_class field-stringification, enforce the single-empty-field rule, append the line terminator, UTF-8 validate, and call self.write. Only the per-field write step differs. Extracting a helper that takes a per-field closure (the differing value) and runs the common logic once would remove the duplication.

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 1247 - 1292, `writerow_quote_none`
duplicates the same row-processing flow already present in
`writerow_quoted_strings`: state locking, `ArgIterable` conversion,
`single_field` handling, field stringification with `match_class!`,
empty-single-field validation, lineterminator appending, UTF-8 checking, and
`self.write` invocation. Refactor this shared boilerplate into a helper that
takes the per-field write behavior as an argument (for example, a closure or
small strategy method), and have both `writerow_quote_none` and
`writerow_quoted_strings` call that helper while supplying only their differing
field-writing step.
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 `@extra_tests/snippets/stdlib_csv.py`:
- Around line 77-124: The new csv writer regression test in
test_quote_none_writer_without_quotechar should also cover carriage return and
newline escaping so field_needs_escape() is exercised for CR/LF paths. Add a
case using csv.writer with QUOTE_NONE and escapechar set that writes a field
containing \r and another containing \n, then assert the output escapes both
correctly, alongside the existing delimiter, quotechar, and None/empty-field
checks.

---

Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 1247-1292: `writerow_quote_none` duplicates the same
row-processing flow already present in `writerow_quoted_strings`: state locking,
`ArgIterable` conversion, `single_field` handling, field stringification with
`match_class!`, empty-single-field validation, lineterminator appending, UTF-8
checking, and `self.write` invocation. Refactor this shared boilerplate into a
helper that takes the per-field write behavior as an argument (for example, a
closure or small strategy method), and have both `writerow_quote_none` and
`writerow_quoted_strings` call that helper while supplying only their differing
field-writing step.

🪄 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: b44454e2-97e7-4d7c-8408-663c2624b255

📥 Commits

Reviewing files that changed from the base of the PR and between 0009dd6 and f29b7a3.

📒 Files selected for processing (2)

crates/stdlib/src/csv.rs
extra_tests/snippets/stdlib_csv.py

Remove the stale RustPython expected-failure marker for the CPython escaped-field writer test that now passes, and cover CR/LF escaping in the snippet regression requested during review. Constraint: RustPython test policy allows removing expectedFailure markers only when the upstream test passes. Rejected: Refactoring writer row boilerplate in this follow-up | review marked it as a heavy-lift nitpick and it would broaden the CI fix. Confidence: high Scope-risk: narrow Tested: cargo run --release --features sqlite -- -m test test_csv; cargo run -- extra_tests/snippets/stdlib_csv.py; PATH=/tmp/pyshim:$PATH prek run --all-files; git diff --check; cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher; pytest -v in extra_tests Assisted-by: Codex:gpt-5.5

github-actions · 2026-06-30T14:15:53Z

📦 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: 27)

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

ShaharNaveh

overall LGTM, just a minor nitpick

ShaharNaveh · 2026-07-02T07:52:45Z

+            if matches!(self.dialect.quoting, QuoteStyle::None) {
+                return self.writerow_quote_none(row, vm);
+            }
+


can we merge this if statement with the one below it? it seems like both are matching on self.dialect.quoting

Review feedback pointed out that adjacent branches selected behavior from the same quoting discriminator. Use one match so future quote-style additions are routed in a single place. Constraint: RustPython review requested merging adjacent quote-style checks. Confidence: high Scope-risk: narrow Tested: cargo fmt --check Tested: target/release/rustpython extra_tests/snippets/stdlib_csv.py Tested: target/release/rustpython -m test test_csv Tested: PATH=/tmp/pyshim:$PATH prek run --all-files Tested: RUST_TEST_THREADS=1 cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher -- --test-threads=1 Tested: cargo clippy --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher --all-targets Assisted-by: Codex:gpt-5.5

doma17 · 2026-07-02T11:07:15Z

ShaharNaveh

ty:)

coderabbitai Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread extra_tests/snippets/stdlib_csv.py

youknowone enabled auto-merge (squash) July 2, 2026 07:35

youknowone requested a review from ShaharNaveh July 2, 2026 07:35

ShaharNaveh requested changes Jul 2, 2026

View reviewed changes

auto-merge was automatically disabled July 2, 2026 11:05
Head branch was pushed to by a user without write access

ShaharNaveh approved these changes Jul 2, 2026

View reviewed changes

Sunbelt Computer Software

PL/B Language Development and Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix csv.writer QUOTE_NONE with quotechar=None#8201

Fix csv.writer QUOTE_NONE with quotechar=None#8201
doma17 wants to merge 3 commits into
RustPython:mainfrom
doma17:fix-csv-quote-none-no-quotechar

doma17 commented Jun 30, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 30, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 30, 2026

Uh oh!

ShaharNaveh left a comment

Uh oh!

ShaharNaveh Jul 2, 2026

Uh oh!

doma17 commented Jul 2, 2026

Uh oh!

ShaharNaveh left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

doma17 commented Jun 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Fixes

Testing

AI use

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 30, 2026

📦 Library Dependencies

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

doma17 commented Jul 2, 2026

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

doma17 commented Jun 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 30, 2026 •

edited

Loading