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
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCSV 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. ChangesCSV dialect behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
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
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/csv.py dependencies:
dependent tests: (4 tests)
Legend:
|
|
@hyoinandout Could you resolve conflicts? |
There was a problem hiding this comment.
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 winValidate every character in
lineterminator.
exactly_one()discards multi-character terminators before collision checking. A dialect withdelimiter='|'andlineterminator="\n|"passes validation even though the delimiter occurs in the terminator. Check each ASCII byte oflineterminatoragainstdelimiter,quotechar, andescapechar.🤖 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 winConfigure csv-core from the resolved dialect.
Writerstores the resolveddialect, butFormatOptions::to_writer()configures csv-core from raw options. An inheritedescapecharfrom a dialect or object option can stay inWriter.dialectwithout reaching.escape(), soQUOTE_ALLandQUOTE_NONNUMERICoutput may use csv-core quoting instead of stored quoting. Build the csv-core writer fromoptions.result(vm)?while still preserving explicitescapechar=Nonebehavior.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 liftPreserve WTF-8 when decoding and emitting CSV fields.
PyStrcan contain WTF-8 for lone surrogates. These paths pass rawPyStrbytes through the parser or writer, then reject them withfrom_utf8. Valid Python string values can therefore fail withUnicodeDecodeError.
crates/stdlib/src/csv.rs#L1048-L1050: construct the parsed field from WTF-8 instead offrom_utf8.crates/stdlib/src/csv.rs#L1451-L1453: construct quoted-string output from WTF-8.crates/stdlib/src/csv.rs#L1497-L1500: constructQUOTE_NONEoutput from WTF-8.crates/stdlib/src/csv.rs#L1546-L1549: constructQUOTE_MINIMALoutput 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
📒 Files selected for processing (1)
crates/stdlib/src/csv.rs
831deb0 to
d49a37f
Compare
|
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
|
@widehyo1 could you please review this changes? |
| 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, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
@coderabbitai Is the "Validate dialect attributes during direct _csv.Dialect(...) construction" comment still valid?
There was a problem hiding this comment.
@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.
| 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"#), | ||
| )); | ||
| } | ||
| }) | ||
| }; |
There was a problem hiding this comment.
@coderabbitai Is the "Use the dialect attribute validation helpers for escapechar and quotechar kwargs" comment still valid?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
The comment in question is #8402 (review) authored by @coderabbitai.
There was a problem hiding this comment.
🧩 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.
escapecharnow usesparse_single_char, andquotecharalready validates a single character. - The remaining issue is
escapechar=None. Theescapecharkeyword branch rejectsNone, although dialect attributes accept it. FormatOptions::escapechar: Option<u8>also cannot represent an explicitNoneoverride for anescapecharinherited from a base dialect.quotecharalready hasOption<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.
There was a problem hiding this comment.
Can @coderabbitai edit @coderabbitai's comment from earlier to that effect?
There was a problem hiding this comment.
🧩 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 -vetLength 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=Noneas a keyword override. The string validation is correct. However, theescapecharkeyword rejectsNone, andFormatOptions::escapechar: Option<u8>cannot represent an explicitNoneoverride that clears an inheritedescapechar.
You are interacting with an AI system.
| 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, |
There was a problem hiding this comment.
@coderabbitai Is the "Configure csv-core from the resolved dialect" comment still valid?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@coderabbitai Is the "Configure csv-core from the resolved dialect" comment still valid?
There was a problem hiding this comment.
🧩 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.rsLength 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.
| 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(); |
There was a problem hiding this comment.
@coderabbitai Is the "Preserve WTF-8 when decoding and emitting CSV fields" comment still valid?
There was a problem hiding this comment.
🧩 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.
yes i will. |
|
I think it is a great work! As above comments shows, I found some CPython incompatibilities
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 |
- 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/stdlib/src/csv.rs (1)
1286-1332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
field_needs_escapetake the current byte explicitly.
field_needs_escapereadsdata[0]without a bounds check. The only caller guarantees a non-empty slice throughsplit_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
⛔ 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
Removes a RustPython-only assertion that contradicts the CPython-compatible dialect validation added by this branch. Scope: test-only AI-Assisted-By: Codex
This reverts commit 3a145db.
| csv.writer(io.StringIO(), lineterminator="é") | ||
|
|
||
| with assert_raises(csv.Error): | ||
| csv.writer(io.StringIO(), lineterminator="\x85") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Does it make sense to temporarily comment the testcase?
I think we can remove it since RustPython's csv module support unicode after all.
| ctor(arg, delimiter='\x85') | ||
| ctor(arg, escapechar='\x85') | ||
| ctor(arg, quotechar='\x85') | ||
| ctor(arg, lineterminator='\x85') |
There was a problem hiding this comment.
CPython’s test_writer_arg_valid requires no exception.
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