Support coercing structured Values into primitive types #379

2020-01-30T23:26:07-06:00

KodrAus commented

2020-01-30 23:26:07 -06:00

(Migrated from github.com)

This PR adds methods to kv::Value so you can attempt to coerce it into primitive types:

let value = Value::from("a string");

assert_eq!("a string", value.to_borrowed_str().unwrap());

I went with the to_ prefix instead of the as_ prefix that serde_json::Value has, because these aren't necessarily trivial conversions. The above example is, but something like Value::from_fill(v) may end up needing to call arbitrary user code to produce the value.

Doing this means our kv support is enough that we could theoretically replace the existing Record type with something like this instead:

pub struct Record<'a>(&'a dyn Source);

impl<'a> Record<'a> {
    fn target(&self) -> &str {
        self.0.get("target").expect("missing target").to_borrowed_str().expect("invalid target value")
    }
}

We don't want to do that, but I think it's a good feature watermark to aim for so folks writing log frameworks can use the key values to store their own arbitrary data.

For #328 This PR adds methods to `kv::Value` so you can attempt to coerce it into primitive types: ```rust let value = Value::from("a string"); assert_eq!("a string", value.to_borrowed_str().unwrap()); ``` I went with the `to_` prefix instead of the `as_` prefix that `serde_json::Value` has, because these aren't necessarily trivial conversions. The above example is, but something like `Value::from_fill(v)` may end up needing to call arbitrary user code to produce the value. Doing this means our `kv` support is enough that we could theoretically replace the existing `Record` type with something like this instead: ```rust pub struct Record<'a>(&'a dyn Source); impl<'a> Record<'a> { fn target(&self) -> &str { self.0.get("target").expect("missing target").to_borrowed_str().expect("invalid target value") } } ``` We don't want to do that, but I think it's a good feature watermark to aim for so folks writing log frameworks can use the key values to store their own arbitrary data.

KodrAus (Migrated from github.com) reviewed

2020-01-30 23:28:14 -06:00

Cargo.toml

					
				@ -53,3 +53,3 @@

				serde = { version = "1.0", optional = true, default-features = false }

				sval = { version = "0.4.2", optional = true, default-features = false }

				sval = { version = "0.5", optional = true, default-features = false }

This minor bump is fine because this is all unstable.

KodrAus commented

2020-01-31 01:30:46 -06:00

(Migrated from github.com)

I've refactored the module layout of kv::value a bit so it should be a bit easier to follow.

I've refactored the module layout of `kv::value` a bit so it should be a bit easier to follow.

KodrAus commented

2020-01-31 01:37:20 -06:00

(Migrated from github.com)

r? @yoshuawuyts

yoshuawuyts commented

2020-01-31 06:07:30 -06:00

(Migrated from github.com)

I went with the get_ prefix instead of the as_ prefix that serde_json::Value has, because these aren't necessarily trivial conversions.

I don't love the get_ prefix here. To me it sounds more like a lookup in some map structure than a type conversion. Even though there are costs associated using as_ or to_ (e.g. i8::to_string) feel like better fits. This is also motivated because there is no precedent for get_ in the stdlib. But I get why it exists, and there's no perfect perfect replacement either.

TryInto?

This also got me thinking: how viable would it be to replace these inherent methods with TryInto trait impls? serde_json predates TryInto, so it makes sense it wouldn't have it. By using NoneError the behavior for Option would essentially be the same.

I'm unsure how good of an idea this is, but this could be implemented with TryInto<T> for &Value so the conversion would never consume the original value (playground). From a look at the existing impls this should be able to cover the cases that have been laid out so far.

#![feature(try_trait)]

struct Foo { s: Option<String> }

impl std::convert::TryInto<String> for &Foo {
    type Error = std::option::NoneError;
    
    fn try_into(self) -> Result<String, Self::Error> {
        let s = self.s.as_ref()?;
        Ok(s.clone())
    }
}

Though admittedly I don't love this either, as usage could be a bit hard and it relies on NoneError from nightly (there's ways around it, but argh).

> I went with the get_ prefix instead of the as_ prefix that serde_json::Value has, because these aren't necessarily trivial conversions. I don't *love* the `get_` prefix here. To me it sounds more like a lookup in some map structure than a type conversion. Even though there are costs associated using `as_` or `to_` (e.g. [`i8::to_string`](https://doc.rust-lang.org/std/primitive.i8.html#method.to_string)) feel like better fits. This is also motivated because there is no precedent for `get_` in the stdlib. But I get why it exists, and there's no perfect perfect replacement either. ## TryInto? This also got me thinking: how viable would it be to replace these inherent methods with `TryInto` trait impls? `serde_json` predates `TryInto`, so it makes sense it wouldn't have it. By using `NoneError` the behavior for `Option` would essentially be the same. I'm unsure how good of an idea this is, but this could be implemented with `TryInto<T> for &Value` so the conversion would never consume the original value ([playground](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=67661ccb179adbb6a30dd39a00e4b681)). From a look at the existing impls this should be able to cover the cases that have been laid out so far. ```rust #![feature(try_trait)] struct Foo { s: Option<String> } impl std::convert::TryInto<String> for &Foo { type Error = std::option::NoneError; fn try_into(self) -> Result<String, Self::Error> { let s = self.s.as_ref()?; Ok(s.clone()) } } ``` Though admittedly I don't love this either, as usage could be a bit hard and it relies on `NoneError` from nightly (there's ways around it, but argh).

yoshuawuyts (Migrated from github.com) approved these changes

2020-01-31 06:11:21 -06:00

yoshuawuyts (Migrated from github.com) left a comment

Left a note with some ideas that popped up while reading this; but that shouldn't be a blocker for merging this. This seems useful overall.

KodrAus commented

2020-01-31 09:43:14 -06:00

(Migrated from github.com)

Thanks for the feedback @yoshuawuyts!

Yeh, the get naming does seem a bit iffy, and calling these coercions is also a bit inaccurate, since they’re also doing explicit casts. Perhaps we could do something like your TryFrom idea, but using From and Option instead?

impl<‘v> Value<‘v> {
    fn cast<T>(&self) -> Option<T>
    where
        Self: Into<Option<T>>;
}

impl<‘v> From<Value<‘v>> for Option<&’v str>>;

...

let s: &str = record.key_values().get(key).unwrap().cast().unwrap();

What do you think? We should be careful about implementing foreign traits for foreign types, but it could be a workable approach that also gives us a bunch of useful From/Into implementations.

Thanks for the feedback @yoshuawuyts! Yeh, the `get` naming does seem a bit iffy, and calling these _coercions_ is also a bit inaccurate, since they’re also doing explicit casts. Perhaps we could do something like your `TryFrom` idea, but using `From` and `Option` instead? ```rust impl<‘v> Value<‘v> { fn cast<T>(&self) -> Option<T> where Self: Into<Option<T>>; } impl<‘v> From<Value<‘v>> for Option<&’v str>>; ... let s: &str = record.key_values().get(key).unwrap().cast().unwrap(); ``` What do you think? We should be careful about implementing foreign traits for foreign types, but it could be a workable approach that also gives us a bunch of useful `From`/`Into` implementations.

KodrAus commented

2020-01-31 21:01:16 -06:00

(Migrated from github.com)

I've refactored the get_* methods into a more open cast API:

impl<'v> Value<'v> {
    pub fn cast<T>(self) -> Result<T, Self>
    where
        Self: Into<Option<T>>;

    pub fn cast_ref<'a, T>(&'a self) -> Option<T>
    where
        &'a Self: Into<Option<T>>;
}

which allows you to call:

let s: &str = value.cast().unwrap();

or:

let s: &str = value.cast_ref().unwrap();

The Into<Option<T>> bound seems a little strange, we could also consider using TryInto<T, Error = Self>.

So why cast and not downcast? The Value::cast method is similar to Box::downcast, but has an important difference: it will attempt to cast the value to the target type, not ensure the type exactly matches.

let v = Value::from(42i32);
let b: Box<dyn Any> = Box::new(42i32);

// This behaviour is the same
assert!(v.cast_ref::<i32>().is_some());
assert!(b.downcast_ref::<i32>().is_some());

// This is different
assert!(v.cast_ref::<u64>().is_some());
assert!(b.downcast_ref::<u64>().is_none());

I think this is much more useful for logging cases where you aren't necessarily in control of what concrete values you're given, but need a way to check if it looks something like what you're expecting (attempting to cast a timestamp to a u64 for instance).

So we can think of Value::cast as a more general Box::downcast.

I've refactored the `get_*` methods into a more open cast API: ```rust impl<'v> Value<'v> { pub fn cast<T>(self) -> Result<T, Self> where Self: Into<Option<T>>; pub fn cast_ref<'a, T>(&'a self) -> Option<T> where &'a Self: Into<Option<T>>; } ``` which allows you to call: ```rust let s: &str = value.cast().unwrap(); ``` or: ```rust let s: &str = value.cast_ref().unwrap(); ``` The `Into<Option<T>>` bound seems a little strange, we could also consider using `TryInto<T, Error = Self>`. So why `cast` and not `downcast`? The `Value::cast` method is similar to `Box::downcast`, but has an important difference: it will attempt to cast the value to the target type, not ensure the type exactly matches. ```rust let v = Value::from(42i32); let b: Box<dyn Any> = Box::new(42i32); // This behaviour is the same assert!(v.cast_ref::<i32>().is_some()); assert!(b.downcast_ref::<i32>().is_some()); // This is different assert!(v.cast_ref::<u64>().is_some()); assert!(b.downcast_ref::<u64>().is_none()); ``` I think this is much more useful for logging cases where you aren't necessarily in control of what concrete values you're given, but need a way to check if it looks something like what you're expecting (attempting to cast a timestamp to a `u64` for instance). So we can think of `Value::cast` as a more general `Box::downcast`.

KodrAus commented

2020-02-02 17:19:34 -06:00

(Migrated from github.com)

So I followed the Value::cast<T> thread and ran into a bit of a design problem with user-defined TryFrom implementations. Say we want to support attempting to downcast the Value to T directly, that logic would either have to live in the Value::cast method, and leave TryFrom implementations behaving inconsistently, or it would have to depend on all TryFrom implementations calling the right incantations to get a T from the Value.

So I've gone back to using inherent methods, with a to_ naming prefix. I've also added some proper downcasting support internally, which means we can do:

let s = Value::from_debug(&42u64).to_u32().expect("invalid value");

which makes the casting API more reliable when you want to try get a specific kind of value out.

I think we should definitely revisit the open trait approach in the future though, and it's something that could pair nicely with the discoverable to_ methods.

So I followed the `Value::cast<T>` thread and ran into a bit of a design problem with user-defined `TryFrom` implementations. Say we want to support attempting to downcast the `Value` to `T` directly, that logic would either have to live in the `Value::cast` method, and leave `TryFrom` implementations behaving inconsistently, or it would have to depend on all `TryFrom` implementations calling the right incantations to get a `T` from the `Value`. So I've gone back to using inherent methods, with a `to_` naming prefix. I've also added some proper downcasting support internally, which means we can do: ```rust let s = Value::from_debug(&42u64).to_u32().expect("invalid value"); ``` which makes the casting API more reliable when you want to try get a specific kind of value out. I think we should definitely revisit the open trait approach in the future though, and it's something that could pair nicely with the discoverable `to_` methods.

KodrAus commented

2020-02-02 17:27:06 -06:00

(Migrated from github.com)