diff --git a/Lib/test/test_csv.py b/Lib/test/test_csv.py index 379f4c9b799..2b1de5d70d9 100644 --- a/Lib/test/test_csv.py +++ b/Lib/test/test_csv.py @@ -262,7 +262,6 @@ def test_write_escape(self): self._write_test(['C\\', '6', '7', 'X"'], 'C\\\\,6,7,"X"""', escapechar='\\', quoting=csv.QUOTE_MINIMAL) - @unittest.expectedFailure # TODO: RUSTPYTHON def test_write_lineterminator(self): for lineterminator in '\r\n', '\n', '\r', '!@#', '\0': with self.subTest(lineterminator=lineterminator): diff --git a/crates/stdlib/src/csv.rs b/crates/stdlib/src/csv.rs index a5203e8a373..69f5c26017a 100644 --- a/crates/stdlib/src/csv.rs +++ b/crates/stdlib/src/csv.rs @@ -62,18 +62,27 @@ mod _csv { #[pyattr] #[pyclass(module = "csv", name = "Dialect")] - #[derive(Debug, PyPayload, Clone, Copy)] + #[derive(Debug, PyPayload, Clone)] struct PyDialect { delimiter: u8, quotechar: Option, escapechar: Option, doublequote: bool, skipinitialspace: bool, - lineterminator: csv_core::Terminator, + lineterminator: String, quoting: QuoteStyle, strict: bool, } + /// Placeholder single-byte terminator for the csv-core writer paths + /// (`QUOTE_ALL` / `QUOTE_NONNUMERIC`). csv-core can only emit a single byte + /// for the record terminator, but its `terminator()` call also performs + /// essential bookkeeping — closing the final quote and emitting `""` for an + /// empty record — that must not be bypassed. So the writer emits this + /// sentinel byte, and `writerow` strips it and appends the real (possibly + /// multi-character) line terminator afterwards. + const CSV_CORE_TERMINATOR_SENTINEL: u8 = b'\n'; + impl Constructor for PyDialect { type Args = PyObjectRef; @@ -106,11 +115,7 @@ mod _csv { #[pygetset] fn lineterminator(&self, vm: &VirtualMachine) -> PyRef { - match self.lineterminator { - Terminator::CRLF => vm.ctx.new_str("\r\n".to_string()), - Terminator::Any(t) => vm.ctx.new_str(format!("{}", t as char)), - _ => unreachable!(), - } + vm.ctx.new_str(self.lineterminator.clone()) } #[pygetset] @@ -215,19 +220,20 @@ mod _csv { }) } - fn prase_lineterminator_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult { + fn prase_lineterminator_from_obj(vm: &VirtualMachine, obj: &PyObject) -> PyResult { match_class!(match obj.get_attr("lineterminator", vm)? { 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"#)); - }) + // Store the full line terminator string. CPython accepts an + // 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 = s + .to_str() + .ok_or_else(|| new_csv_error(vm, r#""lineterminator" must be a string"#))?; + if value.is_empty() { + return Err(new_csv_error(vm, r#""lineterminator" must not be empty"#)); + } + Ok(value.to_owned()) } attr => { Err(vm.new_type_error(format!( @@ -329,7 +335,7 @@ mod _csv { let g = GLOBAL_HASHMAP.lock(); if let Some(dialect) = g.get(name.as_str()) { - return Ok(*dialect); + return Ok(dialect.clone()); } Err(new_csv_error(vm, "unknown dialect")) @@ -525,7 +531,7 @@ mod _csv { escapechar: Option, doublequote: Option, skipinitialspace: Option, - lineterminator: Option, + lineterminator: Option, quoting: Option, strict: Option, } @@ -614,15 +620,16 @@ mod _csv { }; 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"#) - })?, - )) + let value = lineterminator.try_to_value::<&str>(vm)?; + // Preserve the previous behavior of rejecting an empty terminator + // (full validation parity is deferred to a follow-up). Any + // non-empty string, including multi-character ones, is stored. + if value.is_empty() { + return Err(vm + .new_type_error(r#""lineterminator" must not be empty"#) + .into()); + } + res.lineterminator = Some(value.to_owned()); }; if let Some(doublequote) = args.kwargs.swap_remove("doublequote") { @@ -702,7 +709,7 @@ mod _csv { } impl FormatOptions { - const fn update_py_dialect(&self, mut res: PyDialect) -> PyDialect { + fn update_py_dialect(&self, mut res: PyDialect) -> PyDialect { macro_rules! check_and_fill { ($res:ident, $e:ident) => {{ if let Some(t) = self.$e { @@ -726,7 +733,9 @@ mod _csv { }; check_and_fill!(res, quoting); - check_and_fill!(res, lineterminator); + if let Some(t) = &self.lineterminator { + res.lineterminator.clone_from(t); + }; check_and_fill!(res, strict); res } @@ -736,16 +745,16 @@ mod _csv { DialectItem::Str(name) => { let g = GLOBAL_HASHMAP.lock(); if let Some(dialect) = g.get(name) { - Ok(self.update_py_dialect(*dialect)) + Ok(self.update_py_dialect(dialect.clone())) } else { Err(new_csv_error(vm, format!("{name} is not registered."))) } // TODO: Maybe need to update the obj from HashMap } - DialectItem::Obj(o) => Ok(self.update_py_dialect(*o)), + DialectItem::Obj(o) => Ok(self.update_py_dialect(o.clone())), DialectItem::None => { let g = GLOBAL_HASHMAP.lock(); - let res = *g.get("excel").unwrap(); + let res = g.get("excel").unwrap().clone(); Ok(self.update_py_dialect(res)) } } @@ -773,27 +782,6 @@ mod _csv { skipinitialspace } - fn get_lineterminator(&self) -> csv_core::Terminator { - let mut lineterminator = match &self.dialect { - DialectItem::Str(name) => { - let g = GLOBAL_HASHMAP.lock(); - if let Some(dialect) = g.get(name) { - dialect.lineterminator - } else { - Terminator::CRLF - } - } - DialectItem::Obj(obj) => obj.lineterminator, - _ => Terminator::CRLF, - }; - - if let Some(attr) = self.lineterminator { - lineterminator = attr - } - - lineterminator - } - fn get_quoting(&self) -> QuoteStyle { let mut quoting = match &self.dialect { DialectItem::Str(name) => { @@ -817,11 +805,11 @@ mod _csv { fn to_reader(&self) -> csv_core::Reader { let dialect = match &self.dialect { - DialectItem::Str(name) => GLOBAL_HASHMAP.lock().get(name).copied(), - DialectItem::Obj(obj) => Some(*obj), + DialectItem::Str(name) => GLOBAL_HASHMAP.lock().get(name).cloned(), + DialectItem::Obj(obj) => Some(obj.clone()), DialectItem::None => { let g = GLOBAL_HASHMAP.lock(); - Some(*g.get("excel").unwrap()) + Some(g.get("excel").unwrap().clone()) } }; @@ -853,10 +841,6 @@ mod _csv { reader = reader.quoting(self.quoting != Some(QuoteStyle::None)); } - if let Some(t) = self.lineterminator { - reader = reader.terminator(t); - } - if let Some(t) = self.doublequote { reader = reader.double_quote(t); } @@ -865,7 +849,12 @@ mod _csv { reader = reader.escape(self.escapechar); } - reader = reader.terminator(self.lineterminator.unwrap_or(Terminator::CRLF)); + // CPython's reader ignores the dialect's `lineterminator` entirely and + // only recognizes `\r`, `\n`, and `\r\n` as record separators. Match + // that: always use CRLF mode. Feeding a multi-byte terminator's first + // byte here would otherwise split records mid-UTF-8 and raise a + // UnicodeDecodeError. + reader = reader.terminator(Terminator::CRLF); reader.build() } @@ -878,8 +867,7 @@ mod _csv { if let Some(dialect) = g.get(name) { let mut builder = builder .delimiter(dialect.delimiter) - .double_quote(dialect.doublequote) - .terminator(dialect.lineterminator); + .double_quote(dialect.doublequote); if let Some(t) = dialect.quotechar { builder = builder.quote(t); @@ -895,8 +883,7 @@ mod _csv { DialectItem::Obj(obj) => { let mut builder = builder .delimiter(obj.delimiter) - .double_quote(obj.doublequote) - .terminator(obj.lineterminator); + .double_quote(obj.doublequote); if let Some(t) = obj.quotechar { builder = builder.quote(t); @@ -919,7 +906,7 @@ mod _csv { writer = writer.double_quote(t); } - writer = writer.terminator(self.get_lineterminator()); + writer = writer.terminator(Terminator::Any(CSV_CORE_TERMINATOR_SENTINEL)); if let Some(e) = self.escapechar { writer = writer.escape(e); @@ -964,8 +951,8 @@ mod _csv { } #[pygetset] - const fn dialect(&self, _vm: &VirtualMachine) -> PyDialect { - self.dialect + fn dialect(&self, _vm: &VirtualMachine) -> PyDialect { + self.dialect.clone() } } @@ -999,7 +986,7 @@ mod _csv { &mut self, input: &[u8], index: usize, - dialect: PyDialect, + dialect: &PyDialect, unquoted_escape: bool, ) -> (QuoteScanEvent, usize) { let byte = input[index]; @@ -1052,7 +1039,7 @@ mod _csv { fn read_quote_record( input: &[u8], - dialect: PyDialect, + dialect: &PyDialect, field_limit: isize, vm: &VirtualMachine, ) -> PyResult> { @@ -1164,13 +1151,13 @@ mod _csv { ) || (zelf.dialect.quoting == QuoteStyle::None && zelf.dialect.escapechar.is_some()); if use_quote_record { - let out = read_quote_record(input, zelf.dialect, field_limit, vm)?; + let out = read_quote_record(input, &zelf.dialect, field_limit, vm)?; *line_num += 1; return Ok(PyIterReturn::Return(vm.ctx.new_list(out).into())); } #[inline] - fn trim_initial_spaces(input: &[u8], dialect: PyDialect) -> Vec { + fn trim_initial_spaces(input: &[u8], dialect: &PyDialect) -> Vec { let mut trimmed = Vec::with_capacity(input.len()); let mut scan_state = QuoteScanState::new(); let mut index = 0; @@ -1200,7 +1187,7 @@ mod _csv { } let input = if *skipinitialspace { - String::from_utf8(trim_initial_spaces(input, zelf.dialect)).unwrap() + String::from_utf8(trim_initial_spaces(input, &zelf.dialect)).unwrap() } else { String::from_utf8(input.to_vec()).unwrap() }; @@ -1299,7 +1286,7 @@ mod _csv { fn write_quoted_field( output: &mut Vec, data: &[u8], - dialect: PyDialect, + dialect: &PyDialect, vm: &VirtualMachine, ) -> PyResult<()> { let quotechar = dialect @@ -1331,7 +1318,7 @@ mod _csv { fn write_unquoted_field( output: &mut Vec, data: &[u8], - dialect: PyDialect, + dialect: &PyDialect, vm: &VirtualMachine, ) -> PyResult<()> { for &byte in data { @@ -1346,36 +1333,35 @@ mod _csv { Ok(()) } - fn field_needs_quotes(data: &[u8], dialect: PyDialect) -> bool { + 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') - || matches!(dialect.lineterminator, Terminator::Any(t) if byte == t) + // CPython quotes a field that contains any character of the line + // terminator. This byte-wise `contains` matches CPython for + // ASCII terminators; non-ASCII terminators are not fully handled. + || dialect.lineterminator.as_bytes().contains(&byte) }) } - fn field_needs_escape(byte: u8, dialect: PyDialect) -> bool { + fn field_needs_escape(byte: u8, dialect: &PyDialect) -> bool { byte == dialect.delimiter || dialect.quotechar == Some(byte) || dialect.escapechar == Some(byte) || matches!(byte, b'\r' | b'\n') - || matches!(dialect.lineterminator, Terminator::Any(t) if byte == t) + || dialect.lineterminator.as_bytes().contains(&byte) } - fn write_lineterminator(output: &mut Vec, terminator: Terminator) { - match terminator { - Terminator::CRLF => output.extend_from_slice(b"\r\n"), - Terminator::Any(byte) => output.push(byte), - _ => unreachable!(), - } + fn write_lineterminator(output: &mut Vec, terminator: &str) { + output.extend_from_slice(terminator.as_bytes()); } #[pyclass(flags(DISALLOW_INSTANTIATION))] impl Writer { #[pygetset(name = "dialect")] - const fn get_dialect(&self, _vm: &VirtualMachine) -> PyDialect { - self.dialect + fn get_dialect(&self, _vm: &VirtualMachine) -> PyDialect { + self.dialect.clone() } fn writerow_quoted_strings(&self, row: PyObjectRef, vm: &VirtualMachine) -> PyResult { @@ -1406,12 +1392,12 @@ mod _csv { }); let should_quote = match self.dialect.quoting { - QuoteStyle::Strings => is_str || field_needs_quotes(data, self.dialect), + QuoteStyle::Strings => is_str || field_needs_quotes(data, &self.dialect), QuoteStyle::Notnull => !is_none, _ => unreachable!(), }; if should_quote { - write_quoted_field(&mut output, data, self.dialect, vm)?; + write_quoted_field(&mut output, data, &self.dialect, vm)?; } else if single_field && data.is_empty() { return Err(new_csv_error( vm, @@ -1422,7 +1408,7 @@ mod _csv { } } - write_lineterminator(&mut output, self.dialect.lineterminator); + write_lineterminator(&mut output, &self.dialect.lineterminator); let s = core::str::from_utf8(&output) .map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?; self.write.call((s,), vm) @@ -1464,10 +1450,10 @@ mod _csv { )); } - write_unquoted_field(&mut output, data, self.dialect, vm)?; + write_unquoted_field(&mut output, data, &self.dialect, vm)?; } - write_lineterminator(&mut output, self.dialect.lineterminator); + write_lineterminator(&mut output, &self.dialect.lineterminator); let s = core::str::from_utf8(&output) .map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?; @@ -1509,14 +1495,14 @@ mod _csv { // terminator, regardless of which line terminator is // configured. A row with a single empty field is also quoted // so that it is not read back as an empty line. - if field_needs_quotes(data, self.dialect) || (single_field && data.is_empty()) { - write_quoted_field(&mut output, data, self.dialect, vm)?; + if field_needs_quotes(data, &self.dialect) || (single_field && data.is_empty()) { + write_quoted_field(&mut output, data, &self.dialect, vm)?; } else { output.extend_from_slice(data); } } - write_lineterminator(&mut output, self.dialect.lineterminator); + write_lineterminator(&mut output, &self.dialect.lineterminator); let s = core::str::from_utf8(&output) .map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?; @@ -1592,7 +1578,15 @@ mod _csv { handle_res!(writer.terminator(&mut buffer[buffer_offset..])); } - let s = core::str::from_utf8(&buffer[..buffer_offset]) + // csv-core just emitted the single-byte sentinel terminator (after + // closing the final quote / emitting an empty record as needed). + // Drop that sentinel byte and append the real, possibly + // multi-character, line terminator. + assert_eq!(buffer[buffer_offset - 1], CSV_CORE_TERMINATOR_SENTINEL); + let mut output = buffer[..buffer_offset - 1].to_vec(); + output.extend_from_slice(self.dialect.lineterminator.as_bytes()); + + let s = core::str::from_utf8(&output) .map_err(|_| vm.new_unicode_decode_error("csv not utf8"))?; self.write.call((s,), vm) diff --git a/extra_tests/snippets/stdlib_csv.py b/extra_tests/snippets/stdlib_csv.py index 61b82459a28..d686672f1d8 100644 --- a/extra_tests/snippets/stdlib_csv.py +++ b/extra_tests/snippets/stdlib_csv.py @@ -181,6 +181,83 @@ def test_quote_minimal_writer_lineterminator(): test_quote_minimal_writer_lineterminator() +def test_multichar_lineterminator(): + # https://github.com/RustPython/RustPython/issues/8322 + # The writer must store and emit a full multi-character line terminator. + for lineterminator in "\r\n", "\n", "\r", "!@#", "\0": + buf = io.StringIO() + writer = csv.writer(buf, lineterminator=lineterminator) + writer.writerow(["a", "b"]) + writer.writerow([1, 2]) + writer.writerow(["\r", "\n"]) + assert buf.getvalue() == ( + f'a,b{lineterminator}1,2{lineterminator}"\r","\n"{lineterminator}' + ), (lineterminator, buf.getvalue()) + + # A field is quoted when it contains any byte of the terminator (QUOTE_MINIMAL). + for field, expected in [ + ("a@b", '"a@b",x!@#'), + ("a!b", '"a!b",x!@#'), + ("a#b", '"a#b",x!@#'), + ("abc", "abc,x!@#"), + ]: + buf = io.StringIO() + csv.writer(buf, lineterminator="!@#").writerow([field, "x"]) + assert buf.getvalue() == expected, (field, buf.getvalue()) + + # The csv-core-backed QUOTE_ALL / QUOTE_NONNUMERIC paths emit the full + # terminator too, and keep the state machine correct across rows. + allq = io.StringIO() + writer = csv.writer(allq, lineterminator="!@#", quoting=csv.QUOTE_ALL) + writer.writerow(["a", "b"]) + writer.writerow(["c", "d"]) + assert allq.getvalue() == '"a","b"!@#"c","d"!@#', allq.getvalue() + + nonnum = io.StringIO() + csv.writer(nonnum, lineterminator="!@#", quoting=csv.QUOTE_NONNUMERIC).writerow( + ["a", 1] + ) + assert nonnum.getvalue() == '"a",1!@#', nonnum.getvalue() + + # A field that itself contains a line-break byte must be kept intact: the + # csv-core path drops only the trailing record terminator, not a byte from + # the field data. + embedded = io.StringIO() + csv.writer(embedded, lineterminator="!@#", quoting=csv.QUOTE_ALL).writerow( + ["x\ny", "z"] + ) + assert embedded.getvalue() == '"x\ny","z"!@#', embedded.getvalue() + + # QUOTE_NONE escapes any byte of the terminator. + none = io.StringIO() + csv.writer( + none, lineterminator="!@#", quoting=csv.QUOTE_NONE, escapechar="\\" + ).writerow(["a!b", "x"]) + assert none.getvalue() == "a\\!b,x!@#", none.getvalue() + + # register_dialect round-trips a multi-character terminator. + csv.register_dialect("multichar_lt", delimiter=",", lineterminator="!@#") + try: + reg = io.StringIO() + csv.writer(reg, dialect="multichar_lt").writerow(["a", "b"]) + assert reg.getvalue() == "a,b!@#", reg.getvalue() + finally: + csv.unregister_dialect("multichar_lt") + + # The dialect attribute reflects the full terminator. + assert ( + csv.writer(io.StringIO(), lineterminator="!@#").dialect.lineterminator == "!@#" + ) + + # The reader ignores lineterminator (like CPython) and only splits on \r\n. + assert list(csv.reader(io.StringIO("a,b!@#c,d!@#"), lineterminator="!@#")) == [ + ["a", "b!@#c", "d!@#"] + ] + + +test_multichar_lineterminator() + + def test_quote_minimal_writer_empty_fields(): buf = io.StringIO() writer = csv.writer(buf)