Adds APIs for accessing encoding of raw stream items #760

zslayton · 2024-05-04T14:45:25Z

In aggregate, the changes in this PR make it straightforward to access a "lower level" view of a given item in an Ion stream, determining whether it was an input literal, part of a template, or a constructed value. This enables the development of a variety of tools.

Changes:

Uses the bump allocator to handle text escape processing, allowing RawSymbolTokenRef to hold a reference to a &'bump str instead of potentially owning a String. This change allows the RawSymbolTokenRef type to implement Copy, which in turn allows all of the LazyExpandedValue- and LazyValue-related types to also implement Copy.
Removes the RawSymbolToken type, which is now redundant to the RawSymbolTokenRef type.
Adds a Span type that provides access to the input bytes that comprised various raw stream items.
Adds a LazyRawVersionMarker trait and per-encoding impls that can provide a Span upon request.
Moves the field encoding information from the LazyDecoder::Value types into implementations of a new LazyRawFieldName trait. This fixes issue Explore moving struct field info out of Lazy*Value and into Lazy*Field #631.
Adds a LazyRawField trait and per-encoding impls that can provide a Span upon request.
Adds an UnexpandedField type that can represent both raw struct fields and struct fields from a template body. This simplified the code for expanding structs.
Adds methods to convert container types back to the general value type.
Adds EncodedBinaryValueData_1_0 and EncodedBinaryAnnotations_1_0 types that can be used to access spans and ranges for the various components of a binary 1.0 value.

This patch exposes many functions and types which we likely wish to feature gate, but that change is being left for a future PR.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

* Uses the bump allocator to handle text escape processing, allowing `RawSymbolTokenRef` to hold a reference to a `&'bump str` instead of potentially owning a `String`. This change allows the `RawSymbolTokenRef` type to implement `Copy`, which in turn allows all of the `LazyExpandedValue`- and `LazyValue`-related types to also implement `Copy`. * Removes the `RawSymbolToken` type, which is now redundant to the `RawSymbolTokenRef` type. * Adds a `Span` type that provides access to the input bytes that comprised various raw stream items. * Adds a `LazyRawVersionMarker` trait and per-encoding impls that can provide a `Span` upon request. * Adds a `LazyRawField` trait and per-encoding impls that can provide a `Span` upon request. * Adds an `UnexpandedField` type that can represent both raw struct fields and struct fields from a template body. This simplified the code for expanding structs. * Adds methods to convert container types back to the general value type. * Adds `EncodedBinaryValueData_1_0` and `EncodedBinaryAnnotations_1_0` types that can be used to access spans and ranges for the various components of a binary 1.0 value. This patch exposes many functions and types which we likely wish to feature gate, but that change is being left for a future PR.

zslayton

🗺️ PR tour

zslayton · 2024-05-04T15:00:18Z

examples/write_log_events.rs

-                .write(15, RawSymbolToken::SymbolId(19))? // format
+                .write(13, RawSymbolTokenRef::SymbolId(17))? // logger name
+                .write(14, RawSymbolTokenRef::SymbolId(18))? // log level
+                .write(15, RawSymbolTokenRef::SymbolId(19))? // format


🗺️ Now that RawSymbolTokenRef can hold a reference to a decoded string in the bump allocator, there's no longer a need for a separate RawSymbolToken type that can own a string.

Down the line, I'd like to rename RawSymbolTokenRef to simply RawSymbol.

zslayton · 2024-05-04T15:02:01Z

src/lazy/any_encoding.rs

+pub enum LazyRawAnyVersionMarkerKind<'top> {
+    Text_1_0(LazyRawTextVersionMarker_1_0<'top>),
+    Binary_1_0(LazyRawBinaryVersionMarker_1_0<'top>),
+    Text_1_1(LazyRawTextVersionMarker_1_1<'top>),
+    Binary_1_1(LazyRawBinaryVersionMarker_1_1<'top>),
+}


🗺️ There's now a per-encoding version marker type that can provide a Span of its input bytes. Previously we only surfaced the (major: u8, minor: u8) pair, but didn't have information about its encoding (particularly its location).

zslayton · 2024-05-04T15:03:04Z

src/lazy/any_encoding.rs

+        match self.encoding {
+            Text_1_0(marker) => marker.span(),
+            Binary_1_0(marker) => marker.span(),
+            Text_1_1(marker) => marker.span(),
+            Binary_1_1(marker) => marker.span(),
+        }


🗺️ This file (LazyRawAny_____ types) is entirely enum dispatch. A while ago I looked to see if I could use the enum_dispatch crate to eliminate all the repetition, but the use of lifetimes was an obstacle. I have not checked recently to see if that limitation has been overcome.

zslayton · 2024-05-04T15:04:01Z

src/lazy/any_encoding.rs

+    pub fn kind(&self) -> LazyRawValueKind<'top> {
+        self.encoding
+    }
+}


🗺️ Allows an application to access the encoding-specific data type that underlies this value.

zslayton · 2024-05-04T15:09:41Z

src/lazy/any_encoding.rs

@@ -1211,7 +1412,7 @@ mod tests {

            assert!(matches!(
                reader.next(&allocator)?,
-                LazyRawStreamItem::<AnyEncoding>::EndOfStream
+                LazyRawStreamItem::<AnyEncoding>::EndOfStream(_)


🗺️ The EndOfStream item now returns an EndPosition that can provide its range and span. This is not especially useful in and of itself, but it allows the RawStreamItem type that contains it to also provide a range and span for every item.

zslayton · 2024-05-04T15:34:09Z

src/lazy/encoder/text/v1_0/value_writer.rs

-fn token_is_identifier(token: &str) -> bool {
-    if token.is_empty() {
-        return false;
-    }
-    let mut chars = token.chars();
-    let first = chars.next().unwrap();
-    (first == '$' || first == '_' || first.is_ascii_alphabetic())
-        && chars.all(|c| c == '$' || c == '_' || c.is_ascii_alphanumeric())
-}


🗺️ The methods in this section were originally copied from the IonValueFormatter. This PR introduces the IoFmtShim type that allows them to be re-used instead of copied.

zslayton · 2024-05-04T15:36:34Z

src/lazy/expanded/struct.rs

-                let mut iter_adapter = raw_struct_iter.map(
-                    |field: IonResult<LazyRawFieldExpr<'top, D>>| match field? {
-                        RawFieldExpr::NameValuePair(name, RawValueExpr::MacroInvocation(m)) => {
-                            let resolved_invocation = m.resolve(context)?;
-                            Ok(RawFieldExpr::NameValuePair(
-                                name,
-                                RawValueExpr::MacroInvocation(resolved_invocation.into()),
-                            ))
-                        }
-                        RawFieldExpr::NameValuePair(name, RawValueExpr::ValueLiteral(value)) => Ok(
-                            RawFieldExpr::NameValuePair(name, RawValueExpr::ValueLiteral(value)),
-                        ),
-                        RawFieldExpr::MacroInvocation(invocation) => {
-                            let resolved_invocation = invocation.resolve(context)?;
-                            Ok(RawFieldExpr::MacroInvocation(resolved_invocation.into()))
-                        }
-                    },
-                );
-                Self::next_field_from(context, state, e_exp_evaluator, &mut iter_adapter)
+                Self::next_field_from(context, state, e_exp_evaluator, raw_struct_iter)


🗺️ The introduction of the UnexpandedField enum made it possible to eliminate a lot of the complexity and generics involved in struct expansion.

zslayton · 2024-05-04T15:38:37Z

src/lazy/text/buffer.rs

-/// This helper function takes a parser and returns a closure that performs the same parsing
-/// but prints the Result before returning the output. This is handy for debugging.
-// A better implementation would use a macro to auto-generate the label from the file name and
-// line number.
-fn dbg_parse<I: Debug, O: Debug, E: Debug, P: Parser<I, O, E>>(
-    label: &'static str,
-    mut parser: P,
-) -> impl Parser<I, O, E> {
-    move |input: I| {
-        let result = parser.parse(input);
-        #[cfg(debug_assertions)]
-        println!("{}: {:?}", label, result);
-        result
-    }
-}
-


🗺️ This method would cause a clippy warning in release builds because label wouldn't be used. The text reader is spec compliant, so I feel we don't really need this debug helper anymore.

zslayton · 2024-05-04T15:39:10Z

src/lazy/text/buffer.rs

-                // TODO: We're discarding the name encoding information here. When we revise our field name
-                //       storage strategy[1], we should make sure to capture this for tooling's sake.
-                //       [1]: https://github.com/amazon-ion/ion-rust/issues/631


🗺️ 🎉

zslayton · 2024-05-04T15:40:59Z

src/lazy/text/matched.rs

-    pub fn read<'data>(&self, matched_input: TextBufferView<'data>) -> IonResult<StrRef<'data>> {
+    pub fn read<'data>(
+        &self,
+        allocator: &'data BumpAllocator,


🗺️ Text escapes and base64 decoding now happen in bump-allocated scratch space instead of heap-allocated Strings and Vecs.

src/lazy/any_encoding.rs

src/lazy/binary/encoded_value.rs

src/lazy/binary/immutable_buffer.rs

src/lazy/binary/raw/struct.rs

src/lazy/struct.rs

src/lazy/span.rs

popematt · 2024-05-06T21:19:38Z

src/lazy/text/buffer.rs

+    /// Returns the stream byte offset range that this buffer contains.
+    pub fn range(&self) -> Range<usize> {
+        self.offset..self.offset + self.len()
+    }


Should TextBufferView just implement HasSpan/HasRange?

It could, sure. This method predates those traits.

TextBufferView is not a public-facing type though, so there's not really a need for it IMO.

src/lazy/text/encoded_value.rs

src/lazy/value_ref.rs

src/lazy/raw_stream_item.rs

Zack Slayton added 3 commits May 4, 2024 10:28

cargo clippy

ac6ca00

cargo fmt

434281e

zslayton commented May 4, 2024

View reviewed changes

zslayton requested review from popematt and nirosys May 4, 2024 15:41

zslayton marked this pull request as ready for review May 4, 2024 15:42

popematt reviewed May 6, 2024

View reviewed changes

zslayton mentioned this pull request May 7, 2024

Should ValueRef container types implement PartialEq? #761

Open

Incorporates feedback

cc684f5

zslayton requested a review from popematt May 7, 2024 14:28

popematt approved these changes May 7, 2024

View reviewed changes

src/lazy/raw_stream_item.rs Outdated Show resolved Hide resolved

Remove broken doc link

ef44046

zslayton merged commit 892ef2d into main May 7, 2024
32 checks passed

zslayton deleted the tooling-access branch May 7, 2024 16:46

zslayton mentioned this pull request May 7, 2024

Text reader implementation cleanup #763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds APIs for accessing encoding of raw stream items #760

Adds APIs for accessing encoding of raw stream items #760

zslayton commented May 4, 2024 •

edited

zslayton left a comment

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

zslayton May 4, 2024

popematt May 6, 2024

zslayton May 7, 2024

Adds APIs for accessing encoding of raw stream items #760

Adds APIs for accessing encoding of raw stream items #760

Conversation

zslayton commented May 4, 2024 • edited

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton commented May 4, 2024 •

edited