Modernizes the `ElementWriter` trait #487

zslayton · 2023-03-17T14:07:56Z

This PR has a weird commit history that was caused by needing to be rebased a few times in the wake of other (sweeping) changes getting merged. The diff is correct, however, so it can be safely ignored for the purposes of review. It'll get squashed at merge time anyway.

Changes:

ElementWriter is now automatically implemented by all IonWriter implementations, allowing IonWriters to write an Element to output without creating an instance of an intermediate type (e.g. NativeElementWriter).
Removed NativeElementWriter.
Adds the Element::value() method to get a reference to its Value. This allows matching on the Value directly instead of matching on its IonType and then calling the appropriate as_TYPE().unwrap() method.
Added output() and output_mut() to the IonWriter trait, giving users a way to see/manipulate their (eg) output buffer while the writer still exists. Prior to this, calling drop(writer) to unfreeze the output buffer was common.
Fixes a bug in RawBinaryWriter::write_f32 that caused negative zero to be serialized as positive zero.
Renames IonSequence::iter() to IonSequence::elements().
Several doc comments included code fences (triple backticks) containing example output (ie not Rust). Previously, those code fences were marked with ignore to keep them from being treated as doc tests; this caused them to be listed in the cargo test output as IGNORED. Now they are marked text, indicating that they are not test code at all.

Fixes #202.

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

codecov · 2023-03-17T14:09:39Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (dba18aa) 89.47% compared to head (84fd7fb) 89.47%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #487   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files          77       76    -1     
  Lines       13711    13704    -7     
=======================================
- Hits        12268    12262    -6     
+ Misses       1443     1442    -1

Impacted Files	Coverage Δ
src/binary/binary_writer.rs	`81.95% <ø> (ø)`
src/element/element_stream_reader.rs	`86.50% <ø> (ø)`
src/text/parse_result.rs	`84.90% <ø> (ø)`
src/binary/raw_binary_writer.rs	`94.77% <100.00%> (ø)`
src/element/mod.rs	`90.76% <100.00%> (ø)`
src/element/owned.rs	`90.70% <100.00%> (+0.03%)`	⬆️
src/element/writer.rs	`92.45% <100.00%> (+29.95%)`	⬆️
src/ion_hash/representation.rs	`99.06% <100.00%> (ø)`
src/text/raw_text_writer.rs	`96.17% <100.00%> (+0.01%)`	⬆️
src/text/text_formatter.rs	`58.72% <100.00%> (ø)`
... and 2 more

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

zslayton

🗺️ PR tour

zslayton · 2023-03-17T14:19:04Z

src/binary/binary_writer.rs

+            fn output(&self) -> &Self::Output;
+            fn output_mut(&mut self) -> &mut Self::Output;


🗺️ All IonWriters can now provide a handle to their underlying data sink.

For comparison, this is done in a variety of stdlib types:

io::Cursor has get_ref() and get_mut()

BufReader has get_ref() and get_mut()

BufWriter

Chain (for chaining Read implementations)

etc

While get_ref() and get_mut() are common names, I found output() and output_mut() to be more descriptive. As you'll see in later examples, they make for more obvious code.

zslayton · 2023-03-17T14:22:04Z

src/binary/raw_binary_writer.rs

@@ -638,7 +640,7 @@ impl<W: Write> IonWriter for RawBinaryWriter<W> {
    /// Writes an Ion float with the specified value.
    fn write_f32(&mut self, value: f32) -> IonResult<()> {
        self.write_scalar(|enc_buffer| {
-            if value == 0f32 {
+            if value == 0f32 && !value.is_sign_negative() {


🗺️ Fixes a hitherto undiscovered bug: because zero and negative zero are mathematically equal (that is: 0.0f32 == -0.0f32), this branch would cause negative zero to be serialized as positive zero (0x40). write_f64 (just below) did not have this problem.

zslayton · 2023-03-17T14:22:35Z

src/element/element_stream_reader.rs

@@ -110,7 +110,7 @@ impl ElementStreamReader {
                value
                    .as_sequence()
                    .unwrap()
-                    .iter()
+                    .elements()


🗺️ IonSequence::iter() has been renamed IonSequence::elements() to be more self-describing.

Does the loss of iter() in IonSequence make any difference w.r.t. being able to use it in a for loop?

Good question. The short answer is no, what was previously:

for element in sequence.iter() {...}

is now

for element in sequence.elements() {...}

I still intend to impl IntoIterator for &List and for &SExp, which will enable the more desirable:

for element in &sequence {...}

zslayton · 2023-03-17T14:24:22Z

src/element/native_writer.rs

🗺️ The old ElementWriter trait had two implementations: IonCElementWriter and NativeElementWriter. IonCElementWriter was removed around v0.14.0, leaving only NativeElementWriter. It is now superfluous and has been removed.

zslayton · 2023-03-17T14:32:53Z

src/element/owned.rs

+    /// ```
+    pub fn value(&self) -> &Value {
+        &self.value
+    }


🗺️ Exposing the Element's underlying Value enum transforms code like this:

let element: Element = ...; if element.is_null() { // `is_null` matches on inner `value` return handle_null(element.ion_type()); } use IonType::*; match element.ion_type() { // `ion_type()` matches on inner `value` Null => unreachable!("we already handled null above :c"), Bool => handle_bool(element.as_bool().unwrap()), // Matches on inner `value` Int => handle_int(element.as_int().unwrap()), // Matches on inner `value` Float => handle_int(element.as_float()).unwrap()), // Matches on inner `value` // ...etc };

into:

let element: Element = ...; use Value::*; match element.value() { Null(ion_type) => handle_null(ion_type), // Consolidated null handling Bool(b) => handle_bool(b), // No need to call as_bool(), etc Int(i) => handle_int(i), Float(f) => handle_int(f), // ...etc };

which is much less noisy and eliminates a number of redundant match operations.

🎉 🎉 🎉

zslayton · 2023-03-17T14:37:16Z

src/text/parse_result.rs

@@ -51,7 +51,7 @@ pub(crate) type IonParseResult<'a, O> = IResult<&'a str, O, IonParseError<'a>>;
 /// with other parsers that return `IonParseResult`, you can call `upgrade()` on your parser's
 /// output before returning it.
 ///
-/// ```ignore
+/// ```ignore // Doc tests cannot use internal APIs


🗺️ This doc comment's code fence does contain Rust code, so ignore is appropriate, but it also uses crate-visible methods and so will not compile.

It seems like a bad idea to have doc comment code that uses private/pub(crate) functions. Aren't you basically publishing an example that the reader can never use?

This is a private module, so it isn't exposed in the user-facing documentation.

Any doc comment that includes a code fence is treated as code that should compile using the public API; I'm unsure how to convince cargo to compile it with pub(crate) APIs. If you know of a way, let me know!

This is a private module, so it isn't exposed in the user-facing documentation.

Okay.

Any doc comment that includes a code fence is treated as code that should compile using the public API; I'm unsure how to convince cargo to compile it with pub(crate) APIs. If you know of a way, let me know!

I spent way too long trying to figure out even a hacky workaround (I thought I might be able to do something with #[cfg(doctest)]). Alas, it seems to be impossible—and I must apologize for ever doubting you. 😉

zslayton · 2023-03-17T14:38:04Z

src/text/parse_result.rs

-///         tag(" ")                      <-- Parser 2 matches a single space
+///     let parser = terminated(          // <-- Takes two parsers; if both succeed, returns the output of the first
+///         tag("true").or(tag("false")), // <-- Parser 1 matches the text `true` or the text `false`
+///         tag(" ")                      // <-- Parser 2 matches a single space


🗺️ Hazard of using ignore: the Rust code was not being compiled, so no one noticed that these lines should've been comments.

zslayton · 2023-03-17T14:39:11Z

src/text/raw_text_writer.rs

@@ -20,7 +20,7 @@ impl RawTextWriterBuilder {
    /// Constructs a text Ion writer with modest (but not strictly minimal) spacing.
    ///
    /// For example:
-    /// ```ignore
+    /// ```text


🗺️ All of these code fences contain example text Ion output, not Rust code. Tagging them with ignore caused them to be listed as IGNORED in the rust test --doc output, which was misleading. Tagging them with text causes them to not be treated as example code, which is more appropriate.

zslayton · 2023-03-17T14:39:53Z

src/text/raw_text_writer.rs

-        drop(writer);
-        assert_eq!(str::from_utf8(&output).unwrap(), expected);
+        assert_eq!(
+            str::from_utf8(writer.output().as_slice()).unwrap(),
+            expected
+        );


🗺️ Note that we no longer need to drop(writer) in order to be able to access its output.

zslayton · 2023-03-17T14:41:21Z

tests/element_test_vectors.rs

@@ -104,7 +101,7 @@ trait ElementApi {
        for (index, (element1, element2)) in e1.iter().zip(e2.iter()).enumerate() {
            if !element1.ion_eq(element2) {
                return format!(
-                    "The values at position #{index} were not IonEq.\ne1: {element1:?}\ne2: {element2:?}"
+                    "The values at position #{index} were not IonEq.\ne1: {element1}\ne2: {element2}"


🗺️ The Debug impl for Element is really verbose, which makes it unhelpful for comparing actual/expected. Element has implemented Display for a while now (which just shows the text Ion serialization of the Element) and is more useful.

jobarr-amzn · 2023-03-17T16:54:32Z

src/element/owned.rs

+    /// ```
+    pub fn value(&self) -> &Value {
+        &self.value
+    }


🎉 🎉 🎉

jobarr-amzn · 2023-03-17T16:55:59Z

src/element/writer.rs

+where
+    W: IonWriter,
+{
+    fn write_element(&mut self, element: &Element) -> IonResult<()> {


jobarr-amzn · 2023-03-17T16:57:17Z

src/element/writer.rs

+            Value::SExp(s) => {
+                self.step_in(IonType::SExp)?;
+                self.write_elements(s.elements())?;
+                self.step_out()
+            }


This and the other sequence cases make me wish for a more closed form, but I don't know what that would look like yet.

Could you elaborate on what you mean by "closed form?"

I considered extending ElementWriter to also have write_struct(&Struct), write_list(&List), and write_sext(&SExp). Would that address it?

jobarr-amzn · 2023-03-17T17:02:49Z

src/element/writer.rs

+        let mut writer = TextWriterBuilder::new().build(&mut buffer)?;
+
+        let ion = r#"
+            null true 0 1e0 2.0 2022T foo "bar" (foo bar baz) [foo, bar, baz] {foo: true, bar: false}
+        "#;
+        let expected_elements = Element::read_all(ion.as_bytes())?;


Would the semantics of the test be changed by using a builder macro for expected_elements? I feel like that would be an improvement but perhaps I'm missing something.

Separately, this is read text, write text, read text. Is there a version of this that round trips binary or round trips across formats, e.g. read text, write binary, then read binary?

We could switch it to a builder; this test was written before that was an option.

Separately, this is read text, write text, read text. Is there a version of this that round trips binary or round trips across formats, e.g. read text, write binary, then read binary?

Yes, this code is exercised really thoroughly (cross product of all binary/text reader/writer pairs) in element_test_vectors.rs.

jobarr-amzn · 2023-03-17T17:04:45Z

src/text/raw_text_writer.rs

+    fn output(&self) -> &W {
+        self.output.get_ref()
+    }
+
+    fn output_mut(&mut self) -> &mut W {
+        self.output.get_mut()
+    }


This looks like a move- I see the doc comments in the deleted code, are they preserved elsewhere?

EDIT: Nevermind I see much more detailed commentary added in writer.rs.

jobarr-amzn · 2023-03-17T17:06:56Z

tests/element_test_vectors.rs

@@ -104,7 +101,7 @@ trait ElementApi {
        for (index, (element1, element2)) in e1.iter().zip(e2.iter()).enumerate() {
            if !element1.ion_eq(element2) {
                return format!(
-                    "The values at position #{index} were not IonEq.\ne1: {element1:?}\ne2: {element2:?}"
+                    "The values at position #{index} were not IonEq.\ne1: {element1}\ne2: {element2}"


jobarr-amzn · 2023-03-17T17:10:55Z

tests/ion_hash_tests.rs

-    let elems = Element::read_all(&data)?;
+    let elems = Element::read_all(data)?;


What's going on here?

clippy noticed that data (which was already a reference) was being turned into a doubly-indirect reference unnecessarily. This change was unrelated to the core purpose of the PR.

zslayton added 23 commits March 15, 2023 16:44

Adds ElementReader trait

ab997dc

Manual fixes for missed renames

6f58178

Merge remote-tracking branch 'origin/main' into element-reader

164af4f

Removed old comment

6ea5b68

links to GitHub issue 484

4760f6f

one more link to GitHub issue 484

2fbde84

formatting

dbec271

undoing IDE renames

8f5eefe

Renamed crate::value to crate::element

5543e8b

Rename IonType::SExpression to IonType::SExp

4ff980f

Rename IonType::Boolean to IonType::Bool

17f145e

Adds Display impl for List,SExp,Struct,Value

52c1f2b

IonType Display uses official type names

95e8b4f

Rename Integer to Int, UInteger to UInt

85f7cfb

clippy suggestions in ion_hash_tests

057527b

wip

1e16936

Merge branch 'main' into element-writer

7e16638

Initial working version

dfbce88

Remove native-writer module

7ce831b

Added output()/output_mut() methods to IonWriter

4e36fe8

Switches ignore tags to text on non-test rustdoc comments

be06237

doc comment for IonWriter::Output

f60157f

doc comment for Element::value()

84fd7fb

zslayton commented Mar 17, 2023

View reviewed changes

zslayton requested review from jobarr-amzn, popematt and desaikd March 17, 2023 14:43

jobarr-amzn approved these changes Mar 17, 2023

View reviewed changes

zslayton merged commit cffa745 into main Mar 17, 2023

zslayton deleted the element-writer branch March 17, 2023 18:10

zslayton mentioned this pull request Mar 17, 2023

Ion Element/Value API #114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernizes the `ElementWriter` trait #487

Modernizes the `ElementWriter` trait #487

zslayton commented Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 17, 2023

zslayton left a comment

zslayton Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

popematt Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

jobarr-amzn Mar 17, 2023

zslayton Mar 17, 2023

popematt Mar 17, 2023

zslayton Mar 17, 2023

popematt Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

zslayton Mar 17, 2023

jobarr-amzn Mar 17, 2023

jobarr-amzn Mar 17, 2023

jobarr-amzn Mar 17, 2023

jobarr-amzn Mar 17, 2023

zslayton Mar 17, 2023

jobarr-amzn Mar 17, 2023

zslayton Mar 17, 2023

jobarr-amzn Mar 17, 2023

jobarr-amzn Mar 17, 2023

jobarr-amzn Mar 17, 2023

zslayton Mar 17, 2023

		fn output(&self) -> &Self::Output;
		fn output_mut(&mut self) -> &mut Self::Output;

		let elems = Element::read_all(&data)?;
		let elems = Element::read_all(data)?;

Modernizes the ElementWriter trait #487

Modernizes the ElementWriter trait #487

Conversation

zslayton commented Mar 17, 2023 • edited Loading

codecov bot commented Mar 17, 2023

Codecov Report

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

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

Modernizes the `ElementWriter` trait #487

Modernizes the `ElementWriter` trait #487

zslayton commented Mar 17, 2023 •

edited

Loading