Fix amazon-ion#955 by removing support for cr-only line endings

Author: popematt

Cargo.toml

@@ -71,6 +71,7 @@ sha2 = { version = "0.9", optional = true }
 serde = { version = "1.0", features = ["derive"], optional = true }
 serde_with = { version = "3.7.0", optional = true }
 visibility = "0.1.1"
+memchr = "2.7.4"
 
 [dev-dependencies]
 rstest = "0.19.0"

src/lazy/value.rs

@@ -595,107 +595,16 @@ mod tests {
         Ok(())
     }
 
-    #[rstest]
-    #[case::no_crlf("{foo: 1, bar: 2}\"hello\"", (1,17))]
-    #[case::cr_lf_lf("{foo: 1, bar: 2}\r\n\n\"hello\"", (3,1))]
-    #[case::lf_lf_cr("{foo: 1, bar: 2}\n\n\r\"hello\"", (4,1))]
-    #[case::cr_lf_cr("{foo: 1, bar: 2}\r\n\r\"hello\"", (3,1))]
-    #[case::cr_cr_cr("{foo: 1, bar: 2}\r\r\r\"hello\"", (4,1))]
-    #[case::cr_cr_lf("{foo: 1, bar: 2}\r\r\n\"hello\"", (3,1))]
-    #[case::lf_cr_cr("{foo: 1, bar: 2}\n\r\r\"hello\"", (4,1))]
-    #[case::lf_cr_lf("{foo: 1, bar: 2}\n\r\n\"hello\"", (3,1))]
-    #[case::lf_lf_lf("{foo: 1, bar: 2}\n\n\n\"hello\"", (4,1))]
-    #[case::newlines_after("{foo: 1, bar: 2}\"hello\"\n\n", (1, 17))]
-    #[case::tabs("{foo: 1, bar: 2}\n\t\t\t\"hello\"", (2,4))]
-    #[case::tabs_after("{foo: 1, bar: 2}\"hello\"\t\t", (1,17))]
-    #[case::mix_tabs_and_newlines("{foo: 1, bar: 2}\n\t\n\"hello\"", (3,1))]
-    #[case::long_string("{foo: 1, bar: 2}\n\n'''long \n\r\n\t hello'''", (3, 1))]
-    #[case::comment("{foo: 1, bar: 2}\n\n /*multiline \n comment*/'''long \n\r\n\t hello'''", (4, 11))]
-    #[case::on_same_line_as_preceding_multiline_value("{\n  foo: 1,\n  bar: 2\n}\"hello\"", (4, 2))]
-    fn location_test_for_second_tlv(
-        #[case] ion_text: &str,
-        #[case] expected_location: (usize, usize),
-    ) -> IonResult<()> {
-        let mut reader = Reader::new(v1_0::Text, ion_text)?;
-        let result1 = reader.expect_next();
-
-        let expected_source_location =
-            SourceLocation::new(expected_location.0, expected_location.1);
-        assert!(result1.is_ok());
-        if let Ok(lazy_value1) = result1 {
-            let _val = lazy_value1.read();
-            // first tlv will always be (1,1) per the examples here
-            assert_eq!(lazy_value1.location(), SourceLocation::new(1, 1));
-        }
-        let result2 = reader.expect_next();
-        assert!(result2.is_ok());
-        if let Ok(lazy_value2) = result2 {
-            let _val = lazy_value2.read();
-            assert_eq!(lazy_value2.location(), expected_source_location);
-        }
-        Ok(())
-    }
-
-    #[rstest]
-    #[case::no_crlf(vec!["{foo: 1, bar: 2}","\"hello\""], (1,17))]
-    #[case::cr_lf_lf(vec!["{foo: 1, ", "bar: 2}\r\n\n\"hello\""], (3,1))]
-    #[case::lf_lf_cr(vec!["{foo: 1, bar: 2}","\n\n\r\"hello\""], (4,1))]
-    #[case::cr_lf_cr(vec!["{foo: 1, bar: 2}\r\n\r","\"hello\""], (3,1))]
-    #[case::cr_cr_cr(vec!["{foo: 1, bar: 2}\r\r\r","\"hello\""], (4,1))]
-    #[case::cr_cr_lf(vec!["{foo: 1, bar: 2}\r\r\n\"he","llo\""], (3,1))]
-    #[case::lf_cr_cr(vec!["{foo: 1, bar: 2}\n\r\r\"hello\""], (4,1))]
-    #[case::lf_cr_lf(vec!["{foo: 1, bar: 2}\n\r\n\"hello\""], (3,1))]
-    #[case::lf_lf_lf(vec!["{foo: 1, bar: 2}\n\n\n\"hello\""], (4,1))]
-    #[case::newlines_after(vec!["{foo: 1, bar: 2}\"hello\"\n\n"], (1, 17))]
-    #[case::tabs(vec!["{foo: 1, bar: 2}\n\t\t\t\"hello\""], (2,4))]
-    #[case::tabs_after(vec!["{foo: 1, bar: 2}\"hello\"","\t\t"], (1,17))]
-    #[case::mix_tabs_and_newlines(vec!["{foo: 1, bar: 2}\n\t\n\"hello\""], (3,1))]
-    #[case::long_string(vec!["{foo: 1, bar: 2}\n\n'''long \n\r\n\t hello'''"], (3, 1))]
-    #[case::comment(vec!["{foo: 1, bar: 2}\n\n", "/*multiline \n comment*/","'''long \n\r\n\t hello'''"], (4, 11))]
-    #[case::on_same_line_as_preceding_multiline_value(vec!["{\n  foo: 1,\n  bar: 2\n}\"hello\""], (4, 2))]
-    fn location_test_for_second_tlv_in_stream(
-        #[case] ion_text: Vec<&str>,
-        #[case] expected_location: (usize, usize),
-    ) -> IonResult<()> {
-        use crate::IonStream;
-        use std::io;
-        use std::io::{Cursor, Read};
-
-        let expected_source_location =
-            SourceLocation::new(expected_location.0, expected_location.1);
-        let input_chunks = ion_text.as_slice();
-        // Wrapping each string in an `io::Chain`
-        let mut input: Box<dyn Read> = Box::new(io::empty());
-        for input_chunk in input_chunks {
-            input = Box::new(input.chain(Cursor::new(input_chunk)));
-        }
-        let mut reader = Reader::new(v1_0::Text, IonStream::new(input))?;
-        let result1 = reader.expect_next();
-        assert!(result1.is_ok());
-        if let Ok(lazy_value1) = result1 {
-            let _val = lazy_value1.read();
-            // first tlv will always be (1,1) per the examples here
-            assert_eq!(lazy_value1.location(), SourceLocation::new(1, 1));
-        }
-        let result2 = reader.expect_next();
-        assert!(result2.is_ok());
-        if let Ok(lazy_value2) = result2 {
-            let _val = lazy_value2.read();
-            assert_eq!(lazy_value2.location(), expected_source_location);
-        }
-        Ok(())
-    }
-
     #[rstest]
     #[case::no_crlf( "{}\"hello\"",       [(1, 1), (1, 3)])]
     #[case::cr_lf_lf("{}\r\n\n\"hello\"", [(1, 1), (3, 1)] )]
-    #[case::lf_lf_cr("{}\n\n\r\"hello\"", [(1, 1), (4, 1)] )]
-    #[case::cr_lf_cr("{}\r\n\r\"hello\"", [(1, 1), (3, 1)] )]
-    #[case::cr_cr_cr("{}\r\r\r\"hello\"", [(1, 1), (4, 1)] )]
-    #[case::cr_cr_lf("{}\r\r\n\"hello\"", [(1, 1), (3, 1)] )]
-    #[case::lf_cr_cr("{}\n\r\r\"hello\"", [(1, 1), (4, 1)] )]
+    #[case::lf_lf_cr("{}\n\n\r\"hello\"", [(1, 1), (3, 2)] )]
+    #[case::lf_cr_cr("{}\n\r\r\"hello\"", [(1, 1), (2, 3)] )]
     #[case::lf_cr_lf("{}\n\r\n\"hello\"", [(1, 1), (3, 1)] )]
-    #[case::lf_lf_lf("{}\n\n\n\"hello\"", [(1, 1), (4, 1)] )]
+    #[case::cr_lf_lf("{}\r\n\n\"hello\"", [(1, 1), (3, 1)] )]
+    #[case::cr_lf_cr("{}\r\n\r\"hello\"", [(1, 1), (2, 2)] )]
+    #[case::cr_cr_lf("{}\r\r\n\"hello\"", [(1, 1), (2, 1)] )]
+    #[case::cr_cr_cr("{}\r\r\r\"hello\"", [(1, 1), (1, 6)] )]
     #[case::nl_after("{}\"hello\"\n\n",   [(1, 1), (1, 3)])]
     #[case::tabs(    "{}\n\t\t\"hello\"", [(1, 1), (2, 3)] )]
     #[case::tabs_after("{}\"hello\"\t\t", [(1, 1), (1, 3)])]
@@ -785,14 +694,13 @@ mod tests {
     #[rstest]
     #[case::no_crlf( "{}\"hello\"",       [(1, 1), (1, 3)])]
     #[case::lf_lf_lf("{}\n\n\n\"hello\"", [(1, 1), (4, 1)] )]
-    #[case::lf_lf_cr("{}\n\n\r\"hello\"", [(1, 1), (4, 1)] )]
-    #[case::lf_cr_cr("{}\n\r\r\"hello\"", [(1, 1), (4, 1)] )]
-    // FIXME: Currently failing because of https://github.com/amazon-ion/ion-rust/issues/955
-    // #[case::lf_cr_lf("{}\n\r\n\"hello\"", [(1, 1), (3, 1)] )]
-    // #[case::cr_lf_lf("{}\r\n\n\"hello\"", [(1, 1), (3, 1)] )]
-    // #[case::cr_lf_cr("{}\r\n\r\"hello\"", [(1, 1), (3, 1)] )]
-    // #[case::cr_cr_lf("{}\r\r\n\"hello\"", [(1, 1), (3, 1)] )]
-    #[case::cr_cr_cr("{}\r\r\r\"hello\"", [(1, 1), (4, 1)] )]
+    #[case::lf_lf_cr("{}\n\n\r\"hello\"", [(1, 1), (3, 2)] )]
+    #[case::lf_cr_cr("{}\n\r\r\"hello\"", [(1, 1), (2, 3)] )]
+    #[case::lf_cr_lf("{}\n\r\n\"hello\"", [(1, 1), (3, 1)] )]
+    #[case::cr_lf_lf("{}\r\n\n\"hello\"", [(1, 1), (3, 1)] )]
+    #[case::cr_lf_cr("{}\r\n\r\"hello\"", [(1, 1), (2, 2)] )]
+    #[case::cr_cr_lf("{}\r\r\n\"hello\"", [(1, 1), (2, 1)] )]
+    #[case::cr_cr_cr("{}\r\r\r\"hello\"", [(1, 1), (1, 6)] )]
     #[case::nl_after("{}\"hello\"\n\n",   [(1, 1), (1, 3)])]
     #[case::tabs(    "{}\n\t\t\"hello\"", [(1, 1), (2, 3)] )]
     #[case::tabs_after("{}\"hello\"\t\t", [(1, 1), (1, 3)])]

src/location.rs

@@ -99,40 +99,10 @@ impl SourceLocationState {
     pub fn update_from_source<T: AsRef<[u8]>>(&mut self, stream_offset: usize, data: T) {
         let data = data.as_ref();
         if !data.is_empty() {
-            // Calculate `rows` based on occurrence of newline bytes. If we encounter:
-            // 1. b'\r' then increment row count by 1.
-            // 2.a. If there was no b'\r' encountered before b'\n' then increment row count by 1.
-            // 2.b. If there was no b'\r' encountered before b'\n' then don't increment as b'\r\n' should be counted as 1 based on windows line ending pattern.
-            // Calculate `prev_newline_offset` based on the index/offset value of the last seen newline byte.
-            // Adding 1 to the index because we want to include everything after the newline -
-            // if newline is at index 5, we want to start counting from index 6 (5 + 1).
-            let (_, prev_newline_offsets) = data.iter().enumerate().fold(
-                (false, vec![]),
-                |(follows_cr, mut offset), (i, b)| {
-                    match (b, follows_cr) {
-                        // When there's a '\r', add a row and update the offset as this newline offset value
-                        (b'\r', _) => {
-                            offset.push(stream_offset + i + 1);
-                            (true, offset)
-                        }
-                        // When there's a '\n' not after '\r', add a row and update the offset as this newline offset value
-                        (b'\n', false) => {
-                            offset.push(stream_offset + i + 1);
-                            (false, offset)
-                        }
-                        // When there's '\n' immediately following '\r', update the offset without adding a row
-                        (b'\n', true) => {
-                            offset.pop();
-                            offset.push(stream_offset + i + 1);
-                            (false, offset)
-                        }
-                        _ => (false, offset),
-                    }
-                },
-            );
+            let newlines = memchr::memchr_iter(b'\n', data);
             self.row_start_offsets
                 .borrow_mut()
-                .extend(prev_newline_offsets);
+                .extend(newlines.map(|it| it + stream_offset + 1));
         }
     }