Support csv writer QUOTE_STRINGS and QUOTE_NOTNULL by doma17 · Pull Request #8109 · RustPython/RustPython

doma17 · 2026-06-15T10:37:31Z

Closes Panic in csv.writer with QUOTE_STRINGS or QUOTE_NOTNULL #8108
This PR follows our AI policy

Summary

Implements writer support for csv.QUOTE_STRINGS and csv.QUOTE_NOTNULL.
These constants were already exposed, but using either quote style with
csv.writer reached an internal todo!() and panicked.

Problem

csv.writer(..., quoting=csv.QUOTE_STRINGS) and
csv.writer(..., quoting=csv.QUOTE_NOTNULL) should behave like CPython.

Instead, RustPython converted these quote styles through an unimplemented path,
so normal Python code could terminate the interpreter with a Rust panic.

Fixes

Route QUOTE_STRINGS and QUOTE_NOTNULL away from the todo!() writer path.
Add Rust-side writer handling for both quote styles:
- QUOTE_STRINGS quotes original string fields and still quotes fields that
  require quoting because of delimiters, quote characters, or line endings.
- QUOTE_NOTNULL quotes every field except None.
- None is written as an empty unquoted field.
Match CPython error behavior for a single empty field produced from None.
Reject quotechar=None when quoting is enabled.
Add regression coverage in extra_tests/snippets/stdlib_csv.py.

Testing

Verified the original reproducer no longer panics.
Verified the new stdlib_csv regression with RustPython.
Verified test_csv passes.
Verified formatting/lint/pre-commit hooks.
Verified the workspace test suite.

AI use

Assisted by codex gpt-5.5 for implementation and test drafting. I reviewed, edited, and verified the changes locally.

Summary by CodeRabbit

Bug Fixes
- Corrected CSV writer quoting behavior for QUOTE_STRINGS and QUOTE_NOTNULL so fields are escaped/quoted according to the active dialect.
- Strengthened configuration validation: invalid quoting settings now raise appropriate errors, including rejecting quotechar=None when quoting is enabled.
New Features
- CSV writing now consistently applies the configured quoting/escaping rules for these quoting modes.
Tests
- Added in-memory buffer coverage to verify exact output and confirm expected exceptions for invalid quoting options.

coderabbitai · 2026-06-15T10:37:40Z

📝 Walkthrough

Walkthrough

Fixes a panic in csv.writer when quoting is QUOTE_STRINGS or QUOTE_NOTNULL. Maps both styles to csv_core::QuoteStyle::Necessary, broadens quotechar validation, adds internal helpers for manual field quoting, introduces a dedicated writerow_quoted_strings path, and adds tests covering the new behavior.

Changes

CSV QUOTE_STRINGS and QUOTE_NOTNULL writer implementation

Layer / File(s)	Summary
QuoteStyle mapping and quotechar validation `crates/stdlib/src/csv.rs`	`Strings`/`Notnull` now map to `csv_core::QuoteStyle::Necessary` instead of `todo!()`; `quotechar`-required check covers all quoting modes except `None`.
write_quoted_field, field_needs_quotes, write_lineterminator helpers `crates/stdlib/src/csv.rs`	Three new internal functions handle per-field quote-necessity logic, escaped/double-quote field writing, and line terminator emission.
writerow_quoted_strings path and writerow dispatch `crates/stdlib/src/csv.rs`	`writerow_quoted_strings` stringifies each field and applies conditional quoting; `writerow` dispatches to it for `Strings`/`Notnull` and falls through to `csv_core` for all other modes.
QUOTE_STRINGS / QUOTE_NOTNULL writer tests `extra_tests/snippets/stdlib_csv.py`	`test_quote_strings_and_notnull_writer` asserts exact CSV output, `csv.Error` for a lone `None` field, and `TypeError` for `quotechar=None`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

RustPython/RustPython#7834: Modifies crates/stdlib/src/csv.rs around QuoteStyle/FormatOptions::from_args and quotechar validation, which is the same code region changed in this PR.

Poem

🐇 A todo!() once lurked in the CSV den,
Panicking whenever QUOTE_STRINGS ran its pen.
I hopped in with helpers—quote, decide, and write—
Now strings get their quotes and None stays light.
No more panics, just commas and rows done right! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% 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
Title check	✅ Passed	The title accurately describes the main change: implementing writer support for QUOTE_STRINGS and QUOTE_NOTNULL in the CSV module.
Linked Issues check	✅ Passed	The PR fully implements all coding requirements from issue `#8108`: routes QUOTE_STRINGS and QUOTE_NOTNULL away from unimplemented code, implements correct quoting behavior for both styles, handles None fields correctly, rejects invalid quotechar=None combinations, and adds comprehensive test coverage.
Out of Scope Changes check	✅ Passed	All changes are directly related to implementing QUOTE_STRINGS and QUOTE_NOTNULL support in the CSV writer, with no unrelated modifications detected.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/stdlib/src/csv.rs (1)

1172-1173: 💤 Low value

Unused lock acquisition.

The _state lock guard is acquired but never used in this method since it builds output manually rather than using the csv_core::Writer in WriteState. While harmless, this unnecessary synchronization could be removed.

Proposed fix

     fn writerow_quoted_strings(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult {
-        let _state = self.state.lock();
         let row: ArgIterable = ArgIterable::try_from_object(vm, row.clone()).map_err(|_e| {

🤖 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 1172 - 1173, In the
writerow_quoted_strings method, remove the unnecessary lock acquisition line
`let _state = self.state.lock();` since the lock guard is never actually used in
the method. The method builds output manually without relying on the
csv_core::Writer in WriteState, so this synchronization is not needed and can be
safely deleted.

🤖 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 1138-1141: The code is unconditionally doubling the escape
character inside quoted fields when it encounters dialect.escapechar, but when
doublequote=True (the default), only quotes should be doubled, not escape
characters. Modify the logic that checks if dialect.escapechar == Some(byte) to
only apply the escape character doubling when doublequote=False. If doublequote
is True, skip the conditional block that pushes the byte an extra time for
escape characters, allowing only the single push outside the condition to
execute.

---

Nitpick comments:
In `@crates/stdlib/src/csv.rs`:
- Around line 1172-1173: In the writerow_quoted_strings method, remove the
unnecessary lock acquisition line `let _state = self.state.lock();` since the
lock guard is never actually used in the method. The method builds output
manually without relying on the csv_core::Writer in WriteState, so this
synchronization is not needed and can be safely deleted.

🪄 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: 4b5c06f1-7810-4acc-a372-e3408986c961

📥 Commits

Reviewing files that changed from the base of the PR and between dbfd69e and 1c59994.

📒 Files selected for processing (2)

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

ShaharNaveh

overall lgtm, just few nitpicks

I'm genuinely surprised that no new CPython tests are now passing with this change.

TYSM!

doma17 · 2026-06-15T14:22:11Z

overall lgtm, just few nitpicks

I'm genuinely surprised that no new CPython tests are now passing with this change.

TYSM!

I was surprised this wasn't already covered by a CPython test too :)

Thanks for review!

youknowone · 2026-06-15T14:27:57Z

Probably adding a test to cpython will be also worth to do if you are interested in

youknowone

Please check our AI policy. Creating PR yourself will show you AI policy template.

Implement writer handling for QUOTE_STRINGS and QUOTE_NOTNULL so the exported constants no longer panic and follow CPython behavior for string, non-null, None, and quotechar validation cases. Constraint: RustPython contribution policy requires CPython-compatible behavior through Rust-side changes and regression coverage outside Lib/. Rejected: Marking CPython Lib tests or hiding the constants | would preserve a runtime panic for supported public constants. Confidence: high Scope-risk: narrow Directive: Keep QUOTE_STRINGS and QUOTE_NOTNULL writer behavior aligned with CPython's csv module when extending dialect validation. Tested: cargo check -p rustpython-stdlib; cargo run --release -- extra_tests/snippets/stdlib_csv.py; cargo run --release -- -m test test_csv; cargo clippy; pre-commit run --all-files via temporary venv/PATH shim; cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher -- --test-threads=1 Not-tested: Remote CI and PR creation were not run because remote git operations require explicit approval. Assisted-by: Codex:gpt-5.5

Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com> Assisted-by: Codex:gpt-5.5

Apply maintainer-requested expression cleanups and rustfmt wrapping so the PR lint job can pass without changing behavior. Constraint: GitHub Actions Lint failed on rustfmt output after the review suggestion. Rejected: Changing csv writer semantics | the failing check was formatting-only and all behavior tests already passed. Confidence: high Scope-risk: narrow Directive: Keep future review suggestions rustfmt-clean before pushing. Tested: cargo fmt --check; cargo check -p rustpython-stdlib; cargo run --release -- extra_tests/snippets/stdlib_csv.py; cargo run --release -- -m test test_csv; cargo clippy; pre-commit run --all-files; cargo test --workspace --exclude rustpython_wasm --exclude rustpython-venvlauncher -- --test-threads=1 Not-tested: Remote CI has not rerun yet; this commit will trigger it after push. Assisted-by: Codex:gpt-5.5

doma17 marked this pull request as ready for review June 15, 2026 10:41

coderabbitai Bot reviewed Jun 15, 2026

View reviewed changes

Comment thread crates/stdlib/src/csv.rs

ShaharNaveh requested changes Jun 15, 2026

View reviewed changes

Comment thread crates/stdlib/src/csv.rs Outdated

Comment thread crates/stdlib/src/csv.rs Outdated

ShaharNaveh approved these changes Jun 15, 2026

View reviewed changes

youknowone requested changes Jun 16, 2026

View reviewed changes

doma17 and others added 4 commits June 16, 2026 16:36

Update crates/stdlib/src/csv.rs

b11d276

Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com> Assisted-by: Codex:gpt-5.5

Update crates/stdlib/src/csv.rs

aadf2e7

Co-authored-by: Shahar Naveh <50263213+ShaharNaveh@users.noreply.github.com> Assisted-by: Codex:gpt-5.5

doma17 force-pushed the fix-csv-quote-strings-notnull branch from e14d586 to eb9f111 Compare June 16, 2026 07:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support csv writer QUOTE_STRINGS and QUOTE_NOTNULL#8109

Support csv writer QUOTE_STRINGS and QUOTE_NOTNULL#8109
doma17 wants to merge 4 commits into
RustPython:mainfrom
doma17:fix-csv-quote-strings-notnull

doma17 commented Jun 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

ShaharNaveh left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

doma17 commented Jun 15, 2026

Uh oh!

youknowone commented Jun 15, 2026 •

edited

Loading

Uh oh!

youknowone left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

doma17 commented Jun 15, 2026 • edited 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 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShaharNaveh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

doma17 commented Jun 15, 2026

Uh oh!

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

Uh oh!

Uh oh!

youknowone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

doma17 commented Jun 15, 2026 •

edited

Loading

coderabbitai Bot commented Jun 15, 2026 •

edited

Loading

ShaharNaveh left a comment •

edited

Loading

youknowone commented Jun 15, 2026 •

edited

Loading

youknowone left a comment •

edited

Loading