Var now supports #[var(pub)]; add SimpleVar to reuse existing Godot conversions

[Bromeon]

User-facing API

Changes the Var trait from:

pub trait Var: GodotConvert {
    fn get_property(&self) -> Self::Via;
    fn set_property(&mut self, value: Self::Via);

    fn var_hint() -> PropertyHintInfo { ... }
}

to:

pub trait Var: GodotConvert {
    type PubType;

    fn var_get(field: &Self) -> Self::Via;
    fn var_set(field: &mut Self, value: Self::Via);

    fn var_pub_get(field: &Self) -> Self::PubType;
    fn var_pub_set(field: &mut Self, value: Self::PubType;

    // Unchanged:
    fn var_hint() -> PropertyHintInfo { ... }
}

Adds SimpleVar trait. Types that already support ToGodot/FromGodot can benefit from a fast-track Var impl reusing existing conversions:

impl SimpleVar for InstanceId {}

Semantic changes

• Closes Generated enum getters and setters should return the enum type in Rust #1358.
• Closes Engine enums/bitfields in #[var], #[func], #[signal] #1172.
• Back the #[var(pub)] attribute with two new methods in Var.
• Renames {get,set}_property to var_{get,set} and turn methods into associated functions.
  • Reduces namespace pollution for "extension" functions: every godot-rust type used to suggest .get_property()/.set_property() in IDE although that's almost never what you want. It was also an overly generic name.
• Improve compiler error spans when #[var(pub)] fields are missing required trait bounds.
• Add InstanceId and AnyArray impls to Var.

Refactors

• Reorganize Var impls, reducing a lot of code duplication. It turns out that 90% of our Vars were simply delegating to FromGodot + ToGodot. The marker trait SimpleVar reduces a lot of repetition here.
• Simplify GetterSetterImpl::from_generated_impl() logic.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1466

[Bromeon]

Design iteration 1

Original design in this PR, ready in a99e9b9.

VarPub trait

A VarPub trait that is used to determine the type passed to generated Rust setters/getters, i.e. in #[var(pub)].

pub trait VarPub: Var {
    /// Type of the value returned by the getter, and accepted by the setter.
    type Type;

    /// Implementation of the getter generated by `#[var(pub)]`.
    fn var_pub_get(property: &Self) -> Self::Type;

    /// Implementation of the setter generated by `#[var(pub)]`.
    fn var_pub_set(property: &mut Self, value: Self::Type);
}

impl<T: Var + Clone> VarPub for T {
    type Type = Self;

    fn var_pub_get(property: &Self) -> Self::Type {
        property.clone()
    }

    fn var_pub_set(property: &mut Self, value: Self::Type) {
        *property = value;
    }
}

The blanket impl is supposed to avoid having to redefine this for most cases where the field type itself is returned. But there are problems:

• I originally thought we'd only need this trait when #[var(pub)] is supported, but in fact it has another use: verifying the type of setters/getters. This also needs to happen when the user provides getters/setters -- which is currently not done, and a bug. Of course this could technically be yet another trait, but it's already complex.
• Using standard traits as bounds (here Clone) means we don't have very good control when the blanket impl applies -- it would prevent types that need custom VarPub from implementing Clone.
• This is a problem with PhantomVar<T> already, which is Clone but needs a custom impl (extracting the T). We can't work around this without removing Clone or adding a custom bound instead of Clone -- but then users need to manually implement VarPub again for every single type.

VarByGodotConvert blanket-provider trait

The original design also foresaw a special trait VarByGodotConvert, which could be implemented instead of Var:

pub trait VarByGodotConvert: ToGodot + FromGodot {}

impl<T: VarByGodotConvert> Var for T
where
    T::Via: Clone,
{
    fn get_property(&self) -> Self::Via {
        self.to_godot_owned()
    }

    fn set_property(&mut self, value: Self::Via) {
        *self = FromGodot::from_godot(value);
    }
}

I think this is generally a good idea, but the overall design needs some rework.

[Bromeon]

Design iteration 2

I tried a different, more modular approach:

pub trait VarAccess<T> {
    type Value;

    fn var_get(field: &T) -> Self::Value;
    fn var_set(field: &mut T, value: Self::Value);
}

pub trait Var: GodotConvert {
    /// Used for internal conversion -- replaces `get_property`/`set_property` impls.
    type GodotAccess: VarAccess<Self, Value = Self::Via>;

    /// Type used in generated getter/setter (`#[var(pub)]`).
    type PubAccess: VarAccess<Self>;

    /// Unchanged.
    fn var_hint() -> PropertyHintInfo { ... }
}

/// Utility methods and types (maybe hidden):
pub type VarGodotAccess<T> = <<T as Var>::GodotAccess as VarAccess<T>>::Value;
pub type VarPubAccess<T> = <<T as Var>::PubAccess as VarAccess<T>>::Value;

pub fn var_godot_get<T: Var>(field: &T) -> VarGodotAccess<T>      { T::GodotAccess::var_get(field)    }
pub fn var_godot_set<T: Var>(field: &mut T, v: VarGodotAccess<T>) { T::GodotAccess::var_set(field, v) }
pub fn var_pub_get<T: Var>(field: &T) -> VarPubAccess<T>          { T::PubAccess::var_get(field)      }
pub fn var_pub_set<T: Var>(field: &mut T, v: VarPubAccess<T>)     { T::PubAccess::var_set(field, v)   }

Then we would have some building blocks for the "access" strategies:

/// Access that reuses existing Godot conversion: returns `Via`, uses `ToGodot`/`FromGodot`.
pub struct VarAccessVia;

impl<T> VarAccess<T> for VarAccessVia
where
    T: ToGodot + FromGodot,
    T::Via: Clone,
{
    type Value = T::Via;

    fn var_get(field: &T) -> Self::Value {
        field.to_godot_owned()
    }

    fn var_set(field: &mut T, value: Self::Value) {
        *field = FromGodot::from_godot(value);
    }
}

/// Access by using `Self` directly (not through `Via`).
pub struct VarAccessSelf;

impl<T: Clone> VarAccess<T> for VarAccessSelf {
    type Value = T;

    fn var_get(field: &T) -> Self::Value {
        field.clone()
    }

    fn var_set(field: &mut T, value: Self::Value) {
        *field = value;
    }
}

Most types would then simply need something like this -- Which also covers enums (one of the reasons for this separation), which have i64 via but Self pub get/set:

impl Var for MyType {
    type GodotAccess = VarAccessVia;
    type PubAccess = VarAccessSelf;
}

However, the custom impls are quite a bit more involved...

impl<T: Var> Var for PhantomVar<T> {
    type GodotAccess = ...;
    type PubAccess = VarAccessNone;
}

pub struct VarAccessNone;
impl<T> VarAccess<T> for VarAccessNone {
    type Value = std::convert::Infallible;

    fn var_get(_field: &T) -> Self::Value {
        unreachable!("VarAccessNone should not generate public getters")
    }

    fn var_set(_field: &mut T, _value: Self::Value) {
        unreachable!("VarAccessNone should not generate public setters")
    }
}

pub struct VarAccessOnEditor<T>(PhantomData<T>);

impl<T: Var + FromGodot + PartialEq> VarAccess<OnEditor<T>> for VarAccessOnEditor<T>
where
    T::Via: BuiltinExport,
{
    type Value = T::Via;

    fn var_get(field: &OnEditor<T>) -> Self::Value {
        field.get_property_inner().expect("...")
    }

    fn var_set(field: &mut OnEditor<T>, value: Self::Value) {
        field.set_property_inner(Some(value));
    }
}

Overall, the indirection is flexible but makes things quite a bit more complex -- in the cases where people do need to customize Var (addmitedly rare), they need to implement 3 traits and define 2 new tag types. It's a bit much for just allowing to use a field in #[var].

I'll look further...

[Bromeon]

Design iteration 3

At last, I converged towards an API with which I'm somewhat happy. Explanation in the initial post 🙂

[Yarwin]

Similarly as in follow-up PR – OnEditor panics on deref (semantically – this value can't be None/null on runtime, trust me bro) thus setters/getters must delegate this functionality to Var (deref will panic).

[Yarwin]

I can't say much without testing it thoroughly in the practice but I think it is fine?

[Bromeon]

One thing that's not great is that #[derive(Var)] now requires Self: Clone because the generated var_pub_get() impl must return something by value.

That was already somewhat the case before:

• For types where Self = Via, the generated getter also cloned. It invoked to_godot().
  Example, builtins even used Copy:

  gdext/godot-core/src/meta/godot_convert/impls.rs

  Lines 259 to 265 in 5a7b65c

  	impl ToGodot for $T {
  	type Pass = meta::ByValue;
  	fn to_godot(&self) -> Self::Via {
  	*self
  	}
  	}

• For enums, we had Via = i64, so there was never an enum type returned in Rust. Thus Clone was not required.

Potential solutions -- likely (1) is the winner for near future as it's not a huge deal in practice.

1. Call it good enough, SimpleVar impls anyway have Clone. For enums, it's often reasonable to have cloneability as well.
2. Return a reference -- this comes with a whole bunch of complexity, see old ToGodot::ToVia<'v>.
3. Use a slow hack like from_godot(to_godot(x)).
4. Allow to tweak #[derive(Var)] implementation.