Adds the `(annotate ...)` form and bin read support for `flex_uint` parameters #801

zslayton · 2024-07-30T21:54:35Z

Adds:

An implementation of the (annotate) TDL form
Binary read support for flex_uint::-encoded template parameters
Very limited binary write support for flex_uint::-encoded template parameters to enable unit testing for the reader.

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

zslayton

🗺️ PR Tour 🧭

zslayton · 2024-07-30T21:55:37Z

src/ion_data/ion_eq.rs

@@ -24,7 +24,7 @@ use std::ops::Deref;
 /// * Decimal `0.0` and `-0.0` are mathematically equivalent but not Ion equivalent.
 /// * Decimal `0.0` and `0.00` are mathematically equivalent but not Ion equivalent.
 /// * Timestamps representing the same point in time at different precisions or at different
-/// timezone offsets are not Ion equivalent.
+///   timezone offsets are not Ion equivalent.


🗺️ Rust v1.80.0 added some Clippy warnings around doc comment markdown formatting.

zslayton · 2024-07-30T21:56:27Z

src/lazy/binary/encoded_value.rs

@@ -88,6 +90,8 @@ pub(crate) struct EncodedValue<HeaderType: EncodedHeader> {
    pub annotations_encoding: AnnotationsEncoding,
    // The offset of the type descriptor byte within the overall input stream.
    pub header_offset: usize,
+    // If this value was written with a tagless encoding, this will be 0. Otherwise, it's 1.
+    pub opcode_length: u8,


🗺️ Retrofitting support for encodings other than Tagged in EncodedValue.

Can this be derived (i.e. fn is_tagless() -> bool) rather than storing the opcode length in a field? Tagless encodings must have value_body_length + length_length == total_length, and anything else must have an opcode, so value_body_length + length_length < total_length is always true for anything that is not a tagless encoded argument. Right?

Yeah, I had the same thought mid-PR. I've opened #805 to track refactoring this.

zslayton · 2024-07-30T21:57:54Z

src/lazy/binary/raw/v1_1/immutable_buffer.rs

+        // XXX: This *doesn't* slice `self` because FlexUInt::read() is faster if the input
+        //      is at least the size of a u64.
+        let matched_input = self;
+        let remaining_input = self.slice_to_end(size_in_bytes);


🗺️ I have a fix for this waiting in the PR that follows this one.

zslayton · 2024-07-30T21:59:16Z

src/lazy/encoder/binary/v1_1/value_writer.rs

-        }
+        };


🗺️ cargo fmt moved some things around in this file.

zslayton · 2024-07-30T21:59:56Z

src/lazy/expanded/compiler.rs

+    /// Confirms that the provided `value` is a symbol with known text. If so, returns `Ok(text)`.
+    /// If not, returns a decoding error containing the specified label.
+    fn expect_symbol<'a, Encoding: Decoder>(


🗺️ I added some helper methods in the TemplateCompiler to make its main path a bit easier to read.

zslayton · 2024-07-30T22:03:06Z

src/lazy/expanded/e_expression.rs

-    EExpArgGroupIterator, EExpressionArgGroup, MacroExpansion, MacroExpansionKind, MacroExpr,
-    MacroExprArgsIterator, MakeStringExpansion, RawEExpression, TemplateExpansion, ValueExpr,
-    ValuesExpansion,
+    AnnotateExpansion, EExpArgGroupIterator, EExpressionArgGroup, MacroExpansion,


🗺️ Adding AnnotateExpansion to this caused a reflow.

zslayton · 2024-07-30T22:05:18Z

src/types/integer.rs

@@ -9,7 +9,7 @@ use std::mem;
 use std::ops::{Add, Neg};

 /// Represents an unsigned integer of any size.
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]


🗺️ Starting in v1.80, Clippy correctly identified that a lot of our manual implementations could be derived.

popematt · 2024-08-11T04:14:59Z

src/lazy/binary/encoded_value.rs

@@ -88,6 +90,8 @@ pub(crate) struct EncodedValue<HeaderType: EncodedHeader> {
    pub annotations_encoding: AnnotationsEncoding,
    // The offset of the type descriptor byte within the overall input stream.
    pub header_offset: usize,
+    // If this value was written with a tagless encoding, this will be 0. Otherwise, it's 1.
+    pub opcode_length: u8,


Can this be derived (i.e. fn is_tagless() -> bool) rather than storing the opcode length in a field? Tagless encodings must have value_body_length + length_length == total_length, and anything else must have an opcode, so value_body_length + length_length < total_length is always true for anything that is not a tagless encoded argument. Right?

popematt · 2024-08-11T04:20:08Z

src/lazy/binary/raw/v1_1/value.rs

+    pub(crate) fn for_flex_uint(input: ImmutableBuffer<'top>) -> Self {
+        let encoded_value = EncodedValue {
+            encoding: ParameterEncoding::FlexUInt,
+            header: Header {


Perhaps Header should actually be an enum so that it could have a NoHeader variant in addition to whatever other implementations you have going on?

I'll look into this as part of #805.

popematt · 2024-08-11T04:30:20Z

src/lazy/expanded/compiler.rs

@@ -236,6 +255,21 @@ impl TemplateCompiler {
        }
    }

+    fn encoding_for<Encoding: Decoder>(value: LazyValue<Encoding>) -> IonResult<ParameterEncoding> {


Should this check to make sure that there isn't a second annotation?

Yes it should. Tracking this in #806 so I can return to it when my existing PR queue merged.

popematt · 2024-08-11T04:35:47Z

src/lazy/expanded/macro_evaluator.rs

@@ -1091,6 +1202,25 @@ mod tests {
        )
    }

+    #[test]
+    fn annotate() -> IonResult<()> {


Might be a good idea to have another test case where you invoke annotate as an e-expression rather than as a TDL macro invocation. (Just in case there is a regression, it would (hopefully) make it easier to isolate the problem.)

Tracking in #808.

popematt · 2024-08-11T04:40:06Z

src/lazy/system_reader.rs

@@ -1087,11 +1087,11 @@ mod tests {
        // This directive defines two more.
        assert_eq!(new_macro_table.len(), 2 + MacroTable::NUM_SYSTEM_MACROS);
        assert_eq!(
-            new_macro_table.macro_with_id(3),
+            new_macro_table.macro_with_id(4),


Would this work?

Suggested change

new_macro_table.macro_with_id(4),

new_macro_table.macro_with_id(MacroTable::NUM_SYSTEM_MACROS),

Tracking in #807.

popematt · 2024-08-11T04:40:27Z

src/lazy/system_reader.rs

            new_macro_table.macro_with_name("seventeen")
        );
        assert_eq!(
-            new_macro_table.macro_with_id(4),
+            new_macro_table.macro_with_id(5),


And this?

Suggested change

new_macro_table.macro_with_id(5),

new_macro_table.macro_with_id(MacroTable::NUM_SYSTEM_MACROS + 1),

Tracking in #807.

zslayton

I have several open PRs building on one another, so I've opened tracking issues to incorporate your feedback after I get them all merged.

zslayton · 2024-08-11T16:51:15Z

src/lazy/binary/encoded_value.rs

@@ -88,6 +90,8 @@ pub(crate) struct EncodedValue<HeaderType: EncodedHeader> {
    pub annotations_encoding: AnnotationsEncoding,
    // The offset of the type descriptor byte within the overall input stream.
    pub header_offset: usize,
+    // If this value was written with a tagless encoding, this will be 0. Otherwise, it's 1.
+    pub opcode_length: u8,


Yeah, I had the same thought mid-PR. I've opened #805 to track refactoring this.

zslayton · 2024-08-11T16:52:10Z

src/lazy/binary/raw/v1_1/value.rs

+    pub(crate) fn for_flex_uint(input: ImmutableBuffer<'top>) -> Self {
+        let encoded_value = EncodedValue {
+            encoding: ParameterEncoding::FlexUInt,
+            header: Header {


I'll look into this as part of #805.

zslayton · 2024-08-11T16:56:08Z

src/lazy/expanded/compiler.rs

@@ -236,6 +255,21 @@ impl TemplateCompiler {
        }
    }

+    fn encoding_for<Encoding: Decoder>(value: LazyValue<Encoding>) -> IonResult<ParameterEncoding> {


Yes it should. Tracking this in #806 so I can return to it when my existing PR queue merged.

zslayton · 2024-08-11T17:00:20Z

src/lazy/system_reader.rs

@@ -1087,11 +1087,11 @@ mod tests {
        // This directive defines two more.
        assert_eq!(new_macro_table.len(), 2 + MacroTable::NUM_SYSTEM_MACROS);
        assert_eq!(
-            new_macro_table.macro_with_id(3),
+            new_macro_table.macro_with_id(4),


Tracking in #807.

zslayton · 2024-08-11T17:00:39Z

src/lazy/system_reader.rs

            new_macro_table.macro_with_name("seventeen")
        );
        assert_eq!(
-            new_macro_table.macro_with_id(4),
+            new_macro_table.macro_with_id(5),


Tracking in #807.

zslayton · 2024-08-11T17:01:33Z

src/lazy/expanded/macro_evaluator.rs

@@ -1091,6 +1202,25 @@ mod tests {
        )
    }

+    #[test]
+    fn annotate() -> IonResult<()> {


Tracking in #808.

zslayton commented Jul 30, 2024

View reviewed changes

zslayton marked this pull request as ready for review July 30, 2024 22:05

zslayton requested review from nirosys and popematt July 30, 2024 22:06

Base automatically changed from encoding-directives to main July 31, 2024 00:56

Adds binary 1.1 read support for flex_uint, (annotate)

05d9217

zslayton force-pushed the read-flex_uint-annotate branch from a79d691 to 05d9217 Compare August 9, 2024 14:34

unit test tweak

7d14ea0

popematt reviewed Aug 11, 2024

View reviewed changes

zslayton commented Aug 11, 2024

View reviewed changes

popematt approved these changes Aug 11, 2024

View reviewed changes

zslayton merged commit e436f96 into main Aug 11, 2024
29 of 32 checks passed

zslayton deleted the read-flex_uint-annotate branch August 11, 2024 19:10

zslayton mentioned this pull request Aug 13, 2024

Adds additional tooling APIs to enable Ion 1.1 support in the CLI's inspect command #802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the `(annotate ...)` form and bin read support for `flex_uint` parameters #801

Adds the `(annotate ...)` form and bin read support for `flex_uint` parameters #801

zslayton commented Jul 30, 2024 •

edited

Loading

zslayton left a comment

zslayton Jul 30, 2024

zslayton Jul 30, 2024

popematt Aug 11, 2024

zslayton Aug 11, 2024

zslayton Jul 30, 2024

zslayton Jul 30, 2024

zslayton Jul 30, 2024

zslayton Jul 30, 2024

zslayton Jul 30, 2024

popematt Aug 11, 2024

popematt Aug 11, 2024

zslayton Aug 11, 2024

popematt Aug 11, 2024

zslayton Aug 11, 2024

popematt Aug 11, 2024

zslayton Aug 11, 2024

popematt Aug 11, 2024

zslayton Aug 11, 2024

popematt Aug 11, 2024

zslayton Aug 11, 2024

zslayton left a comment

zslayton Aug 11, 2024

zslayton Aug 11, 2024

zslayton Aug 11, 2024

zslayton Aug 11, 2024

zslayton Aug 11, 2024

zslayton Aug 11, 2024

	new_macro_table.macro_with_id(4),
	new_macro_table.macro_with_id(MacroTable::NUM_SYSTEM_MACROS),

	new_macro_table.macro_with_id(5),
	new_macro_table.macro_with_id(MacroTable::NUM_SYSTEM_MACROS + 1),

Adds the (annotate ...) form and bin read support for flex_uint parameters #801

Adds the (annotate ...) form and bin read support for flex_uint parameters #801

Conversation

zslayton commented Jul 30, 2024 • edited Loading

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

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 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

Adds the `(annotate ...)` form and bin read support for `flex_uint` parameters #801

Adds the `(annotate ...)` form and bin read support for `flex_uint` parameters #801

zslayton commented Jul 30, 2024 •

edited

Loading