Improve glib::Value usage by merging FromValue/FromValueOptional traits

[sdroege]

Currently we have

pub trait FromValueOptional<'a>: StaticType + Sized {
    unsafe fn from_value_optional(_: &'a Value) -> Option<Self>;
}

pub trait FromValue<'a>: FromValueOptional<'a> {
    unsafe fn from_value(_: &'a Value) -> Self;
}

This has the effect that we have

let v: Option<i32> = value.get::<i32>().unwrap();

while v can never ever be None here. There's get_some() which only exists for FromValue types.

I would suggest to instead have the following instead of the two traits

pub trait FromValue<'a> {
    type Target;
    unsafe fn from_value(_: &'a Value) -> Self::Target;
}

impl<'a> FromValue<'a> for i32 {
    type Target = i32;

    unsafe fn from_value(_: &'a Value) -> i32 { ... }
}

impl<'a> FromValue<'a> for String {
    type Target = Option<String>;

    unsafe fn from_value(_: &'a Value) -> Option<String> { ... }
}

etc.

This would allow this confusion to disappear, get::<i32>() would return i32 and get::<String>() would return Option<String>.

SetValue and SetValueOptional would stay the same as there this problem doesn't really exist.

@GuillaumeGomez @EPashkin Opinions?

[GuillaumeGomez]

This looks better, indeed. I prefer this approach.

[sdroege]

Implemented it but it doesn't work that well with type-inference anymore:

// Worked but does not work anymore
let x: i32 = v.get();
// Still works
let x = v.get::<i32>();

The reason is, I guess, because there could be multiple impls with the same target type.

[sdroege]

So if someone can make the following compile, let me know :) Otherwise I think having to require type annotations always is a bit suboptimal.

pub trait FromValue<'a> {
    type Target;
    unsafe fn from_value(_: &'a Value) -> Self::Target;
}

impl<'a> FromValue<'a> for i32 {
    type Target = i32;

    unsafe fn from_value(_: &'a Value) -> i32 { unimplemented!() }
}

impl<'a> FromValue<'a> for String {
    type Target = Option<String>;

    unsafe fn from_value(_: &'a Value) -> Option<String> { unimplemented!() }
}

struct Value;

impl Value {
    pub fn get<'a, T: FromValue<'a>>(&'a self) -> Result<T::Target, ()> { unimplemented!() }
}

fn main() {
    let x: i32 = Value.get().unwrap();
    let x: Option<String> = Value.get().unwrap();
    let x = Value.get::<i32>().unwrap();
    let x = Value.get::<String>().unwrap();
}

[nightmared]

Sorry about my previous comment, it was so damn stupid I deleted it ;(
I don't believe this to be possible as is (you cannot prevent multiple traits from having the same type for Target AFAIK), but maybe you could do it by inverting the traits, like this:

pub trait FromValue<'a> {
    type Source;
    unsafe fn from_value(_: &'a Value) -> Self;
}

impl<'a> FromValue<'a> for i32 {
    type Source = i32;
    unsafe fn from_value(_: &'a Value) -> i32 { unimplemented!() }
}

impl<'a> FromValue<'a> for Option<String> {
    type Source = String;
    unsafe fn from_value(_: &'a Value) -> Option<String> { unimplemented!() }
}

pub struct Value;

impl Value {
    fn get<'a, T: FromValue<'a>>(&'a self) -> Result<T, ()> { unimplemented!() }
}


fn main() {
    let x: i32 = Value.get().unwrap();
    let x: Option<String> = Value.get().unwrap();
    // doesn't allow for this sadly:
    let x = Value.get::<i32>().unwrap();
    let x = Value.get::<Option<String>>().unwrap();
}

Edit: the Source type in the example above doesn't serve any purpose, so maybe deleting it is an option. But it stays quite hideous as you have to explain the full destination type Option<String> instead of just String.

[EPashkin]

I have no idea how to fix it, even if remove unneeded Result in get

[jplatte]

I don't believe this to be possible as is (you cannot prevent multiple traits from having the same type for Target AFAIK)

Yes, this isn't possible.

@sdroege why do you want the generic argument and the return type to be disconnected like that? It could be done by having two traits, but I don't see the point in

let x = Value.get::<i32>().unwrap(); // x is an i32
let x = Value.get::<String>().unwrap(); // x is an Option<String>

[nightmared]

I think the point is to specify the type of the value we want to obtain, but more importantly the right FromTrait to apply. So, it would allow to specify the encapsulated value rather than the specific resulting type. But I agree that from a user perspective having

let x: Option<String> = Value.get().unwrap();
assert_eq!(x), Value.get::<String>().unwrap());

is a quite disturbing behavior and probably a counter-intuitive interface.

[sdroege]

Yeah I'm now also leaning towards specifying the type one would actually get. So get::<Option<String>>() and get::<i32>(). That can then be done with a single trait and quite easily.

The main issue here is then that the compiler error users would get for get::<String>() are not too helpful. For users coming from other languages that might be annoying.

23 |     let x = Value.get::<String>().unwrap();
   |                   ^^^ the trait `FromValue<'_>` is not implemented for `std::string::String`

Not sure why it doesn't even give suggestions for the impls here.

[jplatte]

For reference, it doesn't work out exactly as I expected (need an extra _ because functions can't have generic parameter defaults):

pub trait FromValue<'a> {
    type Target: FromValueTarget<'a>;
    unsafe fn from_value(_: &'a Value) -> Self::Target;
}

pub trait FromValueTarget<'a> {
    type Source: FromValue<'a>;
}

impl<'a> FromValue<'a> for i32 {
    type Target = i32;

    unsafe fn from_value(_: &'a Value) -> i32 {
        unimplemented!()
    }
}

impl<'a> FromValueTarget<'a> for i32 {
    type Source = i32;
}

impl<'a> FromValue<'a> for String {
    type Target = Option<String>;

    unsafe fn from_value(_: &'a Value) -> Option<String> {
        unimplemented!()
    }
}

impl<'a> FromValueTarget<'a> for Option<String> {
    type Source = String;
}

pub struct Value;

impl Value {
    pub fn get<'a, T, U>(&'a self) -> Result<U, ()>
    where
        T: FromValue<'a, Target = U>,
        U: FromValueTarget<'a, Source = T>,
    {
        Ok(unsafe { T::from_value(self) })
    }
}

fn main() {
    let x: i32 = Value.get().unwrap();
    let x: Option<String> = Value.get().unwrap();
    let x = Value.get::<i32, _>().unwrap();
    let x = Value.get::<String, _>().unwrap();
}

[jplatte]

Yeah I'm now also leaning towards specifying the type one would actually get. So get::<Option<String>>() and get::<i32>(). That can then be done with a single trait and quite easily.

The main issue here is then that the compiler error users would get for get::<String>() are not too helpful. For users coming from other languages that might be annoying.

23 |     let x = Value.get::<String>().unwrap();
   |                   ^^^ the trait `FromValue<'_>` is not implemented for `std::string::String`

Not sure why it doesn't even give suggestions for the impls here.

Yeah, I was thinking about that too. rustc has its own attribute for that that can be applied to traits to give a custom error message, but of course that's perma-unstable:

#![feature(rustc_attrs)]

#[rustc_on_unimplemented = "helpful error message"]
pub trait FromValue<'a> {
    unsafe fn from_value(_: &'a Value) -> Self;
}

impl<'a> FromValue<'a> for i32 {
    unsafe fn from_value(_: &'a Value) -> i32 { unimplemented!() }
}

impl<'a> FromValue<'a> for Option<String> {
    unsafe fn from_value(_: &'a Value) -> Option<String> { unimplemented!() }
}

struct Value;

impl Value {
    pub fn get<'a, T: FromValue<'a>>(&'a self) -> Result<T, ()> { unimplemented!() }
}

fn main() {
    let x: i32 = Value.get().unwrap();
    let x: Option<String> = Value.get().unwrap();
    let x = Value.get::<i32>().unwrap();
    let x = Value.get::<String>().unwrap();
}

[jplatte]

I was actually meaning to write an RFC for something like

#[on_unimplemented = "error msg"]
impl !Trait for Type {}

at one point, but never got around to it.

[nightmared]

Negative impls ? Isn't there aleady some work towards it in chalk ?

Edit: sorry I didn't realized you were talking about on_unimplemented instead of the !Trait itself ;( Next time I will read comments more carefully ! That said, if we could have negative impls, we could probably prevent the same Target type from having multiple impl.

[jplatte]

Yes, negative impls for non-marker traits are an interest separately from an on_implemented attribute. They've been implemented as an unstable feature which is being tracked here.

[sdroege]

So I guess for now our options are either 1) two traits and another type parameter for get() that is not very ergonomic

    let x: i32 = Value.get().unwrap();
    let x: Option<String> = Value.get().unwrap();
    let x = Value.get::<i32, _>().unwrap();
    let x = Value.get::<String, _>().unwrap();

or 2) one simple trait and using the target type in both all cases (I wonder if it would make sense to also implement on String then and having that fail with an Err() if the String happens to be None? Maybe that would be a nice compromise)

    let x: i32 = Value.get().unwrap();
    let x: Option<String> = Value.get().unwrap();
    let x = Value.get::<i32>().unwrap();
    let x = Value.get::<Option<String>>().unwrap();

or 3) the status quo with get() and get_some().

Opinions? Vote on this comment with 👍 for 1), 🎉 for 2), 🚀 for 2) plus the part in the parenthesis, ❤️ for 3) and 👎 if you hate it all :)

@GuillaumeGomez, @EPashkin, @jplatte @nightmared, @fengalin, @gdesmott to add a few people here but everybody else please also vote.

[sdroege]

Note that for 2), this is not an option directly because... we can't implement the trait on e.g. Option<Widget> inside gtk because neither Option<_> nor the trait are defined inside gtk. But I have a version of 2) that actually behaves the same way (with a small helper trait).

[nightmared]

Note that for the option 2 with parenthesis, I think if you cannot specify a compile time error (as discussed above), then maybe it shouldn't be done. I really like the "If it compiles it works" approach, especially in Rust, as the type system is great for that ! IMHO Deferring preventable errors to runtime when they can be prevented at compile time is a trade-off that must be considered seriously.

[GuillaumeGomez]

I agree with @nightmared on this one. So at least not option 2.

[nightmared]

@GuillaumeGomez I wouldn't say the option 2 itself is the issue, but rather the ability to report errors (although I'm not quite satisfied with option 2 either). More specifically: the tradeoff between runtime and compile time error.

[sdroege]

You're both misunderstanding option 2 with parenthesis then. There would be no runtime errors, let me explain :) Also type inference would work correctly with 2 in all regards.

// This would work as long as the string is not None, otherwise it returns `Err(glib::value::GetError::UnexpectedNone)` or similar
let x: Result<String, GetError> = v.get::<String>();

// This would always work unless the type is just wrong and it e.g. stores an integer
let x: Result<Option<String>, GetError> = v.get::<Option<String>>();

The user would specify the type they want in the end. Do they want an optional string or do they assume that this string is never None? In case of the second they would've gotten an Ok(None) before and now get an Err(GetError::UnexpectedNone), in either case they would've had to either handle this case (which they can still do the same way) or unwrap/expect (which they also can still do the same way). The main difference is that the requested type is more clear and explicit now instead of magically mapping nullable types to an Option.

Addition: Also users assuming the string can never be None would only have to handle one layer of errors now (Err(_)) instead of two (Err(_) and Ok(None)), so even in that case it seems like a usability improvement to me.

[GuillaumeGomez]

With the full type declaration, it makes more sense. Then 🚀 for me. (option 2 with parenthesis)

[nightmared]

All right, thanks for clearing that up, I definitely misunderstood what you meant ;)

[sdroege]

Ok so I guess we have a winner. I'll look into implementing that. Main problem is the interactions with TypedValue and other things now, but I'm sure that's all solveable somehow.

[sdroege]

gtk-rs/glib#709 was the first part of this. With that out of the way, the traits can be cleaned up now.