Support coercing structured Values into primitive types

[KodrAus]

For #328

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.

[KodrAus]

This minor bump is fine because this is all unstable.

[KodrAus]

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

[KodrAus]

r? @yoshuawuyts

[yoshuawuyts]

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).

[yoshuawuyts]

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]

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.

[KodrAus]

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.

[KodrAus]

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.

[KodrAus]

Thanks for taking the time to review this and providing feedback @yoshuawuyts!

[yoshuawuyts]

@KodrAus oh I love that API! I def think you made the right call here; glad it ended up the way it did!