Makes `List` and `SExp` thin wrappers around `Sequence` #502

zslayton · 2023-03-31T14:56:05Z

See issue #500 for discussion.

This PR removes the IonSequence trait and reintroduces a concrete Sequence type. It also converts List and SExp into tuple structs that wrap Sequence.

ListBuilder and SExpBuilder have also been removed in favor of a single SequenceBuilder implementation with build_list() and build_sexp() methods.

List and SExp each implement Deref<Target=Sequence>, allowing them both to expose the functionality of their underlying Sequence of elements transparently.

This eliminates a fair amount of boilerplate code that was needed to provide functionality for both types. It also enables unified handling for List and SExp in match expressions in situations where the author does not care about the Ion type distinction. Finally, users no longer have to import IonSequence in order to access the basic functionality of List and SExp.

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

See issue #500 for discussion. This PR removes the `IonSequence` trait and reintroduces a concrete `Sequence` type. It also converts `List` and `SExp` into tuple structs that wrap `Sequence`. `ListBuilder` and `SExpBuilder` have also been removed in favor of a single `SequenceBuilder` implementation with `build_list()` and `build_sexp()` methods. `List` and `SExp` each implement `Deref<Target=Sequence>`, allowing them both to expose the functionality of their underlying `Sequence` of elements transparently. This eliminates a fair amount of boilerplate code that was needed to provide functionality for both types. It also enables unified handling for `List` and `SExp` in match expressions in situations where the author does not care about the Ion type distinction.

zslayton

🗺️ PR tour

zslayton · 2023-03-31T14:57:14Z

src/element/builders.rs

 /// assert_eq!(actual, expected);
 /// ```
-pub struct ListBuilder {


🗺️ The distinct ListBuilder and SExpBuilder types (which were mostly duplicate code) have been deleted in favor of a unified SequenceBuilder type.

zslayton · 2023-03-31T14:59:28Z

src/element/builders.rs

+/// let actual: SExp = Sequence::builder()
+///     .push(1)
+///     .push(true)
+///     .push("foo")
+///     .build_sexp();


🗺️ While Sequence::builder()... followed by build_list() or build_sexp() works just fine, I expect most users to continue reaching for the ion_list! and ion_sexp! macros, which now use SequenceBuilder under the hood.

zslayton · 2023-03-31T15:01:50Z

src/element/builders.rs

+    /// Builds a [`Sequence`] with the previously specified elements.
+    pub fn build(self) -> Sequence {
+        self.values.into()
+    }
+
+    /// Builds a [`List`] with the previously specified elements.
+    pub fn build_list(self) -> List {
+        List(self.build())
+    }
+
    /// Builds a [`SExp`] with the previously specified elements.
-    pub fn build(self) -> SExp {
-        SExp::new(self.values)
+    pub fn build_sexp(self) -> SExp {
+        SExp(self.build())


🗺️ The unified SequenceBuilder type now offers build(), build_list(), and build_sexp() methods that construct the expected types.

zslayton · 2023-03-31T15:30:53Z

src/element/builders.rs

-// These `From` implementations allow a builder to be passed into any method that takes an
-// `Into<Element>`, allowing you to avoid having to explicitly call `build()` on them when
-// the intent is clear.
-
-impl From<ListBuilder> for Element {
-    fn from(list_builder: ListBuilder) -> Self {
-        list_builder.build().into()
-    }
-}
-
-impl From<SExpBuilder> for Element {
-    fn from(s_expr_builder: SExpBuilder) -> Self {
-        s_expr_builder.build().into()
-    }
-}
-
-impl From<StructBuilder> for Element {
-    fn from(struct_builder: StructBuilder) -> Self {
-        struct_builder.build().into()
-    }
-}
-


🗺️ These From implementations were introduced before the collections macros were available. They made things slightly less verbose when passing off a builder to something that expected an Into<Element>.

For example:

// Old, pre-macro code fn do_something(element: impl Into<Element>) { // ... } // Pass the builder off without calling `build()` do_something(List::builder.push(1).push(2).push(3));

Now that we're using a single type for constructing List and SExp (that is: SequenceBuilder), we can't correctly infer which sequence type the user wants to construct without additional information. However, we also have the macros now which are even more concise, so I'm removing this shortcut.

// New code fn do_something(element: impl Into<Element>) { // ... } do_something(ion_list![1, 2, 3]);

zslayton · 2023-03-31T15:34:02Z

src/element/list.rs

+impl Deref for List {
+    type Target = Sequence;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
    }
+}


🗺️ This impl allows users to transparently access the underlying Sequence's methods.

Deref polymorphism is considered an anti-pattern. Can you explain why you went with this approach instead of any of the alternatives?

I discussed this in our design thread on issue 500.

Oh, I missed that. I'll go take a look.

I just pushed a commit to replace Deref with delegate!. The result is slightly more verbose, but it's probably worth it to sidestep potential controversy.

zslayton · 2023-03-31T15:36:03Z

src/element/mod.rs

+    pub fn as_sequence(&self) -> Option<&Sequence> {
        match &self.value {
-            Value::SExp(sexp) => Some(sexp),
-            Value::List(list) => Some(list),
+            Value::SExp(SExp(s)) | Value::List(List(s)) => Some(s),
            _ => None,
        }
    }


🗺️ Notice:

Accessing the contents of the List or SExp no longer requires dynamic dispatch. 🎉

Handling for SExp and List can occur in the same arm of the match.

zslayton · 2023-03-31T15:37:46Z

src/lib.rs

@@ -110,7 +110,7 @@
 //! ```
 //! use ion_rs::IonResult;
 //! # fn main() -> IonResult<()> {
-//! use ion_rs::element::{Element, IonSequence, IntoAnnotatedElement,  Value};
+//! use ion_rs::element::{Element, IntoAnnotatedElement,  Value};


🗺️ Users no longer have to import the IonSequence trait in order to access the basic functionality of List and SExp.

src/element/list.rs

popematt · 2023-03-31T17:05:53Z

src/element/list.rs

+    delegate! {
+        to self.0 {
+            pub fn clone_builder(&self) -> SequenceBuilder;
+            pub fn elements(&self) -> ElementsIterator<'_>;
+            pub fn get(&self, index: usize) -> Option<&Element>;
+            pub fn len(&self) -> usize;
+            pub fn is_empty(&self) -> bool;
+        }


Nice! I actually quite like this.

src/element/list.rs

popematt · 2023-03-31T17:14:30Z

src/element/builders.rs

    values: Vec<Element>,
 }

-impl SExpBuilder {
-    /// Crate visible; users should call [`SExp::builder()`] or [`Element::sexp_builder()`] instead.
+impl SequenceBuilder {


If we parameterize SequenceBuilder, the return type of build() can be set when the SequenceBuilder is created so that you can't write List::builder().build_sexp().

struct SequenceBuilder<T> { _target_type: Phantom<T>, values: Vec<Element>, } impl <T> SequenceBuilder<T> { /// Crate visible; users should call [`Sequence::builder()`] instead. pub(crate) fn new() -> Self { Self { values: Vec::new(), _target_type: Phantom::default() } } /// Helper method for [`Sequence::clone_builder()`]. pub(crate) fn with_initial_elements(elements: &[Element]) -> Self { let mut new_elements = Vec::with_capacity(elements.len()); new_elements.extend_from_slice(elements); Self { values: new_elements, _target_type: Phantom::default() } } // etc. } // Repeate for Sequence and SExp impl SequenceBuilder<List> { pub fn build(self) -> List { List(self.values.into()) } }

This would work, though I think my current leaning would be to simply eliminate the

List::builder() -> SequenceBuilder
and

SExp::builder() -> SequenceBuilder

synonyms in favor of the (already available) Sequence::builder() -> SequenceBuilder method.

The doc comment examples for List and SExp can demonstrate using Sequence::builder() to construct them, and most users will reach for the macros anyway.

An interesting (minor) benefit of using the same builder type for all sequences is that I can do:

let sexp = ion_sexp!(1 2 3); // I can make a List using the SExp's elements as a starting point let list = sexp.clone_builder().build_list();

It's possible that we'll want to introduce a Stream sequence type--values with no container, which are used at the top level and (in Ion 1.1) template expansion. We would also be able to use SequenceBuilder for that.

What do you think?

An interesting (minor) benefit of using the same builder type for all sequences is that I can do:

let list = sexp.clone_builder().build_list();

We could use the generics so that List::builder() only lets you build a list, but list.clone_builder() will let you build a List, SExp, or Sequence. E.g.:

impl SequenceBuilder<Cloned> { pub fn build_list(self) -> List { List(self.values.into()) } pub fn build_sexp(self) -> SExp { List(self.values.into()) } // Etc. }

That being said, I think the solution you've proposed is fine (and certainly simpler).

codecov · 2023-04-03T12:46:43Z

Codecov Report

Patch coverage: 89.72% and project coverage change: +0.02 🎉

Comparison is base (ecdf439) 89.58% compared to head (40f0e21) 89.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   89.58%   89.60%   +0.02%     
==========================================
  Files          75       80       +5     
  Lines       13894    13965      +71     
==========================================
+ Hits        12447    12514      +67     
- Misses       1447     1451       +4

Impacted Files	Coverage Δ
src/binary/non_blocking/raw_binary_reader.rs	`81.79% <ø> (ø)`
src/element/writer.rs	`92.45% <ø> (ø)`
src/ion_hash/representation.rs	`99.06% <ø> (ø)`
src/lib.rs	`88.84% <ø> (ø)`
src/stream_reader.rs	`100.00% <ø> (ø)`
src/text/text_formatter.rs	`58.72% <0.00%> (ø)`
src/types/mod.rs	`75.00% <ø> (ø)`
src/types/string.rs	`65.78% <65.78%> (ø)`
src/symbol.rs	`84.69% <80.00%> (ø)`
src/element/sexp.rs	`84.37% <84.37%> (ø)`
... and 14 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 · 2023-04-03T19:01:16Z

@almann I think the change in this PR gets us closer to what you're after. I'm going to go ahead and merge this, but I won't close #500; we can continue to discuss it.

zslayton added 2 commits March 31, 2023 10:46

fixed doc comment

82f6a78

zslayton commented Mar 31, 2023

View reviewed changes

zslayton requested review from popematt and almann March 31, 2023 15:38

zslayton mentioned this pull request Mar 31, 2023

Value/Content APIs Factoring to Capture Structurally Similar Content Types #500

Closed

Replaces Deref with delegate!

86dafa7

popematt reviewed Mar 31, 2023

View reviewed changes

zslayton added 2 commits April 3, 2023 08:37

Removes builder() from List, SExp

bb0d55f

Adds doc link from List, SExp to SequenceBuilder

40f0e21

popematt approved these changes Apr 3, 2023

View reviewed changes

zslayton merged commit 4205694 into main Apr 3, 2023

zslayton deleted the sequence branch April 3, 2023 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes `List` and `SExp` thin wrappers around `Sequence` #502

Makes `List` and `SExp` thin wrappers around `Sequence` #502

zslayton commented Mar 31, 2023

zslayton left a comment

zslayton Mar 31, 2023

zslayton Mar 31, 2023

zslayton Mar 31, 2023

zslayton Mar 31, 2023

zslayton Mar 31, 2023

popematt Mar 31, 2023

zslayton Mar 31, 2023

popematt Mar 31, 2023

zslayton Mar 31, 2023

zslayton Mar 31, 2023

zslayton Mar 31, 2023

popematt Mar 31, 2023

popematt Mar 31, 2023 •

edited

Loading

zslayton Apr 1, 2023 •

edited

Loading

popematt Apr 3, 2023

codecov bot commented Apr 3, 2023 •

edited

Loading

zslayton commented Apr 3, 2023

Makes List and SExp thin wrappers around Sequence #502

Makes List and SExp thin wrappers around Sequence #502

Conversation

zslayton commented Mar 31, 2023

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

popematt Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

zslayton Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2023 • edited Loading

Codecov Report

zslayton commented Apr 3, 2023

Makes `List` and `SExp` thin wrappers around `Sequence` #502

Makes `List` and `SExp` thin wrappers around `Sequence` #502

popematt Mar 31, 2023 •

edited

Loading

zslayton Apr 1, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading