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

zslayton · 2024-08-09T18:46:05Z

This PR:

Adds a new trait, EncodedBinaryValue, with methods for exposing Spans representing the various components of a value's encoding. This trait is implemented by the 1.0 and 1.1 binary value types. These methods allow ion inspect to handle both versions' values uniformly.
Adds peek methods to raw container iterators and MacroExpansion, allowing tools to see the next value without advancing state.
Adds an ExpandedStreamItem enum that surfaces input EExps in addition to their expansion. This allows tools (like inspect to investigate the EExp and then investigate its output values.
Renames ExpandedValueSource::EExp to SingletonEExp. If a LazyValue is backed by an EExp, it's because it was found to always expand to a single value at compile time--that makes it possible to hand out a LazyValue knowing that when it is eventually read there will be exactly one value. Other values can come from an EExp, but are not backed by it. The renaming hopes to make that less fuzzy.
Adds convenience methods for inspecting where a value/expr came from, like is_ephemeral, is_parameter, etc.

It also fixes three bugs for detecting incomplete input:

Text EExp IDs were being parsed using the identifier symbol parser. Unlike a symbol, an EExp's ID cannot be the last thing in a stream.
The float parser correctly rejected input like 123e (no digits after e), but it did it by declaring it invalid rather than incomplete. If the buffer received more data, it's possible retrying would succeed.
Binary 1.1 values were not confirming that the entire value was in the buffer after reading its length.

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-08-09T18:57:46Z

src/lazy/any_encoding.rs

-        let (remaining_data, stream_offset, mut encoding) = match &self.encoding_reader {
-            Text_1_0(r) => r.stream_data(),
-            Binary_1_0(r) => r.stream_data(),
-            Text_1_1(r) => r.stream_data(),
-            Binary_1_1(r) => r.stream_data(),
+        let reader_state = match &self.encoding_reader {
+            Text_1_0(r) => r.save_state(),
+            Binary_1_0(r) => r.save_state(),
+            Text_1_1(r) => r.save_state(),
+            Binary_1_1(r) => r.save_state(),


🗺️ I renamed stream_data to save_state to eliminate the noun/verb ambiguity Matt noticed in the feedback for #796.

It took me a while to find the comment: #796 (comment)

Doc comment would be helpful because it's not immediately obvious whether "stream" is a verb or a noun here.

It's a little ironic because "save state" is nearly as ambiguous as "stream data". Both "save" and "stream" may be adjectives or verbs. "save state" is clearer though, and has the correct implication interpreted either way. I like the change :)

If it's a noun, consider data_stream, saved_state. If it's a verb, consider save_reader_state maybe?

Or even get_reader_state()? (Or, if it accepts owned self, then maybe into_reader_state().)

While the name stream_data was noun/verb ambiguous, I think the actual problem being solved was that the name didn't explain what it did. I think save_state addresses that, even if it remains noun/verb ambiguous.

zslayton · 2024-08-09T18:59:19Z

src/lazy/any_encoding.rs

@@ -1149,10 +1153,12 @@ impl<'top> LazyContainerPrivate<'top, AnyEncoding> for LazyRawAnyList<'top> {
    }
 }

+#[derive(Debug, Copy, Clone)]


🗺️ Lots of structs now implement Copy et al not only because they can/should, but also because it makes implementing peek() possible for raw containers/readers.

zslayton · 2024-08-09T19:15:34Z

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

+        if total_length > input.len() {
+            return IonResult::incomplete(
+                "the stream ended unexpectedly in the middle of a value",
+                header_offset,
+            );
+        }


🗺️ Somehow the test for incomplete binary values never made the leap from 1.0 to 1.1, so here it is.

zslayton · 2024-08-09T19:17:26Z

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

@@ -3,9 +3,12 @@
 use std::fmt::Debug;
 use std::ops::Range;

+use num_traits::PrimInt;


🗺️ cargo fmt moved this from below.

zslayton · 2024-08-09T19:19:20Z

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

@@ -823,3 +825,35 @@ impl<'top> LazyRawBinaryValue_1_1<'top> {
        Ok(LazyRawBinaryStruct_1_1::from_value(self))
    }
 }
+
+impl<'top> EncodedBinaryValue<'top, BinaryEncoding_1_1> for &'top LazyRawBinaryValue_1_1<'top> {


🗺️ The definition of this trait is in the next file down in the PR. For now, just notice that these methods are exposing chunks of the input backing various encoding primitives in the value's bytes.

zslayton · 2024-08-09T21:04:07Z

src/lazy/streaming_raw_reader.rs

+pub struct RawReaderState<'a> {
+    data: &'a [u8],
+    offset: usize,
+    encoding: IonEncoding,
+}


🗺️ I created a type for this instead of passing around a 3-tuple.

zslayton · 2024-08-09T21:13:16Z

src/lazy/text/buffer.rs

+            // Unlike a symbol value with identifier syntax, an e-expression identifier cannot be
+            // the last thing in the stream.
+            return Err(nom::Err::Incomplete(Needed::Unknown));
+        }


🗺️ Fixing an incompleteness detection bug that occurs when the last thing in the buffer is the e-exp ID.

I remember you!

zslayton · 2024-08-09T21:13:53Z

src/lazy/text/buffer.rs

        ))(self)
    }

    /// Like `match_digits_before_dot`, but allows leading zeros.
-    fn match_digits_after_dot(self) -> IonMatchResult<'top> {
+    fn match_one_or_more_digits_after_dot(self) -> IonMatchResult<'top> {


🗺️ Fixing an incompleteness detection bug that occurs when the e in a float is the last thing in the buffer.

zslayton · 2024-08-09T21:14:41Z

src/lazy/text/buffer.rs

+        expect_incomplete: [
+            "5d",
+            "-5d",
+            "5.d",
+            "-5.d",
+            "5.D",
+            "-5.D",
+            "5.1d", "-5.1d",
+            "5.1D", "-5.1D",


🗺️ All of these could be valid decimals if the buffer ends up getting at least one more digit.

zslayton · 2024-08-09T21:17:33Z

src/lazy/text/parse_result.rs

+                let buffer_tail = &input.bytes()[input.len() - tail_bytes_to_take..];
+                let tail = format!("{:X?}", buffer_tail);
+                (head, tail)
+            }


🗺️ These changes make the error message that surfaces for illegal input much more friendly.

Nice, looks good!

jobarr-amzn · 2024-08-12T20:54:48Z

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

+                // Nonsense values for now
+                ion_type_code: OpcodeType::Nop,
+                low_nibble: 0,


For now? Until when?

My intent (not communicated in this comment) was to modify the EncodedValue type such that it wouldn't require a Header anymore. Matt also suggested this independently; I'm going to do it as part of #805.

jobarr-amzn · 2024-08-12T23:21:07Z

src/lazy/expanded/macro_evaluator.rs

+    /// Returns `true` if this expression was produced by evaluating a macro. Otherwise, returns `false`.
+    pub fn is_ephemeral(&self) -> bool {


🚲 🏠 is_ephemeral doesn't say "is the result of evaluating a macro" to me, but I am ignorant of the standard terminology here. Is this phrasing used elsewhere?

I've been using it in ion-rust and ion inspect to describe values that don't appear anywhere in the input stream. They exist only during macro evaluation. I'm open to other ideas.

Can you expand on "don't appear anywhere in the input stream"? An example, perhaps?

Sure!

Here's an example in which the stream contains:

An encoding directive ($ion_encoding::(...)) that defines a macro called triple

An e-expression with a string argument ((:triple "hello"))

When a Reader traverses the stream it will hand out three LazyValues, one for each "hello" in the stream's expansion. However, if you were to ask any of those LazyValues for its Span (backing bytes), you would get None--the values aren't encoded as values the stream itself, they're the ephemeral result of evaluating the e-expression.

Does that help?

Thanks for the example :) Yes that does help. If the triple macro produced only one value then you could say that that is encoded where the macro is invoked (as you describe in the comment below about "singleton macros".

Hmm, I could be inclined to think of those "hello" values as occupying the same 0-byte span between bytes... 73 and 74? But then I'd have to wonder- how do I distinguish the position of the second from the first? If I'm able to distinguish the difference between multiple values in the same place then I might as well say all three values are "in" the macro invocation. Is the answer then that we need another dimension to locate values, that all three "hello" are located between bytes 67-74 at different ordinals? E.g. ([67-74], 0), ([67-74], 1), ([67-74], 2)?

Are these values ephemeral? Virtual? Implicit? Derived? Computed?

Lots of questions. No suggestions :) Works for me.

jobarr-amzn · 2024-08-12T23:41:32Z

src/lazy/expanded/macro_evaluator.rs

-            _ => unreachable!("e-expression-backed lazy values must yield a single value literal"),
+            _ => ice!(IonResult::decoding_error(format!(
+                "expansion of {self:?} was required to produce exactly one value",
+            ))),


What's happening here?

This path is an optimization available to macros that are statically determined to expand to a single non-system value at template-compile time. Consider this macro definition:

(macro Point2D ($x $y) { x: $x, y: $y, } )

When the TemplateCompiler processes this, it notices that the body of the macro is an unannotated struct. This means that it will always evaluate to exactly one value and that value cannot be a symbol table or encoding directive (because those require specific annotations).

The codebase refers to this category of macros as "singleton macros." They offer a few benefits:

Because it will definitely be exactly one value, the reader can hand out a LazyValue holding the e-expression as its backing data. Other macros cannot do this because if you're holding a LazyValue and the macro evaluates to nothing or 100 things, there's not a way for the application to handle those outcomes.

At the top level, singleton macros can be skipped over without evaluation because we know they won't produce a system value that could modify the encoding context.

Expanding a singleton macro doesn't require an evaluator with a stack because as soon as you've gotten a value, you're done--no need to pop() and preserve state.

Now that we're looking at this more closely though, the decoding error should actually be a panic--if it ever happens, it's a bug in the library and not a problem with the data. While I'm in there I'll also add some of the description above to the ExpansionSingleton type.

jobarr-amzn · 2024-08-13T00:35:37Z

src/lazy/system_reader.rs

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


Did these change because of the addition of annotate? I see the assertion on new line 1097 doesn't change. The comment on new line 1095 is now out of date though, yes?

Did these change because of the addition of annotate?

Yep. Matt rightly pointed out that I should use an offset from NUM_SYSTEM_MACROS to prevent future breakage. I'm going to do that for #807.

The comment on new line 1095 is now out of date though, yes?

Yeah, good catch.

jobarr-amzn · 2024-08-13T00:43:43Z

src/lazy/text/buffer.rs

+            // Unlike a symbol value with identifier syntax, an e-expression identifier cannot be
+            // the last thing in the stream.
+            return Err(nom::Err::Incomplete(Needed::Unknown));
+        }


I remember you!

jobarr-amzn · 2024-08-13T00:45:21Z

src/lazy/text/buffer.rs

+            // Note: ^-- We use this `pair(satisfy(...), complete_digit0)` to guarantee a subtle
+            //       behavior. At the end of the buffer, an empty input to this parser must be
+            //       considered 'incomplete' instead of 'invalid'. In contrast, an input of a single
+            //       digit would be considered complete even though the buffer could get more data later.
+            //       (If the buffer gets more data, it's the StreamingRawReader's responsibility to
+            //       discard the `1.1` and try again.)


I very much appreciate comments like these, thanks!

jobarr-amzn · 2024-08-13T00:52:35Z

src/lazy/text/parse_result.rs

+                let buffer_tail = &input.bytes()[input.len() - tail_bytes_to_take..];
+                let tail = format!("{:X?}", buffer_tail);
+                (head, tail)
+            }


Nice, looks good!

zslayton requested a review from popematt August 9, 2024 18:55

zslayton commented Aug 9, 2024

View reviewed changes

zslayton marked this pull request as ready for review August 9, 2024 21:24

zslayton requested a review from nirosys August 9, 2024 21:24

zslayton mentioned this pull request Aug 10, 2024

Adds a 1.1 ValueWriter setting for writing symbol text inline #804

Merged

Base automatically changed from read-flex_uint-annotate to main August 11, 2024 19:10

jobarr-amzn reviewed Aug 12, 2024

View reviewed changes

jobarr-amzn reviewed Aug 13, 2024

View reviewed changes

zslayton requested a review from jobarr-amzn August 13, 2024 15:43

jobarr-amzn approved these changes Aug 13, 2024

View reviewed changes

zslayton added 15 commits August 13, 2024 15:06

Adds EncodedBinaryValue trait

10dfcd4

Rename ExpandedValueSource::EExp to SingletonEExp

6cf797d

Makes raw iterators and MacroExpansion Copy, adds peek_next()

e386d22

Adds RawReaderState type, simplifies singleton eexp annotations

5ffea14

Adds ExpandedStreamItem, is_ephemeral

f33e007

Adds value completeness check for text eexp ID, bin values

d9b4133

improved formatting for InvalidInputError message

eca73b4

No digits after float e is incomplete, improves error fmt

174522f

Improved doc comments

d3e5cc5

Fixes incompleteness check for float/decimal digits after dot

bd495be

clippy suggestion

5134408

comment cleanup

2ee125b

Removes doc link to private type

e5152f3

Add variable attribution to expansion values

4bda375

Improved comments

ddeaa7f

zslayton force-pushed the inspect-1_1 branch from adc1b2e to ddeaa7f Compare August 13, 2024 19:56

zslayton merged commit d4ad0c5 into main Aug 13, 2024
28 of 31 checks passed

zslayton deleted the inspect-1_1 branch August 13, 2024 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zslayton commented Aug 9, 2024 •

edited

Loading

zslayton left a comment

zslayton Aug 9, 2024

jobarr-amzn Aug 12, 2024 •

edited

Loading

popematt Aug 12, 2024 •

edited

Loading

zslayton Aug 13, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

jobarr-amzn Aug 13, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

zslayton Aug 9, 2024

jobarr-amzn Aug 13, 2024

jobarr-amzn Aug 12, 2024

zslayton Aug 13, 2024

jobarr-amzn Aug 12, 2024

zslayton Aug 13, 2024

jobarr-amzn Aug 13, 2024

zslayton Aug 13, 2024

jobarr-amzn Aug 13, 2024

jobarr-amzn Aug 12, 2024

zslayton Aug 13, 2024

jobarr-amzn Aug 13, 2024

zslayton Aug 13, 2024 •

edited

Loading

jobarr-amzn Aug 13, 2024

jobarr-amzn Aug 13, 2024

jobarr-amzn Aug 13, 2024

		/// Returns `true` if this expression was produced by evaluating a macro. Otherwise, returns `false`.
		pub fn is_ephemeral(&self) -> bool {

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

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

Conversation

zslayton commented Aug 9, 2024 • edited Loading

zslayton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jobarr-amzn Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

popematt Aug 12, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zslayton Aug 13, 2024 • edited Loading

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 additional tooling APIs to enable Ion 1.1 support in the CLI's `inspect` command #802

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

zslayton commented Aug 9, 2024 •

edited

Loading

jobarr-amzn Aug 12, 2024 •

edited

Loading

popematt Aug 12, 2024 •

edited

Loading

zslayton Aug 13, 2024 •

edited

Loading