Design - Class builder and wrappers in awesome APIs

[AdminXVII]

Design - Class builder and wrappers in awesome APIs

[Timidger]

Thanks for the in depth description @psychon, I'll be sure to point to these comments next time someone asks this.

Gonna go ahead and close this, assuming all your questions are answered @AdminXVII.

[Timidger]

It's been a long strange trip to where we currently are with the object system in place in Way Cooler. Most of it was motivated by how rlua works, so you might want to read up on the limitations that I've mentioned in another comment here and to read the API docs yourself.

Way Cooler actually does use the UserData trait for its objects, We use the class/object builders and traits to make it easier to construct objects, exactly like how C Awesome currently does it.

In particular, look at the Objectable trait, which abstracts away a lot of boiler plate while also stipulating that the thing it's wrapping implements UserData. The reason you won't find it implemented everywhere is because that too has been abstracted out by this macro (search for uses of it to see how to use it).

The object system is heavily abstracted, perhaps too much so, in both Way Cooler and Awesome. It's a teeny bit easier to use here, but just as confusing when you dig into the details.

[AdminXVII]

Thanks for your time.
However, sorry, my question was not asked properly, I was more wondering about the nesting of classes (with for example Tag wrapping an object, wrapping a TagState) and the relative roles of the two wrappers. In my understanding, the top-level wrappers implement a class, the Object class serves as a "boilerplate remover" and the lowermost structs are the real state. However, I do not understand the need for the two top-most wrapper, as the classes are native rust + rlua feature and the general code could be implemented as it is for now in a trait, but without the casting and the wrapping, simply as helpers given by the Object trait.

[Timidger]

I'm not 100% sure I'm following, but if you're asking why are we using AnyUserData and then casting the type it's because that's the value we get from Lua: it can pass any user data and so we need to cast it to the correct type (or signal an error if it's not what we expect).

If I'm still not understanding, apologies. Perhaps some code examples of your proposed structure would help me understand.

[AdminXVII]

Ok now the code follows the structure:

pub struct Tag<'lua>(Object<'lua>);

                   ↓

pub struct Object<'lua> {
    pub object: AnyUserData<'lua>
}

                   ↓

pub struct TagState {
    name: Option<String>,
    selected: bool,
    activated: bool
}

I think this could be expressed as

pub struct Tag {
    name: Option<String>,
    selected: bool,
    activated: bool
}
impl Objectable for Tag{}

where Objectable provides the same functionalities as it provides now, simply redesigned. I could be wrong as I am fairly new to Rust. However, this would mean

fn set_name<'lua>(lua: &'lua Lua,
                  (obj, val): (AnyUserData<'lua>, String))
                  -> rlua::Result<Value<'lua>> {
    let mut tag = Tag::cast(obj.clone().into())?;
    tag.get_object_mut()?.name = Some(val.clone());
    signal::emit_object_signal(lua, obj.into(), "property::name".into(), ())?;
    Ok(Value::Nil)
}

would then become

fn set_name<'lua>(lua: &'lua Lua,
                  (tag, val): (Tag, String))
                  -> rlua::Result<Value<'lua>> {
    tag.name = Some(val.clone());
    signal::emit_object_signal(lua, &tag, "property::name".into(), ())?;
    Ok(Value::Nil)
}

which is a lot more straight-forward (or something along those lines). This would mean we could (I think) drop the Rc and the is statements, which Rust warns against and which is slower than compile-time safety.

Moreover, the allocate method could be relocated to the add_methods function, so the code would follow more closely RLua's intent.

So the question is, why this abstraction? I would like to understand the constrains it solves, as it seems to me as more verbose and not following the guidelines of the library.

Concerning AnyUserData, RLua states 'This API should only be used when necessary. Implementing UserData already allows defining methods which check the type and acquire a borrow behind the scenes.', so this means part of the job of Object is already done, and RLua is designed not to use this.

[psychon]

Sorry if I did not read all of these, but rlua's UserData needs the object to be also Clone to be easily usable. For example, FromLua is implemented for: https://docs.rs/rlua/0.15.3/src/rlua/conversion.rs.html#129-140

impl<'lua, T: 'static + UserData + Clone> FromLua<'lua> for T {

Why is the Clone needed? Just look at line 132 of the link above: Because rlua calls clone() on the object all the time. So, instead of having one tag object, you get lots and lots of copies of the original tag object that then lose all the connections to each other. So, if some property is changed, well, only this one copy of the object gets the property changed.

The way around this is AnyUserData. This gives you a borrow() method that borrows instead of copies.

Coming back to your question:

In the current design, Tag does not actually contain anything. It just has a reference to a value in "Lua land". TagState is what is being referenced here. There can be many Tag instances that all reference the same TagState.

In your proposed version, for example your set_name gets a copy of the Tag that lives in Lua, modifies it, and then the version in Lua still looks the way it did before.

Does this make sense?

(Also, UserData + Clone means that you also lose the value that was saved by set_user_value on the AnyUserData. I don't remember off the top of my head what that was used for in way-cooler, but this is the only possible way that object A can refer to object B and also prevent object B from being garbage collected)

[psychon]

Ahhh, after re-reading, I think my last paragraph is the most important one: set_user_value.

You write:

Concerning AnyUserData, RLua states 'This API should only be used when necessary. Implementing UserData already allows defining methods which check the type and acquire a borrow behind the scenes.', so this means part of the job of Object is already done, and RLua is designed not to use this.

Exercise (I once also tried something like this myself):
Implement the following with rlua:

• You have two kinds of objects, let's say tag and client.
• Both can be created from lua code via e.g. tag() and client().
• A tag has a list of clients that can be set from lua: t.clients = { c1, c2 }
• The list of clients can also be "getted": print(t.clients[1])

Now make this code work correctly:

local t = tag()
-- Check that tag really keeps a client and does not just copy it
do
  local c1 = client()
  t.clients = { c1 }
  assert(c1 == t.clients[1])
  local check_real_equality = {}
  check_real_equality[t.clients[1]] = 1
  assert(check_real_equality[c1] == 1)
end
-- Now check for "tag keeps clients alive"
do
  local c1, c2 = client(), client()
  t.clients = { c1, c2 }
end
collectgarbage("collect") -- Just because c1 and c2 are now out of scope
local clients = t.clients
print(clients[1], clients[2], clients[3], clients[1] == clients[2])

(Really, try this out, it helps a lot)

Since the clients are Lua objects and since this example is designed so that tags need to keep a reference to clients, you can only really do this with set_user_value on AnyUserData. One cannot cheat here and just create a new instance refering to the same client, because the == and the table index would catch this. Thus, assignment to clients needs to be implemented by e.g. having a tag save a table in its set_user_value. This table contains all the clients that are saved in the tag.

[AdminXVII]

Ok, thanks for your reply. Concerning cloning the RLua documentation states "Implementing UserData already allows defining methods which check the type and acquire a borrow behind the scenes", so I thought cloning the values only meant cloning the rust-side values, all pointing to the same Lua object, thus allowing to change the values in clones but keeping the same references, thus exactly the same lua tables.

It is true though that I'd need to experiment with RLua before really contributing to way-cooler, so will-do.

[AdminXVII]

Yes, this covered it I think.

For the record, this is a minimal, stripped-down, working example of my idea:
POC.tar.gz

[psychon]

By the way: Sorry if I sounded a a bit harsh yesterday. That was not my intention. I don't claim nor think that the current object system is perfect or easy to work with and I'd be happy if we can find a way to improve it.

---

I added a global function get_tags() to your example that gets a list of all the existing tags. I also modified the Lua code that is run in main to highlight that this copies the tags instead of just returning references to the already-known tags:

diff --git a/src/main.rs b/src/main.rs
index 3a8d700..6c92c22 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -13,6 +13,13 @@ local t = tag{ clients = {c}, name = "bob" }
 
 assert(t, "Tag not defined")
 assert(t.name == "bob", "name is not updated")
+local tags = get_tags()
+t.name = "kevin"
+assert(t.name == "kevin", "name is not updated")
+for k, v in pairs(tags) do
+  print(k, v.name)
+  assert(v.name == "kevin")
+end
 return(t)
 "#,
        None
diff --git a/src/tag.rs b/src/tag.rs
index 5afefc0..8161e01 100644
--- a/src/tag.rs
+++ b/src/tag.rs
@@ -114,4 +114,14 @@ pub fn init_global(lua: &Lua) {
     //meta.set("emit_signal", lua.create_function(emit_signal)?)?;
     global.set_metatable(Some(meta));
     lua.globals().set("tag", global).unwrap();
-}
\ No newline at end of file
+
+    let func = lua.create_function(|lua, ()| {
+        let result = lua.create_table()?;
+        let tags = lua.named_registry_value::<Table>(TAG_LIST)?;
+        for val in tags.sequence_values::<Value>() {
+            result.set(result.len()? + 1, val?)?;
+        }
+        Ok(result)
+    }).unwrap();
+    lua.globals().set("get_tags", func).unwrap();
+}

Even though the Lua code renames the tag to "kevin", the output from running this is:

1	bob
Err(RuntimeError("[string \"?\"]:11: assertion failed!\nstack traceback:\n\t[C]: in ?\n\t[C]: in function \'assert\'\n\t[string \"?\"]:11: in main chunk"))

If one removes #[derive(Clone)] from struct Tag, then there are two errors. One is in main: This needs Clone to find an implementation of FromLua for tag::Tag for the return value. So, this is code only runs after the error above and does not matter for now.
The other error is in line 109 of tag.rs:

error[E0277]: the trait bound `tag::Tag: std::clone::Clone` is not satisfied
   --> src/tag.rs:109:14
    |
109 |         tags.get::<rlua::Integer, Tag>(tags.len()?)
    |              ^^^ the trait `std::clone::Clone` is not implemented for `tag::Tag`
    |
    = note: required because of the requirements on the impl of `rlua::FromLua<'_>` for `tag::Tag`

The metafunction __call can only return a Tag if the Tag can be cloned for this. Thus, the function that creates tags already causes two instances of Tag to be created: One is saved in the global list of tags and the other is returned to Lua.

This can be worked around in this instance as follows (no idea how to make the code in main work):

diff --git a/src/main.rs b/src/main.rs
index 6c92c22..97ad1f9 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -7,7 +7,7 @@ use rlua::Lua;
 fn main() {
     let lua = Lua::new();
     tag::init_global(&lua);
-    println!("{:?}", (lua.eval(
+    lua.eval::<_, ()>(
        r#"
 local t = tag{ clients = {c}, name = "bob" }
 
@@ -23,5 +23,5 @@ end
 return(t)
 "#,
        None
-    ) as rlua::Result<tag::Tag>));
+    ).unwrap();
 }
diff --git a/src/tag.rs b/src/tag.rs
index 9e01233..d9803fd 100644
--- a/src/tag.rs
+++ b/src/tag.rs
@@ -11,7 +11,7 @@ use rlua::{self, Lua, UserData, UserDataMethods, MetaMethod, Value, Table,
 
 pub const TAG_LIST: &'static str = "__tag_list";
 
-#[derive(Clone, Debug)]
+#[derive(Debug)]
 pub struct Tag {
     name: Option<String>,
     selected: bool,
@@ -106,7 +106,7 @@ pub fn init_global(lua: &Lua) {
         }
         let tags = lua.named_registry_value::<Table>(TAG_LIST)?;
         tags.set(tags.len()? + 1, tag)?;
-        tags.get::<rlua::Integer, Tag>(tags.len()?)
+        tags.get::<rlua::Integer, Value>(tags.len()?)
     }).unwrap()).unwrap();
     meta.set("signals", lua.create_table().unwrap()).unwrap();
     //meta.set("connect_signal", lua.create_function(connect_signal)?)?;

But the problem of "how can a tag refer to a (list of) client(s)?" remains. And, assuming clients are objects, the only way around this that I know is AnyUserData and set_user_value.

[AdminXVII]

Don't worry, you sounded just fine. Just to clarify the intent of the code, this was to serve as a base point for future reference, so if someone has this same idea, they can see the problems you mentioned without too much effort, but maybe it wasn't clear enough. Thanks for your detailed remarks as they further illustrate your comments and the flaws of this approach, and they will help as future reference.