amazon-ion / ion-rust

Rust implementation of Amazon Ion
Apache License 2.0
150 stars 37 forks source link

Strongly Typed Element Views #503

Open almann opened 1 year ago

almann commented 1 year ago

When discussing how Value and the content within Value works in #500, we discussed adding typed views overElement`.

Having typed views as distinct types can allow a user to say accept as a parameter a StructElement or for types that are often used more structurally accept as a parameter a TextElement which could be represented as a non-null string or symbol value.

Another use for these APIs would be to have non Option return values for any of the relevant methods, StringElement::text() could return &str versus Element::as_text() returning Option<&str>

How we model this API is also interesting. The simplest option is to just model this as struct wrappers over Element or the annotations and the underlying content de-structured from Value. Adding trait abstraction on top of this (and potentially hiding the implementation with impl has some benefits as well--we could allow for better compositions of the APIs with trait bounds (e.g., trait StringElement: TextElement) which would make it easier to write generic functions over things that could take TextElement/StringElement/SymbolElement and so on.


Context From Thread on Value Factoring

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.

Originally posted by @almann in https://github.com/amazon-ion/ion-rust/issues/500#issuecomment-1492558726

popematt commented 1 year ago

I talked with @zslayton, and we came up with some requirements that we believe are important for a typed element API.

almann commented 1 year ago

Agree with the above as API requirements. I am also coming to the opinion that we should be capturing these views as trait as well because we will start to compose them in interesting ways. The struct implementations can then potentially be hidden as impl trait and I think for these kinds of APIs, encouraging generic code to be written is probably the right way to have users consume these APIs and no dynamic dispatch is required.

E.g.,

fn do_something(lob: &impl LobElement) {
  // do something with the reference to some concrete `LobElement`
}
almann commented 1 year ago

I put together a straw proposal of what I was thinking:

https://github.com/almann/ion-rust/commit/6c4ab4ead3e455bd16742f0a73a388a71d58fa68

The above is not complete by any means, but to illustrate what I was thinking.

I would love to use impl ClobView for this, but impl trait as type aliases and trait methods are in nightly still and we need that to make it easy to directly convert to a ClobView implementation from various places (e.g. Bytes, Clob, Blob).

popematt commented 1 year ago

I put together a prototype of another idea that I had: https://github.com/popematt/ion-rust/commit/01e4d4a587a0d0421689c25a6e47ef3df50ea8ae

This gets us (nearly) free casting between different concrete Element types and their references, and I think we can figure out a way to put trait-based views on top of this.

almann commented 1 year ago

Added some comments on your commit. I think this is close--but I think the traits are a little off. The derivation macros are regrettable, but probably correct.

popematt commented 1 year ago

FYI, I talked to @zslayton earlier this week and forgot to put an update here.

The idea that spawned my approach was that with some control over the memory layout (hence the use of repr(C)) and a little bit of unsafe Rust, we could get some nifty features, like casting pointers so that we can have e.g. an &IntElement where the underlying data is still Element. However, I learned from Zack about how the compiler can optimize some of the destructuring and creating a new object so that it can essentially be a no-op, so the transmuting is unnecessary.

I think the impl Trait approach may be the way to go, but I think some usage examples in your straw proposal would help me understand it better. Could you provide some usage examples?

almann commented 1 year ago

In general, the typical pattern we'd encourage is to be generic with respect to XXXView traits when dealing with typed elements. If we had impl trait as type aliases, we'd encourage folks to use those type aliases when the concrete type is required (generally when coercing with conversion traits).

fn do_something<V: LobView>(view_ref: &V) { ... }
fn do_something2<V: LobView>(view: V) { ... }

Narrowing conversions to concrete types though, do need a concrete type:

let view: ClobElement = element.try_into()?;
...
// this  could be defined with impl trait with current Rust (though not yet object safe)
let view = element.try_into_clob()?;

In the future, with type aliases as impl trait we can make things like ClobElement and even Element opaque so no one can actually depend on how it is defined, just that it is some concrete type that implements a trait, but for now we still need concrete structs for the "leaf" types of each view. I still recommend defining traits fully here to make everything consistent with the abstract types and also make it easier to move to impl trait fully in the future.