Changes as part of gtk-rs/gir#354

[johncf]

WIP: Do not merge!

Why is .to_glib() generated? I saw this, but I could not understand the purpose of it. It looks like an unnecessary indirection. What changes should be made in cairo (since there's only one type involved)? Should I simply do:

[EPashkin]

Normally we redefine enums in rust part (https://github.com/gtk-rs/gdk/blob/master/src/auto/enums.rs#L9-L66). It also prevent direct conversion from/to ints for more safety.

Not sure if that needed for Content. IMHO you just need define ToGlib and FromGlib in direct way in cairo

#[doc(hidden)]
impl ToGlib for Content {
type GlibType = Content;
fn to_glib(&self) -> Content {
  *self
}
}

[johncf]

Normally we redefine enums in rust part

I saw that and was wondering why? And the stranger part is the docs for gdk::AxisUse actually points to gdk_sys::GdkAxisUse if you click the src button. Why is that?

It also prevent direct conversion from/to ints for more safety.

How is that? I'm getting an error when doing this:

#[repr(C)]
enum X {
    Hello = 0,
    World = 1,
}
fn main() {
    let x: X = 0 as X;
}

error: non-scalar cast: `i32` as `X`
 --> src/main.rs:7:16
  |
7 |     let x: X = 0 as X;
  |                ^^^^^^

[EPashkin]

Before #144 gdk reexported sys enums.

Strange, before it normally converted and gtk-rs/gir@2941754 need to fix that,
and https://github.com/gtk-rs/gtk/blob/master/src/enums.rs#L10-L15 still compiles normally

[johncf]

https://github.com/gtk-rs/gtk/blob/master/src/enums.rs#L10-L15 still compiles normally

That's the other way round -- enum to i32 -- which is of course fine. i32 to enum was never allowed in Rust as far as I can remember (which is about 1-2 years).

EDIT: Corrected typo (i32 and enum was mistakenly swapped before).

---

I cannot impl ToGlib for Content from cairo since both the type and the trait are remote. I'd have to do it from cairo-sys but that's against the pattern. What should be done? Creating another set of identical enum just seems ugly (WET) to me.

[johncf]

Before #144 gdk reexported sys enums.

I think reexporting sys enums is the right thing to do, if there are no structural difference between them.

[EPashkin]

You right about impl ToGlib for Content, and I don't know how to fix it.
Maybe just copy create_similar_surface to manual window.rs without to_glib
@GuillaumeGomez maybe you have better idea?

[johncf]

Since there are no extra safety achieved by this, wouldn't it be better to remove the whole enum redefining thingy?

(Note: there was a typo in my previous comment)

[GuillaumeGomez]

For now, it's a common base to gave ToGlib implemented on every types. However, we should add a special case for enums because it's obviously just a basic conversion that can be performed with a match.

[EPashkin]

I not remember why we redefine enum in non-sys crate but by https://github.com/gtk-rs/gdk/blob/master/src/auto/enums.rs#L9-L66 example maybe it to get normal names for cases

[EPashkin]

But https://github.com/gtk-rs/sys/blob/master/gdk-sys/src/lib.rs#L28-L42 also seems normal enum

[johncf]

maybe it to get normal names for cases

Those constants are just referencing camel cased enum variants.

[johncf]

we should add a special case for enums

It should probably be done at the gir-for-rust level. Refer to the gir file, if it is an "enumeration", skip emitting to_glib.

[EPashkin]

Yes, we can change https://github.com/gtk-rs/gir/blob/master/src/analysis/conversion_type.rs#L58 to remove to_glib/from_glib and https://github.com/gtk-rs/gir/blob/master/src/codegen/enums.rs to generate reexports but IMHO better wait some time before do this massive change.
Maybe someone remember reason why we make enums that way.

[GuillaumeGomez]

I think it was mostly to have it implemented on every object so we could call to_glib on it.

[EPashkin]

@GuillaumeGomez IMHO not, we have ConversionType::Direct at moment when gtk-rs/gir#229 and gtk-rs/gtk#310 landed year ago.
Maybe comment in gtk-rs/gir#229 and related issues will help to understand why it was done that way

[johncf]

Maybe comment in gtk-rs/gir#229 and related issues will help to understand why it was done that way

Sure, meanwhile here is a PR for it. 😄

[EPashkin]

Seems main reason to redefine is checking returned value

[johncf]

That seems like a good idea at first, but when I think about it, that doesn't make a lot of sense. Additional "safety" provided by doing that seems to be in "preventing gtk from returning bad enumerations due to bugs in it." But gtk-rs is just a wrapper, as such it may not be worth checking for upstream bugs, from a performance point-of-view.

As of now, it just transmutes an i32 into an FFI-side enum which is then converted to Rust-side enum. There's no gain here, since i32 values aren't actually checked.

[EPashkin]

"Checks" done when converting FFI-enum to rust-enum not when converting from i32 to FFI-enum

[johncf]

"Checks" done when converting FFI-enum to rust-enum not when converting from i32 to FFI-enum

But when you transmute an invalid i32 to an FFI-enum, wouldn't it already be in an undefined state? I'm not very sure about this, but transmute will always succeed from what I remember.

[EPashkin]

As I understand this match will panic on not listed values.

[johncf]

No, there's no built-in runtime panic behavior for match block. (At least I haven't read anything like that in the docs. The compiler checks for exhaustiveness at compile-time, that's it. Beyond that, at runtime, you are on your own. Which would mean that incorrect mem::transmute-s will have consequences!)

[EPashkin]

@johncf you right. Match not panic on wrong values but go to one branch ("B" in my test case).
@GuillaumeGomez seems current version of enum's FromGlib really useless

#[repr(C)]
pub enum Test {
   A = 0,
   B = 1,
}
    let v: Test = unsafe { std::mem::transmute(2) } ;
    match v {
        Test::A => println!("A"),
        Test::B => println!("B"),
    }

[GuillaumeGomez]

That's what I said, we should write a full match block on the incoming value so we can "generate" the correct rust value.

[johncf]

seems current version of enum's FromGlib really useless

Let's ditch it! 😄 Tons of lines of code will be saved! (-1.6kloc just for this repo!!) And also result in lot more simplified code!

That's what I said, we should write a full match block on the incoming value so we can "generate" the correct rust value.

No, performance overheads!! 😢

[GuillaumeGomez]

Well, it doesn't like a good idea to just transmute any value. :-/

[EPashkin]

@GuillaumeGomez what you mean about full match?
In my example it was full, all other cases just ignored with unreachable pattern.
Also debug print also think that it last pattern

println!("{:?}", v);   //B
println!("{:?}", v as i32); //2
assert_eq!(v, Test::B); //this fails

[johncf]

@GuillaumeGomez Using Rust, people like to do strong compile-time checks so as to avoid certain runtime checks thereby gaining performance. But in our case, since we are in the ffi boundary, there's a limit to what compile-time checks can accomplish. I'm very much against sacrificing runtime performance for doing upstream bug-checks. (transmuting is fine as long as we assume that upstream gtk library is bug-free, which is the assumption under which most "wrappers" work.)

[GuillaumeGomez]

Hum... The comparison should work normally... Strange.

[GuillaumeGomez]

transmuting is fine as long as we assume that upstream gtk library is bug-free

That's my main concern: the day we have a bug, debugging this will be terrible. Can we at least add a check before transmuting?

[EPashkin]

What type of check? IMHO we can't check that i32 not present in enum in general

[johncf]

what you mean about full match?

This is what I'm thinking he's referring to:

#[repr(C)]
pub enum Test {
   A = 0,
   B = 1,
}

fn toTest(v) -> Test {
    match v {
        0 => Test::A,
        1 => Test::B,
        _ => panic!("Bad!"),
    }
}

[GuillaumeGomez]

@johncf: Exactly.

[EPashkin]

all other cases just ignored with unreachable pattern.

[EPashkin]

and still print "B"

[GuillaumeGomez]

The comparison is another issue from my point of view.

[EPashkin]

@johncf By way, wrong i32 may be not from bug in upstream, but also when upstream add new case to enum

[johncf]

That's my main concern: the day we have a bug

This is everyday! 😄

In my opinion, what gtk-rs should strive to accomplish is:

1. Not introduce another layer of bugs.
2. Make it less likely for the user to write buggy programs.

Those two goals are hard enough to achieve. Once they are achieved 🤞 , perhaps we can think about preventing upstream bugs too.

[johncf]

By way, wrong i32 may be not from bug in upstream, but also when upstream add new case to enum

They are essentially API changes, which means versions change.

[GuillaumeGomez]

That's my main concern: the day we have a bug

This is everyday! 😄

Don't only take the parts you want. 😝 The last part was the important one: don't make bug tracking harder.

@johncf By way, wrong i32 may be not from bug in upstream, but also when upstream add new case to enum

In libc, they check if their code matches the C headers on the platform. Maybe we could take a look at how they do it and try a similar thing, no?

[EPashkin]

IMHO it impossible in case of check enum values

[johncf]

Don't only take the parts you want. 😝 The last part was the important one: don't make bug tracking harder.

Sorry, if I sounded argument-y... And yeah, bugs beyond ffi boundary will be harder to track. But do you agree with those two goals? Oh, and also:

3. Provide very similar performance!!

[GuillaumeGomez]

Maybe I wasn't clear enough: I agree with the transmute change, it's a good idea. My main concern is principally: if an user gets an invalid converted value, then Rust safety is just gone. And it seems quite simple for it to happen: you use the crate of someone targetting 3.8 and you have a superior version which inserted a value inside the enum. You got the jackpot.

[johncf]

From my point of view, much of your concerns should not be taken care of from a wrapper (even if the wrapper is written in a language which promises safety). Those safety guarantees must be built ground up -- the underlying library should be so designed that languages with the highest safety guarantees will be able to withhold those guarantees. A C-library written without that in mind can only be so much safe... (To be honest, I'm quite happy that most gtk libraries come with those gir files describing a lot more than what the language could, though still far from complete. And I'm very happy that using Rust, we can actually make the compiler do many of the checks written in those gir files!)

targetting 3.8

Version? Of course the wrapper should be very much concerned about version differences, and provide correct behavior across them. This is a very different topic, though.

[EPashkin]

If we still prefer checked FromGlib we can do it with int conversion

    match v as i32 {
        0 => println!("A"),
        1 => println!("B"),
        _ => panic!("Bad value"),
    }

[johncf]

After some long careful considerations, I've come to the conclusion that I'll be a lot more productive if I shifted the GUI development to Vala, and form a feature-specific FFI interface between Vala and Rust, which should be very easy. Vala, being specifically designed for GObject, simply provides a lot of convenience when it comes to Gtk development. The convenience and maturity of Vala ecosystem far outweighed all arguments raised by the Rust-fanboy in me.

Anyway, I'll see to the conclusion of this PR. So feel free to suggest changes or close this, after the maintainers have reached a decision.

[GuillaumeGomez]

I was thinking about a way to make everyone happy: adding an unchecked method which uses transmute meanwhile the to_glib will use a match statement. After this, we'll need to see at the upper level, how to decide which solution to use. Still no idea on this part...

[johncf]

Updated to reflect changes from gtk-rs/gir#354

[johncf]

I think adding cairo.Content can wait for a separate PR... :) I'm renaming this appropriately...