csv: validate dialect options #8402
+99
−23
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ mod _csv { | |
| use itertools::Itertools; | ||
| use parking_lot::Mutex; | ||
| use rustpython_common::{lock::LazyLock, wtf8::Wtf8Buf}; | ||
| use rustpython_vm::match_class; | ||
| use std::collections::HashMap; | ||
|
|
||
| #[pyattr] | ||
|
|
@@ -173,12 +173,11 @@ mod _csv { | |
| } else { | ||
| match_class!(match obj.to_owned() { | ||
| s @ PyStr => { | ||
| parse_single_char(&s, |len| { | ||
| vm.new_type_error(format!( | ||
| r#""delimiter" must be a unicode character, not a string of length {len}"# | ||
| )) | ||
| }) | ||
| } | ||
| attr => { | ||
| Err(vm.new_type_error(format!( | ||
|
|
@@ -193,8 +192,13 @@ mod _csv { | |
| fn parse_quotechar_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult<Option<u8>> { | ||
| match_class!(match obj.get_attr("quotechar", vm)? { | ||
| s @ PyStr => { | ||
| Ok(Some(parse_single_char(&s, |len| { | ||
| new_csv_error( | ||
| vm, | ||
| format!( | ||
| r#""quotechar" must be a unicode character or None, not a string of length {len}"# | ||
| ), | ||
| ) | ||
| })?)) | ||
| } | ||
| _n @ PyNone => { | ||
|
|
@@ -215,10 +219,12 @@ mod _csv { | |
| fn parse_escapechar_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult<Option<u8>> { | ||
| match_class!(match obj.get_attr("escapechar", vm)? { | ||
| s @ PyStr => { | ||
| Ok(Some(parse_single_char(&s, |len| { | ||
| new_csv_error( | ||
| vm, | ||
| format!( | ||
| r#""escapechar" must be a unicode character or None, not a string of length {len}"# | ||
| ), | ||
| ) | ||
| })?)) | ||
| } | ||
|
|
@@ -277,6 +283,18 @@ mod _csv { | |
| }) | ||
| } | ||
|
|
||
| fn parse_single_char( | ||
| s: &Py<PyStr>, | ||
| error: impl Fn(usize) -> PyBaseExceptionRef, | ||
| ) -> PyResult<u8> { | ||
| let ch = s | ||
| .as_wtf8() | ||
| .code_points() | ||
| .exactly_one() | ||
| .map_err(|_| error(s.char_len()))?; | ||
| u8::try_from(ch.to_u32()).map_err(|_| error(s.char_len())) | ||
| } | ||
|
|
||
| fn prase_quoting_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult<QuoteStyle> { | ||
| match_class!(match obj.get_attr("quoting", vm)? { | ||
| i @ PyInt => { | ||
|
|
@@ -344,6 +362,7 @@ mod _csv { | |
| }; | ||
|
|
||
| let dialect = opts.update_py_dialect(dialect); | ||
| validate_dialect(vm, &dialect)?; | ||
| GLOBAL_HASHMAP | ||
| .lock() | ||
| .insert(name.as_str().to_owned(), dialect); | ||
|
|
@@ -441,13 +460,14 @@ mod _csv { | |
| _rest: FuncArgs, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<Reader> { | ||
| let dialect = options.result(vm)?; | ||
| Ok(Reader { | ||
| iter, | ||
| state: PyMutex::new(ReadState { | ||
| line_num: 0, | ||
| generation: 0, | ||
| }), | ||
| dialect, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -466,14 +486,15 @@ mod _csv { | |
| return Err(vm.new_type_error(r#"argument 1 must have a "write" method"#)); | ||
| } | ||
| }; | ||
| let dialect = options.result(vm)?; | ||
|
|
||
| Ok(Writer { | ||
| write, | ||
| state: PyMutex::new(WriteState { | ||
| buffer: vec![0; 1024], | ||
| writer: options.to_writer(), | ||
| }), | ||
| dialect, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -640,11 +661,14 @@ mod _csv { | |
|
|
||
| if let Some(escapechar) = args.kwargs.swap_remove("escapechar") { | ||
| res.escapechar = match_class!(match escapechar { | ||
| 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"#), | ||
| )); | ||
| } | ||
| }) | ||
| }; | ||
|
Comment on lines
662
to
673
Contributor
There was a problem hiding this comment. @coderabbitai Is the "Use the dialect attribute validation helpers for
Contributor
There was a problem hiding this comment.
The new However, The comment should be narrowed to preserve You are interacting with an AI system.
Contributor
There was a problem hiding this comment. The comment in question is #8402 (review) authored by @coderabbitai.
Contributor
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
The original comment is partially valid.
The original wording should be narrowed. It should request consistent You are interacting with an AI system.
Contributor
There was a problem hiding this comment. Can @coderabbitai edit @coderabbitai's comment from earlier to that effect?
Contributor
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
The narrowed finding is:
You are interacting with an AI system. |
||
|
|
||
|
|
@@ -690,9 +714,9 @@ mod _csv { | |
|
|
||
| if let Some(quotechar) = args.kwargs.swap_remove("quotechar") { | ||
| res.quotechar = match_class!(match quotechar { | ||
| s @ PyStr => Some(Some(parse_single_char(&s, |_| { | ||
| vm.new_type_error(r#""quotechar" must be a 1-character string"#) | ||
| })?)), | ||
| PyNone => { | ||
| if res | ||
| .quoting | ||
|
|
@@ -735,6 +759,58 @@ mod _csv { | |
| } | ||
| } | ||
|
|
||
| fn validate_dialect(vm: &VirtualMachine, dialect: &PyDialect) -> PyResult<()> { | ||
| let special = |name: &str, value: u8| { | ||
| if matches!(value, b'\r' | b'\n') { | ||
| Err(vm.new_value_error(format!( | ||
| "{name} must be a single character, not a line break" | ||
| ))) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| }; | ||
|
|
||
| special("delimiter", dialect.delimiter)?; | ||
| if let Some(quotechar) = dialect.quotechar { | ||
| special("quotechar", quotechar)?; | ||
| } | ||
| if let Some(escapechar) = dialect.escapechar { | ||
| special("escapechar", escapechar)?; | ||
| } | ||
|
|
||
| if dialect.skipinitialspace | ||
| && (matches!(dialect.escapechar, Some(b' ')) || matches!(dialect.quotechar, Some(b' '))) | ||
| { | ||
| return Err(vm.new_value_error( | ||
| "escapechar or quotechar cannot be a space when skipinitialspace is enabled", | ||
| )); | ||
| } | ||
|
|
||
| let values = [ | ||
| ("delimiter", Some(core::slice::from_ref(&dialect.delimiter))), | ||
| ( | ||
| "quotechar", | ||
| dialect.quotechar.as_ref().map(core::slice::from_ref), | ||
| ), | ||
| ( | ||
| "escapechar", | ||
| dialect.escapechar.as_ref().map(core::slice::from_ref), | ||
| ), | ||
| ("lineterminator", Some(dialect.lineterminator.as_bytes())), | ||
| ]; | ||
| for (index, (left_name, left)) in values.iter().enumerate() { | ||
| let Some(left) = left else { continue }; | ||
| for (right_name, right) in values.iter().skip(index + 1) { | ||
| if right.as_ref() == Some(left) { | ||
| return Err(vm.new_value_error(format!( | ||
| "{left_name} and {right_name} cannot be the same" | ||
| ))); | ||
| } | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| Ok(()) | ||
| } | ||
|
|
||
| impl FormatOptions { | ||
| fn update_py_dialect(&self, mut res: PyDialect) -> PyDialect { | ||
| macro_rules! check_and_fill { | ||
|
|
@@ -768,7 +844,7 @@ mod _csv { | |
| } | ||
|
|
||
| fn result(&self, vm: &VirtualMachine) -> PyResult<PyDialect> { | ||
| let dialect = match &self.dialect { | ||
| DialectItem::Str(name) => { | ||
| let g = GLOBAL_HASHMAP.lock(); | ||
| if let Some(dialect) = g.get(name) { | ||
|
|
@@ -784,7 +860,9 @@ mod _csv { | |
| let res = g.get("excel").unwrap().clone(); | ||
| Ok(self.update_py_dialect(res)) | ||
| } | ||
| }?; | ||
| validate_dialect(vm, &dialect)?; | ||
| Ok(dialect) | ||
| } | ||
|
|
||
| fn get_quoting(&self) -> QuoteStyle { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanninpmYes. The comment is still valid.writerresolvesdialectwithoptions.result(vm), but it configurescsv_core::Writerwithoptions.to_writer(). These two paths can produce different settings.For example,
FormatOptions::to_writer()appliesescapecharonly fromself.escapechar. It does not apply anescapecharinherited from the resolvedPyDialect. The storedWriter::dialectuses that inherited value, but thecsv_core::Writerdoes not.Configure
csv_core::Writerdirectly from the resolveddialectto keep both writer paths consistent.You are interacting with an AI system.