Simple zero-dependency visitor for kv::Value

KodrAus commented

2021-01-08 20:35:40 -06:00

(Migrated from github.com)

Motivation

Users want to be able to visit simple primitives captured in kv::Values but currently resort to chaining calls like:

if let Some(v) = value.to_i64() {
    ..
} else if let Some(v) = value.to_u64() {
    ..
} ..

This is flaky and not fun to write. The goal of the current design is that if you're serializing values you probably already have a serialization framework handy (either std::fmt, serde, or sval) that you'll want to use.

There are reasons to want to visit values without pulling one of these in though. For example, https://github.com/Thomasdezeeuw/std-logger/pull/29#discussion_r553903486 wants to write custom formatting logic to make use of IoSlices, and others don't want the additional mental overhead in learning another framework.

Proposal

Add a simple zero-dependency visit API for log::kv::Value that can be used instead of chaining to_* methods or learning a full serialization framework. It intentionally won't support complex values, but should be enough for simple cases to get off the ground with. A new trait, kv::value::Visitor will be added with the following methods:

pub mod value {
    pub trait Visitor<'v> {
        fn visit_i64(&mut self, v: i64) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_u64(&mut self, v: u64) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_bool(&mut self, v: bool) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_str(&mut self, v: &str) -> Result<(), Error> { self.visit_any(v.to_value()) }
        fn visit_borrowed_str(&mut self, v: &'v str) -> Result<(), Error> { self.visit_str(v) }
        fn visit_error(&mut self, v: &(dyn std::error::Error + 'static)) -> Result<(), Error> { self.visit_any(Value::from_dyn_error(v)) }
        fn visit_borrowed_error(&mut self, v: &'v (dyn std::error::Error + 'static)) -> Result<(), Error> { self.visit_error(v) }

        // NOTE: This is the only required method
        fn visit_any(&mut self, v: Value) -> Result<(), Error>;
    }
}

with a default implementation looking something like:

impl<'v> value::Visitor<'v> for MyVisitor {
    fn visit_any(&mut self, v: Value) -> Result<(), Error> {
        write!(self.buf, "{}", v)?;
        Ok(())
    }
}

A Value can be visited using an inherent visit method:

impl<'v> Value<'v> {
    pub fn visit(&self, visitor: impl Visitor<'v>) -> Result<(), Error> {
        // Here we know what kind of value we're looking at,
        // so can call the most specific method on `value::Visitor`
    }
}

Other methods can be overridden to customize their behavior.

Migrating from `value::Visitor` to `sval` or `serde`

If callers want to switch from value::Visitor to sval or serde to support more complex serialization needs, the change will be from calling Value::visit to either <Value as sval::Value>::stream or <Value as serde::Serialize>::serialize.

Questions

Why not add these to `source::Visitor`?

The source::Visitor trait is centred around visit_pair, which lets us build higher-level methods on top, like get. If we add other methods to source::Visitor it may become confusing how the trait should be implemented to support these higher-level methods.

Why does `visit_any` take a `Value`?

The Value type is a carrier for a bunch of trait implementations, fmt::Debug, fmt::Display, and possibly sval::Value, and serde::Serialize. By passing in a Value here we give the caller flexibility to decide what trait they want to use without adding a lot of complexity to the value::Visitor trait.

## Motivation Users want to be able to visit simple primitives captured in `kv::Value`s but currently resort to chaining calls like: ```rust if let Some(v) = value.to_i64() { .. } else if let Some(v) = value.to_u64() { .. } .. ``` This is flaky and not fun to write. The goal of the current design is that if you're serializing values you probably already have a serialization framework handy (either `std::fmt`, `serde`, or `sval`) that you'll want to use. There are reasons to want to visit values without pulling one of these in though. For example, https://github.com/Thomasdezeeuw/std-logger/pull/29#discussion_r553903486 wants to write custom formatting logic to make use of `IoSlice`s, and others don't want the additional mental overhead in learning another framework. ## Proposal Add a simple zero-dependency visit API for `log::kv::Value` that can be used instead of chaining `to_*` methods or learning a full serialization framework. It intentionally won't support complex values, but should be enough for simple cases to get off the ground with. A new trait, `kv::value::Visitor` will be added with the following methods: ```rust pub mod value { pub trait Visitor<'v> { fn visit_i64(&mut self, v: i64) -> Result<(), Error> { self.visit_any(v.to_value()) } fn visit_u64(&mut self, v: u64) -> Result<(), Error> { self.visit_any(v.to_value()) } fn visit_bool(&mut self, v: bool) -> Result<(), Error> { self.visit_any(v.to_value()) } fn visit_str(&mut self, v: &str) -> Result<(), Error> { self.visit_any(v.to_value()) } fn visit_borrowed_str(&mut self, v: &'v str) -> Result<(), Error> { self.visit_str(v) } fn visit_error(&mut self, v: &(dyn std::error::Error + 'static)) -> Result<(), Error> { self.visit_any(Value::from_dyn_error(v)) } fn visit_borrowed_error(&mut self, v: &'v (dyn std::error::Error + 'static)) -> Result<(), Error> { self.visit_error(v) } // NOTE: This is the only required method fn visit_any(&mut self, v: Value) -> Result<(), Error>; } } ``` with a default implementation looking something like: ```rust impl<'v> value::Visitor<'v> for MyVisitor { fn visit_any(&mut self, v: Value) -> Result<(), Error> { write!(self.buf, "{}", v)?; Ok(()) } } ``` A `Value` can be visited using an inherent `visit` method: ```rust impl<'v> Value<'v> { pub fn visit(&self, visitor: impl Visitor<'v>) -> Result<(), Error> { // Here we know what kind of value we're looking at, // so can call the most specific method on `value::Visitor` } } ``` Other methods can be overridden to customize their behavior. ## Migrating from `value::Visitor` to `sval` or `serde` If callers want to switch from `value::Visitor` to `sval` or `serde` to support more complex serialization needs, the change will be from calling `Value::visit` to either `<Value as sval::Value>::stream` or `<Value as serde::Serialize>::serialize`. ## Questions ### Why not add these to `source::Visitor`? The `source::Visitor` trait is centred around `visit_pair`, which lets us build higher-level methods on top, like `get`. If we add other methods to `source::Visitor` it may become confusing how the trait should be implemented to support these higher-level methods. ### Why does `visit_any` take a `Value`? The `Value` type is a carrier for a bunch of trait implementations, `fmt::Debug`, `fmt::Display`, and possibly `sval::Value`, and `serde::Serialize`. By passing in a `Value` here we give the caller flexibility to decide what trait they want to use without adding a lot of complexity to the `value::Visitor` trait.

👍 1

KodrAus commented

2021-01-08 22:44:41 -06:00

(Migrated from github.com)

If we wanted to re-introduce the Fill API which would be needed to do zero-cost bridging between log and tracing then this Visitor type could be used in place of that Slot, which is really just a value::Visitor.

If we wanted to re-introduce [the `Fill` API](https://docs.rs/value-bag/1.0.0-alpha.5/value_bag/fill/struct.Slot.html) which would be needed to do zero-cost bridging between `log` and `tracing` then this `Visitor` type could be used in place of that `Slot`, which is really just a `value::Visitor`.

KodrAus commented

2021-01-10 18:03:26 -06:00

(Migrated from github.com)

I've updated the proposed trait based on the design that ended up coming out of value-bag in https://github.com/sval-rs/value-bag/pull/16

All it really does is add a 'v lifetime to the value::Visitor trait, so that it becomes possible to capture borrowed strings across calls if you need. This 'v lifetime will be the same as the 'kvs in Visitor::visit_pair<'kvs> so it ends up being threaded all the way through from a borrow of the Source to visitors of the Values.

I've updated the proposed trait based on the design that ended up coming out of `value-bag` in https://github.com/sval-rs/value-bag/pull/16 All it really does is add a `'v` lifetime to the `value::Visitor` trait, so that it becomes possible to capture borrowed strings across calls if you need. This `'v` lifetime will be the same as the `'kvs` in `Visitor::visit_pair<'kvs>` so it ends up being threaded all the way through from a borrow of the `Source` to visitors of the `Value`s.

KodrAus commented

2021-01-31 17:21:48 -06:00

(Migrated from github.com)

Now that we've got 0.4.14 out we can make progress on this 🎉

Now that we've got `0.4.14` out we can make progress on this :tada:

Thomasdezeeuw commented

2021-11-13 10:57:03 -06:00

(Migrated from github.com)

@KodrAus what parts are missing to implement this?

KodrAus commented

2021-11-13 22:56:05 -06:00

(Migrated from github.com)

Hey @Thomasdezeeuw 👋

The implementation work was actually all there, so I've opened up #467 that surfaces the visitor API.

Hey @Thomasdezeeuw :wave: The implementation work was actually all there, so I've opened up #467 that surfaces the visitor API.

👍 1

Rows
Columns

Simple zero-dependency visitor for kv::Value #440

Motivation

Proposal

Migrating from value::Visitor to sval or serde

Questions

Why not add these to source::Visitor?

Why does visit_any take a Value?

Migrating from `value::Visitor` to `sval` or `serde`

Why not add these to `source::Visitor`?

Why does `visit_any` take a `Value`?