From d49a37fe00782ec0f5c7e1542f1b9564c2f2c708 Mon Sep 17 00:00:00 2001 From: hyoinandout Date: Mon, 27 Jul 2026 23:51:14 +0900 Subject: [PATCH 1/3] csv: validate dialect options 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 --- Lib/test/test_csv.py | 2 - crates/stdlib/src/csv.rs | 190 +++++++++++++++++++++++++++------------ 2 files changed, 131 insertions(+), 61 deletions(-) diff --git a/Lib/test/test_csv.py b/Lib/test/test_csv.py index 379f4c9b799..b76318d7160 100644 --- a/Lib/test/test_csv.py +++ b/Lib/test/test_csv.py @@ -87,12 +87,10 @@ def _test_arg_valid(self, ctor, arg): self.assertRaises(ValueError, ctor, arg, quotechar='\x85', lineterminator='\x85') - @unittest.expectedFailure # TODO: RUSTPYTHON def test_reader_arg_valid(self): self._test_arg_valid(csv.reader, []) self.assertRaises(OSError, csv.reader, BadIterable()) - @unittest.expectedFailure # TODO: RUSTPYTHON def test_writer_arg_valid(self): self._test_arg_valid(csv.writer, StringIO()) class BadWriter: diff --git a/crates/stdlib/src/csv.rs b/crates/stdlib/src/csv.rs index 91717801bc4..495a1e5bb74 100644 --- a/crates/stdlib/src/csv.rs +++ b/crates/stdlib/src/csv.rs @@ -17,7 +17,7 @@ mod _csv { use itertools::Itertools; use parking_lot::Mutex; use rustpython_common::{lock::LazyLock, wtf8::Wtf8Buf}; - use rustpython_vm::{match_class, sliceable::SliceableSequenceOp}; + use rustpython_vm::match_class; use std::collections::HashMap; #[pyattr] @@ -169,12 +169,11 @@ mod _csv { } else { match_class!(match obj.to_owned() { s @ PyStr => { - Ok(s.as_bytes().iter().copied().exactly_one().map_err(|_| { + parse_single_char(&s, |len| { vm.new_type_error(format!( - r#""delimiter" must be a unicode character, not a string of length {}"#, - s.len() + r#""delimiter" must be a unicode character, not a string of length {len}"# )) - })?) + }) } attr => { Err(vm.new_type_error(format!( @@ -189,8 +188,13 @@ mod _csv { fn parse_quotechar_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { match_class!(match obj.get_attr("quotechar", vm)? { s @ PyStr => { - Ok(Some(s.as_bytes().iter().copied().exactly_one().map_err(|_| { - new_csv_error(vm, format!(r#""quotechar" must be a unicode character or None, not a string of length {}"#, s.len())) + 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 => { @@ -211,10 +215,12 @@ mod _csv { fn parse_escapechar_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult> { match_class!(match obj.get_attr("escapechar", vm)? { s @ PyStr => { - Ok(Some(s.as_bytes().iter().copied().exactly_one().map_err(|_| { + 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 {}"#, s.len()), + format!( + r#""escapechar" must be a unicode character or None, not a string of length {len}"# + ), ) })?)) } @@ -235,13 +241,10 @@ mod _csv { s @ PyStr => { Ok(if s.as_bytes().eq(b"\r\n") { csv_core::Terminator::CRLF - } else if let Some(t) = s.as_bytes().first() { - // Due to limitations in the current implementation within csv_core - // the support for multiple characters in lineterminator is not complete. - // only capture the first character - csv_core::Terminator::Any(*t) } else { - return Err(new_csv_error(vm, r#""lineterminator" must be a string"#)); + csv_core::Terminator::Any(parse_first_char(&s, |_| { + new_csv_error(vm, r#""lineterminator" must be a string"#) + })?) }) } attr => { @@ -253,6 +256,30 @@ mod _csv { }) } + fn parse_single_char( + s: &Py, + error: impl Fn(usize) -> PyBaseExceptionRef, + ) -> PyResult { + 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 parse_first_char( + s: &Py, + error: impl Fn(usize) -> PyBaseExceptionRef, + ) -> PyResult { + let ch = s + .as_wtf8() + .code_points() + .next() + .ok_or_else(|| 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 { match_class!(match obj.get_attr("quoting", vm)? { i @ PyInt => { @@ -320,6 +347,7 @@ mod _csv { }; let dialect = opts.update_py_dialect(dialect); + validate_dialect(vm, &dialect)?; GLOBAL_HASHMAP .lock() .insert(name.as_str().to_owned(), dialect); @@ -417,17 +445,18 @@ mod _csv { _rest: FuncArgs, vm: &VirtualMachine, ) -> PyResult { + let dialect = options.result(vm)?; Ok(Reader { iter, state: PyMutex::new(ReadState { buffer: vec![0; 1024], output_ends: vec![0; 16], reader: options.to_reader(), - skipinitialspace: options.get_skipinitialspace(), + skipinitialspace: dialect.skipinitialspace, line_num: 0, generation: 0, }), - dialect: options.result(vm)?, + dialect, }) } @@ -446,6 +475,7 @@ mod _csv { return Err(vm.new_type_error(r#"argument 1 must have a "write" method"#)); } }; + let dialect = options.result(vm)?; Ok(Writer { write, @@ -453,7 +483,7 @@ mod _csv { buffer: vec![0; 1024], writer: options.to_writer(), }), - dialect: options.result(vm)?, + dialect, }) } @@ -620,24 +650,34 @@ mod _csv { 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"#), + )); + } }) }; if let Some(lineterminator) = args.kwargs.swap_remove("lineterminator") { - res.lineterminator = Some(csv_core::Terminator::Any( - lineterminator - .try_to_value::<&str>(vm)? - .bytes() - .exactly_one() - .map_err(|_| { - vm.new_type_error(r#""lineterminator" must be a 1-character string"#) - })?, - )) + res.lineterminator = Some(match_class!(match lineterminator { + s @ PyStr => { + if s.as_bytes() == b"\r\n" { + Terminator::CRLF + } else { + Terminator::Any(parse_single_char(&s, |_| { + vm.new_type_error(r#""lineterminator" must be a string"#) + })?) + } + } + _ => { + return Err(ArgumentError::Exception( + vm.new_type_error(r#""lineterminator" must be a string"#), + )); + } + })); }; if let Some(doublequote) = args.kwargs.swap_remove("doublequote") { @@ -671,9 +711,9 @@ mod _csv { if let Some(quotechar) = args.kwargs.swap_remove("quotechar") { res.quotechar = match_class!(match quotechar { - s @ PyStr => Some(Some(s.as_bytes().iter().copied().exactly_one().map_err( - |_| { vm.new_type_error(r#""quotechar" must be a 1-character string"#) } - )?)), + s @ PyStr => Some(Some(parse_single_char(&s, |_| { + vm.new_type_error(r#""quotechar" must be a 1-character string"#) + })?)), PyNone => { if res .quoting @@ -716,6 +756,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)?; + } + + let line_terminator = match dialect.lineterminator { + Terminator::CRLF => None, + Terminator::Any(value) => Some(value), + _ => unreachable!(), + }; + 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(dialect.delimiter)), + ("quotechar", dialect.quotechar), + ("escapechar", dialect.escapechar), + ("lineterminator", line_terminator), + ]; + 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" + ))); + } + } + } + Ok(()) + } + impl FormatOptions { const fn update_py_dialect(&self, mut res: PyDialect) -> PyDialect { macro_rules! check_and_fill { @@ -747,7 +839,7 @@ mod _csv { } fn result(&self, vm: &VirtualMachine) -> PyResult { - match &self.dialect { + let dialect = match &self.dialect { DialectItem::Str(name) => { let g = GLOBAL_HASHMAP.lock(); if let Some(dialect) = g.get(name) { @@ -763,29 +855,9 @@ mod _csv { let res = *g.get("excel").unwrap(); Ok(self.update_py_dialect(res)) } - } - } - - fn get_skipinitialspace(&self) -> bool { - let mut skipinitialspace = match &self.dialect { - DialectItem::Str(name) => { - let g = GLOBAL_HASHMAP.lock(); - if let Some(dialect) = g.get(name) { - dialect.skipinitialspace - // TODO: RUSTPYTHON; Perfecting the remaining attributes. - } else { - false - } - } - DialectItem::Obj(obj) => obj.skipinitialspace, - _ => false, - }; - - if let Some(attr) = self.skipinitialspace { - skipinitialspace = attr - } - - skipinitialspace + }?; + validate_dialect(vm, &dialect)?; + Ok(dialect) } fn get_lineterminator(&self) -> csv_core::Terminator { From c698c865cdaa7c00091c95b8182ac43be7432ad5 Mon Sep 17 00:00:00 2001 From: hyoinandout Date: Wed, 5 Aug 2026 23:43:27 +0900 Subject: [PATCH 2/3] cargo fmt --- crates/stdlib/src/csv.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/stdlib/src/csv.rs b/crates/stdlib/src/csv.rs index 495a1e5bb74..f2ae7acc301 100644 --- a/crates/stdlib/src/csv.rs +++ b/crates/stdlib/src/csv.rs @@ -781,8 +781,7 @@ mod _csv { _ => unreachable!(), }; if dialect.skipinitialspace - && (matches!(dialect.escapechar, Some(b' ')) - || matches!(dialect.quotechar, Some(b' '))) + && (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", From 70ebe18539f87a19de53fed6be0eb240ebd79252 Mon Sep 17 00:00:00 2001 From: hyoinandout Date: Sat, 8 Aug 2026 00:55:44 +0900 Subject: [PATCH 3/3] Validate csv dialect conflicts with line terminators - 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 --- Lib/test/test_csv.py | 1 - crates/stdlib/src/csv.rs | 179 +++++++++++++-------------------------- 2 files changed, 60 insertions(+), 120 deletions(-) diff --git a/Lib/test/test_csv.py b/Lib/test/test_csv.py index 1585f35d42c..0fbf026aee2 100644 --- a/Lib/test/test_csv.py +++ b/Lib/test/test_csv.py @@ -1286,7 +1286,6 @@ class mydialect(csv.Dialect): self.assertEqual(str(cm.exception), '"lineterminator" must be a string, not NoneType') - @unittest.expectedFailure # TODO: RUSTPYTHON def test_invalid_chars(self): def create_invalid(field_name, value, **kwargs): class mydialect(csv.Dialect): diff --git a/crates/stdlib/src/csv.rs b/crates/stdlib/src/csv.rs index 2eda90e19c2..bfe701b06ec 100644 --- a/crates/stdlib/src/csv.rs +++ b/crates/stdlib/src/csv.rs @@ -100,8 +100,10 @@ mod _csv { impl Constructor for PyDialect { type Args = PyObjectRef; - fn py_new(_cls: &Py, ctx: Self::Args, vm: &VirtualMachine) -> PyResult { - Self::try_from_object(vm, ctx) + fn py_new(_cls: &Py, obj: Self::Args, vm: &VirtualMachine) -> PyResult { + let dialect = Self::try_from_object(vm, obj)?; + validate_dialect(vm, &dialect)?; + Ok(dialect) } } @@ -240,26 +242,7 @@ mod _csv { }) } - /// Validate that a line terminator is ASCII and return it as a `str`. - /// - /// The writer's quoting and escaping predicates compare raw bytes, so a - /// non-ASCII terminator would either quote a field that merely shares a - /// UTF-8 lead byte or splice an escape character into the middle of a - /// multi-byte sequence. Reject those here. - /// - /// The ASCII check must come before any UTF-8 conversion so that lone - /// surrogates are reported as this `csv.Error` too. - /// - /// TODO: RUSTPYTHON; handle non-ASCII terminators code-point-wise as part - /// of full Unicode dialect support. - fn ascii_lineterminator<'a>(vm: &VirtualMachine, s: &'a PyStr) -> PyResult<&'a str> { - if !s.as_wtf8().is_ascii() { - return Err(new_csv_error( - vm, - r#""lineterminator" must be an ASCII string"#, - )); - } - // An ASCII string is always valid UTF-8. + fn parse_lineterminator<'a>(vm: &VirtualMachine, s: &'a PyStr) -> PyResult<&'a str> { s.to_str() .ok_or_else(|| new_csv_error(vm, r#""lineterminator" must be a string"#)) } @@ -271,7 +254,7 @@ mod _csv { // arbitrary-length terminator; the manual writer paths emit it // verbatim and the csv-core writer path appends it after a // sentinel terminator (see `writerow`). - let value = ascii_lineterminator(vm, &s)?; + let value = parse_lineterminator(vm, &s)?; Ok(value.to_owned()) } attr => { @@ -492,7 +475,7 @@ mod _csv { write, state: PyMutex::new(WriteState { buffer: vec![0; 1024], - writer: options.to_writer(), + writer: FormatOptions::to_writer(&dialect), }), dialect, }) @@ -578,7 +561,7 @@ mod _csv { dialect: DialectItem, delimiter: Option, quotechar: Option>, - escapechar: Option, + escapechar: Option>, doublequote: Option, skipinitialspace: Option, lineterminator: Option, @@ -661,9 +644,10 @@ mod _csv { if let Some(escapechar) = args.kwargs.swap_remove("escapechar") { res.escapechar = match_class!(match escapechar { - s @ PyStr => Some(parse_single_char(&s, |_| { + s @ PyStr => Some(Some(parse_single_char(&s, |_| { vm.new_type_error(r#""escapechar" must be a 1-character string"#) - })?), + })?)), + PyNone => Some(None), _ => { return Err(ArgumentError::Exception( vm.new_type_error(r#""escapechar" must be a 1-character string"#), @@ -679,7 +663,7 @@ mod _csv { lineterminator.class().name() )) })?; - let value = ascii_lineterminator(vm, s)?; + let value = parse_lineterminator(vm, s)?; res.lineterminator = Some(value.to_owned()); }; @@ -786,27 +770,29 @@ mod _csv { )); } - 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())), + let values: [(&str, Option); 3] = [ + ("delimiter", Some(dialect.delimiter)), + ("quotechar", dialect.quotechar), + ("escapechar", dialect.escapechar), ]; 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) { + if left.is_some() && left == right { return Err(vm.new_value_error(format!( "{left_name} and {right_name} cannot be the same" ))); } } + if left.is_some_and(|value| { + dialect + .lineterminator + .chars() + .any(|character| character == value as char) + }) { + return Err(vm.new_value_error(format!( + "{left_name} and lineterminator cannot be the same" + ))); + } } Ok(()) } @@ -828,7 +814,7 @@ mod _csv { check_and_fill!(res, skipinitialspace); if let Some(t) = self.escapechar { - res.escapechar = Some(t); + res.escapechar = t; }; if let Some(t) = self.quotechar { @@ -865,81 +851,23 @@ mod _csv { Ok(dialect) } - fn get_quoting(&self) -> QuoteStyle { - let mut quoting = match &self.dialect { - DialectItem::Str(name) => { - let g = GLOBAL_HASHMAP.lock(); - if let Some(dialect) = g.get(name) { - dialect.quoting - } else { - QuoteStyle::Minimal - } - } - DialectItem::Obj(obj) => obj.quoting, - _ => QuoteStyle::Minimal, - }; - - if let Some(attr) = self.quoting { - quoting = attr - } - - quoting - } - - fn to_writer(&self) -> csv_core::Writer { + fn to_writer(dialect: &PyDialect) -> csv_core::Writer { let mut builder = csv_core::WriterBuilder::new(); - let mut writer = match &self.dialect { - DialectItem::Str(name) => { - let g = GLOBAL_HASHMAP.lock(); - if let Some(dialect) = g.get(name) { - let mut builder = builder - .delimiter(dialect.delimiter) - .double_quote(dialect.doublequote); - - if let Some(t) = dialect.quotechar { - builder = builder.quote(t); - } - - builder - - // TODO: RUSTPYTHON; Perfecting the remaining attributes. - } else { - &mut builder - } - } - DialectItem::Obj(obj) => { - let mut builder = builder - .delimiter(obj.delimiter) - .double_quote(obj.doublequote); - - if let Some(t) = obj.quotechar { - builder = builder.quote(t); - } - - builder - } - _ => &mut builder, - }; - - if let Some(t) = self.delimiter { - writer = writer.delimiter(t); - } + let mut writer = builder + .delimiter(dialect.delimiter) + .double_quote(dialect.doublequote); - if let Some(Some(t)) = self.quotechar { + if let Some(t) = dialect.quotechar { writer = writer.quote(t); } - if let Some(t) = self.doublequote { - writer = writer.double_quote(t); - } - writer = writer.terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)); - if let Some(e) = self.escapechar { + if let Some(e) = dialect.escapechar { writer = writer.escape(e); } - writer = writer.quote_style(self.get_quoting().into()); + writer = writer.quote_style(dialect.quoting.into()); writer.build() } @@ -1355,39 +1283,52 @@ mod _csv { dialect: &PyDialect, vm: &VirtualMachine, ) -> PyResult<()> { - for &byte in data { - if field_needs_escape(byte, dialect) { + let mut data = data; + while let Some((&byte, rest)) = data.split_first() { + if field_needs_escape(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; } Ok(()) } + fn data_contains_lineterminator_char(data: &[u8], dialect: &PyDialect) -> bool { + dialect.lineterminator.chars().any(|character| { + let mut encoded = [0; 4]; + let character = character.encode_utf8(&mut encoded).as_bytes(); + data.windows(character.len()) + .any(|window| window == character) + }) + } + + fn data_starts_with_lineterminator_char(data: &[u8], dialect: &PyDialect) -> bool { + dialect.lineterminator.chars().any(|character| { + let mut encoded = [0; 4]; + let character = character.encode_utf8(&mut encoded).as_bytes(); + data.starts_with(character) + }) + } + fn field_needs_quotes(data: &[u8], dialect: &PyDialect) -> bool { data.iter().any(|&byte| { byte == dialect.delimiter || dialect.quotechar == Some(byte) || matches!(byte, b'\r' | b'\n') - // CPython quotes a field containing any character of the line - // terminator. The terminator is ASCII-validated at parse time, so - // comparing raw bytes cannot match part of a multi-byte character. - // TODO: RUSTPYTHON; supporting non-ASCII terminators needs - // code-point-wise quoting and escaping as part of full - // Unicode dialect support. - || dialect.lineterminator.as_bytes().contains(&byte) - }) + }) || data_contains_lineterminator_char(data, dialect) } - fn field_needs_escape(byte: u8, dialect: &PyDialect) -> bool { + fn field_needs_escape(data: &[u8], dialect: &PyDialect) -> bool { + let byte = data[0]; byte == dialect.delimiter || dialect.quotechar == Some(byte) || dialect.escapechar == Some(byte) || matches!(byte, b'\r' | b'\n') - || dialect.lineterminator.as_bytes().contains(&byte) + || data_starts_with_lineterminator_char(data, dialect) } fn write_lineterminator(output: &mut Vec, terminator: &str) {