Value/Content APIs Factoring to Capture Structurally Similar Content Types

[almann]

Problem Statement

Ion contains types that are structurally identical, but differ only in their type. In our current factoring of Value, we end up creating variants that are basically the same content, but differ only in the type of struct used in the variant. This leads to some code duplication and traits, that require generic code to be used if one wants to operate across these structurally identical types. I believe we could "push down" the value variants as a way to factor the API to avoid this boilerplate.

This is a distillation of my thinking after an offline thread with @zslayton.

Background

In the original Element/Value APIs the Value was the enum representing type and its contents of the value were modeled directly in the enum. The following is the original factoring abbreviated for illustration purposes:

struct Bytes {
    bytes: Vec<u8>,
}

struct Sequence {
    children: Vec<Element>,
}

enum Value {
    Int(Int),
    Float(f64),
    Decimal(Decimal),
    Timestamp(Timestamp),
    String(String),
    Symbol(Symbol),
    Bool(bool),
    Blob(Bytes),
    Clob(Bytes),
    SExp(Sequence),
    List(Sequence),
    Struct(Struct),
}

The latest refactoring of these APIs have captured the type within the content for the variants that are useful as intermediaries for the various APIs that build up these types and need to capture the intent of the user (e.g., I would like to build this sequence that is to become a Value::List(...)) this need is captured for example in the builder APIs and macros PR. This factoring looks something like this (again, abbreviated/digested for illustration purposes):

struct Blob {
    bytes: Vec<u8>,
}

struct Clob {
    bytes: Vec<u8>,
}

// trait to allow us to generically operate across Blob/Clob
trait Lob { ... }

struct SExp {
    children: Vec<Element>,
}

struct List {
    children: Vec<Element>,
}

// trait to allow us to generically operate across SExp/List
// called IonSequence in the real APIs
trait Sequence { ... }

enum Value {
    Int(Int),
    Float(f64),
    Decimal(Decimal),
    Timestamp(Timestamp),
    String(String),
    Symbol(Symbol),
    Bool(bool),
    Blob(Blob),
    Clob(Clob),
    SExp(SExp),
    List(List),
    Struct(Struct),
}

The problem with the above is two-fold, we have to sort of copy/paste the implementation Clob/Blob and SExp/List. These types have no relationship to each other (modulo a shared trait) and have some minor ergonomic implications to matching on say any Blob/Clob (i.e., work on any "LOB" value).

Proposed Factoring

I think we can mitigate the boilerplate above while retaining the benefits of having content that is strongly typed and "knows" what kind of variant it needs to be by pushing down these shared types in the Value enum:

enum LobType {
    Blob,
    Clob,
}
struct Lob {
    lob_type: LobType,
    bytes: Vec<u8>,
}

enum SequenceType {
    List,
    SExp,
}
struct Sequence {
    sequence_type: SequenceType,
    children: Vec<Element>,
}

enum Value {
    Int(Int),
    Float(f64),
    Decimal(Decimal),
    Timestamp(Timestamp),
    String(String),
    Symbol(Symbol),
    Bool(bool),

    // single variant for CLOB/BLOB
    Lob(Lob),
    // single variant for List/SExp
    Sequence(Sequence),
    
    Struct(Struct),
}

In the above, de-structuring becomes a bit more nested and the representation is marginally bigger in memory, but we no longer need the bridge traits and code can construct Lob and Sequence independently from Value which eliminates the boilerplate and the need for the bridge trait. This factoring also makes it trivial to operate on the structurally similar types by destructing to the common variant (i.e., Lob(...) and Sequence(...)).

[popematt]

I don't think I like the Lob and Sequence enums as you have proposed them. I think I'd rather just have a wrapper type. E.g.

struct Blob(Vec<u8>)
struct Clob(Vec<u8>)

enum Value {
    Null(IonType),
    Bool(bool),
    Int(Int),
    Float(f64),
    Decimal(Decimal),
    String(String),
    Symbol(Symbol),
    Blob(Blob),
    Clob(Clob),
    Timestamp(Timestamp),
    SExp(SExp),
    List(List),
    Struct(Struct),
}

If you want to operate on e.g. blobs and clobs the same way, you can match like this:

match element.value() {
    Value::Blob(Blob(bytes)) |
    Value::Clob(Clob(bytes)) => {
        bytes.len();
    }
    _ => todo!()
}

If we're concerned about abstracting away the Vec<u8>, then I'd actually consider going a step further with the wrapper types:

pub struct Lob {
    bytes: LobBytes,
}
// Private type, analogous to `SymbolText`, but for Lobs.
enum LobBytes {
    Vec(Vec<u8>),
    StaticSlice(&'static [u8])
}

pub struct Sequence {
    contents: Vec<Element>,
}

pub struct Blob(pub Lob)
pub struct Clob(pub Lob)
pub struct List(pub Sequence)
pub struct SExp(pub Sequence)

// ...

match element.value() {
    Value::Blob(Blob(lob)) |
    Value::Clob(Clob(lob)) => {
        // Do something with the Lob.
    }
    Value::List(List(seq)) |
    Value::SExp(SExp(seq)) => {
        // Do something with the Sequence.
    }
    _ => todo!()
}

We'll need to write a few more impl From... to keep the boilerplate down for users of the library, but I don't think it's all that unreasonable.

[zslayton]

If we're concerned about abstracting away the Vec...

We are; it gives us the ability to replace the implementation later.

I just gave Matt's proposal a whirl in the Rust playground. I like that it allows you to match on the two kinds at once OR individually without an additional guard. I'm ok with this if we don't mind the verbosity of the Value::Blob(Blob(lob)) matching case. I do note that we still end up with distinct Blob and Clob types, however, but there would probably much less boilerplate to support them.

[almann]

I like the idea of using the tuple struct for this to capture the tagging provided that the struct is fully transparent and we are just using it for the structure (e.g., pub struct Clob(pub Bytes) as written in @popematt's post). I think the trivial impl From <Bytes> for Clob and impl From<Clob> for Bytes is probably the only traits I would add if that's what we mean here.

I like the de-structuring of this a lot. The stuttering is regrettable, but not a deal breaker for me.

[popematt]

In the case of sequences, we might want more functionality than a [cb]?lob. (Can I use a regex to match multiple words in prose like that?) It's relatively simple to share most of the implementation with all of the involved types if we model the impl as a trait. Here's what I mean.

Whether we want to do this is another matter, though.

[zslayton]

Another (potentially controversial!) option to consider: leveraging Deref. Using Deref to mimic inheritance is labelled an anti-pattern, but I think that in this case it could make sense.

In our case:

• We are not attempting to create a type hierarchy. (EDIT: In tinkering with this in a PR, it could be considered a very small/simple type hierarchy. I still think it's limited enough that it's ok.)
• We own the only inner type to which the wrappers dereference (Bytes/Sequence). We have total control over what methods it exposes, so there's less risk of developing a leaky abstraction if (for example) the inner type were to unexpectedly get new methods.
• The inner type is public and readily accessible to users via .0 syntax. The fact that this is forwarding to the wrapped type feels pretty transparent to me, but that's subjective.

Of course, we could achieve the same thing with the delegate! macro. It would just be more verbose and slightly more tedious to update. Because we own Sequence, we're not likely to only want to offer a subset of its methods via the wrapper types.

Here's a playground demonstration.

This would also work for the sequence types, having List and SExp be able to dereference to the wrapped Sequence.

[zslayton]

I put together PR #502 that implements this pattern for Sequence & co, and I think the result is actually pretty nice.

[almann]

I like where this is going--one last thought about factoring (actually one I had originally but dismissed until @popematt brought up the tuple struct approach. What do you folks think about:

pub enum Lob {
  Blob(Bytes),
  Clob(Bytes)
}

versus two top level structs. It lets us treat these related things as a unit like the pub struct Blob(pub Bytes) and pub Clob(pub Bytes) which really are the same thing modulo the type as a tag.

[zslayton]

In the enum Lob design, can I somehow write a method that statically only accepts a Blob?

[popematt]

In the enum Lob design, can I somehow write a method that statically only accepts a Blob?

I agree with Zack.
This is the reason we are going to introduce the Blob and Clob types in the first place. The enum Lob design isn't much different from just sticking a Bytes struct in the Blob and Clob variants of Value.

[almann]

This is the reason we are going to introduce the Blob and Clob types in the first place.

This is, I think, is the core of where I don't line up with your thinking. We have Element to capture the full Ion value with its annotations, Value to capture the type + contents. To me, Blob/Clob/List/SExp are really just structure that is part of the Value to get to the native contents. Honestly, these intermediates are really just for structuring and de-structuring Ion values in my opinion.

I think we shouldn't try to force the Ion type granularity on content part of Value, but provide typed views at the Element (easy ways to get there from Value and their constituents) levels if you really want typed APIs to use in method/function signatures.

For example, I think a bunch of XElement trait/struct is the right way to model this because that is actually the view I would likely need if I want to type against the Ion value specifically at the granularity of Ion types. In practice if I am looking for a SExp specifically, wouldn't I potentially want the annotations for that as well? Wouldn't this go for any other statically typed APIs against Ion data? I'd rather solve the problem at that level if we want users to have typed APIs and make it really easy to get at the contents in those views (since you don't need Option anymore). The other reason is at this level we'd probably want other views that don't line up with our structuring like TextElement for string/symbol and maybe even something like NumberElement that normalizes across the numerics.

In other words, I'd rather put the proper typing and API ergonomics at the Element level and leave these enum/struct things as structuring into and out of native values.

When I get a chance, I'll draft something up to illustrate what I mean, but if you think I am being out of line to how the team wants the APIs to be factored, I am very much willing to disagree and commit.

[popematt]

Okay, so I think we have a decision to make about whether there should be structs that represent a single, specific Ion type, but it is orthogonal to the question about having a enum Lob { Blob(Bytes), Clob(Bytes) }.

• If the answer is "no, we don't want a struct for each Ion type", then we don't need a Lob enum because we already have the Value enum, and adding another level of indirection is not helpful.
• If the answer is "yes, we do want a struct for each Ion type" then we don't want a Lob enum because it is redundant. We have Clob and Blob tuples that can wrap a Bytes struct.

Either way, introducing a Lob enum does nothing to solve the problem of whether you can have a function that accepts only a certain type of Element, which is (I believe) the problem Zack encountered that prompted this whole thing. (Please correct me if I'm wrong.) Looking back, I think I was unclear in my previous message.

That being said, I think you're right that the Ion type-ing should probably be modeled on Element using Rust's type system rather than being modeled just an enum. However, I think the trouble we're going to run into is that we can't really encode the IonType into Element using traits without having to use dynamic dispatch. The benefit of the enum approach we currently have is that no dynamic dispatch is required anywhere to traverse through some tree of Elements.

That being said, I could see something like this maybe being useful:

pub struct BlobElement(pub Annotations, pub Bytes)
pub struct ClobElement(pub Annotations, pub Bytes)
pub struct SymbolElement(pub Annotations, pub Symbol)
// Etc. for every other Ion type.

enum Element {
    Blob(BlobElement),
    Clob(ClobElement),
    // ...
}

enum LobElement {
    Blob(BlobElement),
    Clob(ClobElement),
}
// Also:
// NumberElement
// TextElement
// SequenceElement


impl TryFrom<Element> for LobElement {
    fn try_from(element: Element) -> Result<LobElement, IonTypeCastError> {
        match element {
            Element::Blob(b) => Ok(LobElement::Blob(b)),
            Element::Clob(c) => Ok(LobElement::Clob(c)),
            other => Err(IonTypeCastError(format!("Expected a Blob or Clob; was {}", other.ion_type()))),
        }
    }
}
// Also:
impl TryFrom<LobElement> for BlobElement { /*...*/ }
impl TryFrom<LobElement> for ClobElement { /*...*/ }
impl TryFrom<Element> for BlobElement { /*...*/ }
impl TryFrom<Element> for ClobElement  { /*...*/ }
// Etc. for every other possible narrowing in the type hierarchy.

// Also also:
impl From<BlobElement> for Element { /*...*/ }
impl From<ClobElement> for Element { /*...*/ }
impl From<LobElement> for Element { /*...*/ }
impl From<BlobElement> for LobElement { /*...*/ }
impl From<ClobElement> for LobElement { /*...*/ }
// Etc. for every other possible widening in the type hierarchy.

Then, any time you want to accept a specific type of element, you can do fn foo(blob: BlobElement), and if you want a more general type of element, you can do fn foo<T: Into<LobElement>(lob: T).

[almann]

So I think your Element variants are very much what I would lean to--an important variant of this that the Value structure does not model is TextElement which is either a string or symbol. That one has no correspondance to Value directly because of the misalignment of the native content to the logical content. The TextElement example is actually the most common of these in my experience with working with Ion data.

That said, if we had the element APIs above and the trivial converters (or even trait layering on top with impl trait or the like).

I might still consider factoring Value as follows:

    /// Represents the type and content of an Ion value.
    /// The types of Ion values are encoded
    enum Value {
        Null(IonType),
        Int(Int),
        Float(f64),
        Decimal(Decimal),
        Timestamp(Timestamp),
        String(Str),
        Symbol(Symbol),
        Bool(bool),
        Binary(BinaryValue),
        Elements(ElementsValue),
        Struct(Struct),
    }

    /// Structural part of the [Value] that represents `blob`/`clob`.
    /// Generally, users will not refer to this type directly, but as part of de-structuring
    /// a [Value] and as an intermediate to construct a [Value] of the intended type.
    enum BinaryValue {
        Clob(Bytes),
        Blob(Bytes),
    }

    impl BinaryValue {
        fn bytes(&self) -> &Bytes {
            match &self {
                Blob(b) => b,
                Clob(b) => b,
            }
        }
    }

    /// Structural part of the [Value] that represents `list`/`sexp`
    /// Generally, users will not refer to this type directly, but as part of de-structuring
    /// a [Value] and as an intermediate to construct a [Value] of the intended type.
    enum ElementsValue {
        List(Elements),
        SExp(Elements),
    }

I named the sub-enum types with the suffixed Value to really emphasize what I think is going on. To wit, the we are pushing down some of the structure of Value into BinaryValue and ElementsValue respectively when multiple Ion types have one structural native type for the contents. This mostly avoids the stuttering of de-structuring and also allows us to define the methods for structurally identical native types under one type. It doesn't of course allow for a granular ...Value type to be used, but I think that is better handled at the Element level as your example shows.

[almann]

Here are some of the de-structuring cases that I think read better:

// match any binary type
match &v {
    Binary(b) => Ok(b.bytes()),
    _ => Error::msg("Not what I was expecting"),
}

// match specifically BLOB
match &v {
    Binary(Blob(b)) => Ok(b),
    _ => Error::msg("Not what I was expecting"),
}

[popematt]

Here are some of the de-structuring cases that I think read better

Read better than what—the current API or something else?

// match specifically BLOB
match &v {
    Binary(Blob(b)) => Ok(b),
   _ => Error::msg("Not what I was expecting"),
}

I actually don't like this example because lots of matching like this will get very noisy—I'd rather just use the short-circuit return. I also think that using try_into() can eliminate a lot of the stuttering even if there is a lot of repetitive structure in the data model. E.g.:

let Blob(b) = &v.try_into()?;

I think we really ought to be using TryFrom for downcasting from Element to specific element types or groups of element types. TryFrom make it even easier to have idiomatic conversion because TryFrom returns a Result, allowing library users to take advantage for the ? operator instead of always having to have an _ => Err(...) case. My personal preference is that I only have to use pattern matching when I actually want to match more than one non-error case, and those cases are handled differently from each other.

I think I've actually changed my mind since writing my last comment. After playing around with some examples, I'd like even LobElement to be a (Annotations, Bytes) tuple rather than an enum. Enums can be used for handling variations in the values of an element type.

// Using try_into() and infallible destructuring
let ListElement(annotations, some_sequence) = element_1.try_into()?;
let LobElement(_, bytes) = element_2.try_into()?;

// For a symbol:
let symbol_text = element_3.try_into::<SymbolElement>()?.text_or_err()?;

// Using try_into() and exhaustive pattern matching, ignoring annotations
let NumberElement(_, number) = element_3.try_into()?;
match number {
    NumberValue::I64(i) => { /* handle int */ }
    NumberValue::BigInt(i) => { /* handle int */ }
    NumberValue::Float(f) => { /* handle float */ }
    NumberValue::Decimal(d) => { /* handle decimal */ }
}

Here's a real-world example—I'd love to be able to rewrite something like these ~55 lines of ion-schema-rust more like this:

let ListElement(annotations, sequence) = value.try_into()?;
invalid_isl!(!annotations.is_empty(), "timestamp_offset list may not be annotated")?;

let valid_offsets: Vec<TimestampOffset> = sequence.iter()
    .map(|it| {
        let StringElement(annotations, text) = it.try_into()?;
        invalid_isl!(!annotations.is_empty(), "timestamp_offset value may not be annotated")?;
        text.try_into::<TimestampOffset>()
    } )
    .collect()?;

As long as we have impl From<IonTypeCastError> for InvalidSchemaError, this is so much more succinct and readable than the existing code because we can use try_into()?. (Though I do acknowledge that I am saving a few lines by inventing a invalid_isl! macro.)

[almann]

Here are some of the de-structuring cases that I think read better

Read better than what—the current API or something else?

Read better than the current Value de-structuring which is (from your example):

match element.value() {
    Value::Blob(Blob(lob)) |
    Value::Clob(Clob(lob)) => {
        // Do something with the Lob.
    }
    Value::List(List(seq)) |
    Value::SExp(SExp(seq)) => {
        // Do something with the Sequence.
    }
    _ => todo!()
}

I would suggest we move the discussion about typed Element to another #503, that is really a different API than the thing I want to zero in on in this issue which is the structure of Value.

To be clear, I think we're in violent agreement about a typed Element API (with a lot of devil in the details), but what is your take on the structure of Value?

[popematt]

To be clear, I think we're in violent agreement about a typed Element API

I agree with you so much that you'd better watch your back. 😉

what is your take on the structure of Value?

I think if we have properly typed Element API, then I don't think it's absolutely necessary to have the Ion type baked into the Value part of the API.

If we have some sort of NumberElement API, we might want a NumberValue enum with F64, I64, Decimal, and BigInt variants. I could also see it being convenient to keep a Value::Blob(Bytes) style of enum if we continue to have an AnyElement sort of struct, but I don't think the Ion type needs to live anywhere other than the Element API. But I think the properly typed Element API is far more important, and Value is a secondary concern.

[zslayton]

I'm still not sold on the value of unions like Lob and Sequence as direct variants of Value.

Having union variants for structurally similar types optimizes for the case in which users of the API:

1. Understand that there are pairs of very similar types (Blob/Clob and List/SExp)
2. Know why they should or should not care about the distinction between the similar types
3. Wish to accept EITHER kind from the pair with unified handling in a single match arm.

I don't believe that this is a common enough case to optimize for. The vast majority of Ion users don't know what a Clob is or why they would pick it over a Blob. Similarly (but to a lesser degree), most Ion users don't use SExp.

I also don't think we should actively encourage users to treat Blob/Clob and List/SExp as interchangeable. They are, of course, free to do so where that makes sense. But it should be an opt-in behavior.

Following PR #502, a user would do this to get a List:

if let List(l) = value {
   // Do something with a list
}

They want a list, they get a list. If it's unified, they need to do:

if let Sequence(List(l)) = value {
  // Do something with `l`
}

Which means:

1. They have to know that there's a Sequence type and what it's for. IDE autocomplete will not be super helpful when they type if let L and hope for List to be there.
2. It's harder (more verbose) to constrain element to a single Ion type, which I believe should be the default behavior.

As currently implemented, allowing unified handling of the List and SExp types isn't terrible apart from the stutter needed to do an extra layer of destructuring:

if let List(List(sequence)) | SExp(SExp(sequence)) {
  // do something with `sequence`
}

I find the 1:1 mapping from Value variants to Ion types to be both intuitive and helpful for automating conversions. I'm very reluctant to sacrifice that for what seems to be a minimal syntactic difference in an uncommon use case.

[zslayton]

I had a discussion with @popematt about this offline; I think I have a way to get the best of both worlds. Will follow up shortly.

[zslayton]

Please see PR #505 for my proposed fix. It removes the extra nesting from the Value variants, but preserves the standalone struct List(Sequence) and struct SExp(Sequence) types for use in the SequenceBuilder, ion_list!, and ion_sexp!.

[zslayton]

PR #506 implements the proposed fix for Bytes/Blob/Clob as well.