Extend bound classes with custom methods in Lua #114
Comments
|
Check out Odd that you should write today, but I just discovered that mixins are broken out of the box with LB3. That is because of security around letting scripts access If you find a solution that does not require exposing the metatables to script, let us know. I've been looking at whether some sort of forwarding is possible, but my Lua API chops will take some polishing to get there, I expect. |
|
Setting a static Regarding the metatables access, this is something i will review shortly. |
The way function mixin_private.apply_mixin_foundation(object)This a cool technique, but it definitely raises security concerns. It really depends how much you trust the Lua code that will be executing it. |
In my experimental code first I tried to just create static And then I do And then it works this way for calling added method on instance. About |
|
Yes this makes sense, i think it's a good approach for what you want to achieve. |
|
The main thing about |
|
@rpatters1 you mean which parameters exactly? In this particular code I don't do any calls from C++ code, I just store reference to custom class method (basically Lua function) passed to Currently for our project I implemented solution which in the end makes me add 3 lines of code to each class I plan to extend in Lua and so far it looks sufficient to me, which is why I quesiton myself whether I need any more help of third-party library like @kunitoki just to be clear, is it not technically possible to make it not needing to make custom |
|
The advantage of rewriting the metatables is that you don't need to change the C++ code to extend your classes. You can do it in pure Lua. But that's also the disadvantage, in that your classes can be extended without their/your knowledge, so you have to trust the code that's doing it. I am just referring to mixin as an example of how to do this. You would have to write it yourself. That library is specific to the Finale Lua environment. |
|
I would make it selectable somehow, you don't always want classes to be extensible. But definately can be abstracted away completely and encapsulated by luabridge. Regarding security, i think only allowing to change the __gc for a custom userdata is a potential risk, but i'm sure injecting a malicious method (if you allow bytecode, unguarded i/o, loading compiled lua libraried at runtime and anything reported here as unsafe) it could in theory keep the door open for malicious code. Just hiding metatables isn't enough, if you want better security you should look at compiling the lua vm into a wasm binary with only a subset of wasi exposed (so any access to host memory is guarded). |
|
This is more an issue with the LuaBridge library here, and should be addressed in LuaBridge, without needing any 3rd party stuff, like that mixin library to be involved. It shouldn't even be considered as a viable solution to the issue at hand here. |
|
Well there are no issues in luabridge regarding this and @dtroitskiy already shown that is possible with the available tools. This is more of a request for making things nicer from an api perspective. |
|
@dtroitskiy it would be nice if you could run some testing on #116 I will run some test on explicit metamethods overrides (like overriding __gc and the like). |
|
Tested it, great stuff! At first I didn't know what I need to modify in my code, then saw in tests it's just one option I still get |
Yes this is next step, will work on it. For now i just wanted to have methods working. But this could be a bit more tricky to get right, as we have a userdata (not a table) for |
|
Is using additional storage an option, using the userdata as a weak key? Bound classes can have properties that are readable or readable/writable and also only accept certain types. Is it possible to have the same level of control with custom properties without using |
|
I'm not a fan of global storage, and i wouldn't see myself using a table in the type metatable to store instance data, or a table in the registry indexed by type or by weak userdata |
|
Hm, yeah, I understand the problem. In my current code I wrote before the improvements I basically followed this example https://kunitoki.github.io/LuaBridge3/Manual#27---index-and-new-index-metamethods-fallback (with the difference I also added static With the new solution in LuaBridge I understand that without modifications in C++ code it's not easy to determine what is the best way to store custom properties. If some global storage is not a good option, what about if having a container for custom properties becomes mandatory for C++ class? Yes, this adds a bit of manual work for C++ side, but also makes the code cleaner, I suppose. You can, of course invent any other name instead of |
|
I was thinking that when i allocate the userdata to hold the class instance, i could in theory use a special |
|
Not possible with extensible classes |
Sounds good, there is no need for these functions then. |
|
Apparently it doesn't seem possible, replacing __index and __newindex directly is not possible when using extensible classes, so i will have to revert it. |
|
I think for now, this gives the best flexibility (in term of storage and customizability of the properties lookup, as you might have a map or an unordered_map, or a custom other container, imagine you want to use a QHash when using Qt and so on...). Methods is different as they are stored in the class metatable anyway, but properties don't. luabridge::getGlobalNamespace(L)
.beginClass<ExtensibleBase>("ExtensibleBase", luabridge::extensibleClass)
.addConstructor<void(*)()>()
.addFunction("baseClass", &ExtensibleBase::baseClass)
.addIndexMetaMethod([](ExtensibleBase& self, const luabridge::LuaRef& key, lua_State* L)
{
auto metatable = luabridge::getGlobal(L, "ExtensibleBase").getMetatable();
if (auto value = metatable[key])
return value.cast<luabridge::LuaRef>().value();
auto it = self.properties.find(key);
if (it != self.properties.end())
return it->second;
return luabridge::LuaRef(L, luabridge::LuaNil());
})
.addNewIndexMetaMethod([](ExtensibleBase& self, const luabridge::LuaRef& key, const luabridge::LuaRef& value, lua_State* L)
{
self.properties.emplace(std::make_pair(key, value));
return luabridge::LuaRef(L, luabridge::LuaNil());
})
.endClass();
runLua(R"(
function ExtensibleBase:test()
return 41 + self.xyz + self:baseClass()
end
local base = ExtensibleBase()
base.xyz = 1000
result = base:test()
)");Later on i can see if providing a |
|
I do have one concern tho.
So in this case I push my stored value on the stack and then it gets picked up my LuaRef again which is a bit unnecessary. |
|
Indeed, but you can also implement a simple lua_CFunction for the index + newindex fallback metamethods and manage that with simple lua calls. That is user code, luabridge assumes it is just a callable supported, and because LuaRef can be obtained from values on the stack, that works with it |
Yep, that's what I basically implemented as my first solution. The only inconvenience here is that, as I wanted extensibility to be shared across many classes, I made it a base class, and then you have to somehow provide the name for By the way, |
|
No, there is no multiple inheritance support for binded c++ classes in luabridge, but definately this is something that could be added with minimal effort. |
|
I thought re-opening this would be better than creating new issue, since it's quite relevant. But turned out we're unable to store custom class properties, i.e. static properties. It fails with exception in At the same time, if I do it the old way like I did before with |
|
mmmh it really depends on how you are resolving the call, can you use |
|
If I call |
|
Or if I add it as a first line in |
|
Which line in lua produces this ? i suspect this is accessing |
|
I have this test working. Can we modify this test in order to check all your specific edge cases ? TEST_F(ClassExtensibleTests, IndexFallbackMetaMethodFreeFunctorOnClass)
{
auto indexMetaMethod = [&](OverridableX& x, const luabridge::LuaRef& key, lua_State* L) -> luabridge::LuaRef
{
auto it = x.data.find(key);
if (it != x.data.end())
return it->second;
return luabridge::LuaRef(L);
};
auto newIndexMetaMethod = [&](OverridableX& x, const luabridge::LuaRef& key, const luabridge::LuaRef& value, lua_State* L) -> luabridge::LuaRef
{
x.data.emplace(std::make_pair(key, value));
return luabridge::LuaRef(L);
};
luabridge::getGlobalNamespace(L)
.beginClass<OverridableX>("X", luabridge::extensibleClass)
.addIndexMetaMethod(indexMetaMethod)
.addNewIndexMetaMethod(newIndexMetaMethod)
.endClass();
OverridableX x, y;
luabridge::setGlobal(L, &x, "x");
luabridge::setGlobal(L, &y, "y");
runLua(R"(
X.xyz = 1
function X:setProperty(v)
self.xyz = v
end
function X:getProperty()
return self.xyz
end
y.xyz = 100
x:setProperty(2)
result = x.xyz + y.xyz
)");
ASSERT_EQ(102, result<int>());
} |
|
For example, overwriting a lua extended method should be possible or not ? runLua(R"(
function X:property(v)
self.property = v
end
x:property(2)
result = x.property
)");
ASSERT_EQ(2, result<int>()); |
No, it's without Calling |
|
I pulled latest changes in your branch, and at first I was a bit confused, because I noticed prints 1, 1, 1, 2 for me, as it should. Thus class property remains unchanged, while instance property gets changed (or, more precisely, added to the internal container) after calling About
You mean overriding methods assigned from C++ or something else? That's what Honestly can't think of other edge cases right now, I think I can only report again if we stumble upon something else. For now things look good. |
|
About metatable access, seems like it becomes irrelevant if we're able to achieve what we wanted without additional customizations, but still would be nice to know how it works, did I do something wrong in my test or not. |
|
I overlooked one small issue, my bad, I should have tested this right away. But I suppose it's going to be a quick fix. So, in my code I use base Now the actual issue is that handler assigned by class Extensible
{
public:
LuaRef getProperty(const LuaRef& key, lua_State* L)
{
auto it = properties.find(key);
if (it != properties.end())
{
return it->second;
}
return LuaRef(L, LuaNil());
}
LuaRef setProperty(const LuaRef& key, const LuaRef& value, lua_State* L)
{
properties[key] = value;
return LuaRef(L, LuaNil());
}
private:
unordered_map<LuaRef, LuaRef> properties;
};
class Test : public Extensible
{
public:
inline Test() {};
};
// ...
getGlobalNamespace(L)
.beginClass<Extensible>("Extensible", extensibleClass)
.addIndexMetaMethod(&Extensible::getProperty) // <-- Doesn't get called.
.addNewIndexMetaMethod(&Extensible::setProperty) // <-- Called fine.
.endClass()
.deriveClass<Test, Extensible>("Test", extensibleClass)
.addConstructor<void()>()
.endClass();And the Lua code is the same as before. |
|
I just tried it now and it works for me, this is the scripts i executed (i just renamed Test with DerivedExtensible): function DerivedExtensible:getProperty()
return self.property
end
function DerivedExtensible:setProperty(value)
self.property = value
end
local test = DerivedExtensible()
test:setProperty(2)
result = test:getProperty()and local test = DerivedExtensible()
test.property = 3
result = test.propertyin both cases, the parent |
Yes, i will think on a way to make this transparent, maybe we don't even require to enter into c++ and we could pin the properties in a hidden lua table attached to the instance. |
|
This is the test: namespace {
class NonExtensible
{
public:
luabridge::LuaRef getProperty(const luabridge::LuaRef& key, lua_State* L)
{
auto it = properties.find(key);
if (it != properties.end())
return it->second;
return luabridge::LuaRef(L);
}
luabridge::LuaRef setProperty(const luabridge::LuaRef& key, const luabridge::LuaRef& value, lua_State* L)
{
properties.emplace(key, value);
return luabridge::LuaRef(L);
}
private:
std::unordered_map<luabridge::LuaRef, luabridge::LuaRef> properties;
};
class DerivedExtensible : public NonExtensible
{
public:
inline DerivedExtensible() {};
};
} // namespace
TEST_F(ClassExtensibleTests, IndexAndNewMetaMethodCalledInBaseClass)
{
luabridge::getGlobalNamespace(L)
.beginClass<NonExtensible>("NonExtensible")
.addIndexMetaMethod(&NonExtensible::getProperty)
.addNewIndexMetaMethod(&NonExtensible::setProperty)
.endClass()
.deriveClass<DerivedExtensible, NonExtensible>("DerivedExtensible", luabridge::extensibleClass)
.addConstructor<void()>()
.endClass();
runLua(R"(
function DerivedExtensible:getProperty()
return self.property
end
function DerivedExtensible:setProperty(value)
self.property = value
end
local test = DerivedExtensible()
test:setProperty(2)
result = test:getProperty()
)");
EXPECT_EQ(2, result<int>());
runLua(R"(
local test = DerivedExtensible()
test.property = 3
result = test.property
)");
EXPECT_EQ(3, result<int>());
}
|
|
I just checked, again it happens when you have |
Yep, I was also thinking about it today, that if you can create new table for each instance of a class and somehow associate that table with that instance, then C++ container won't be necessary. |
|
But they shouldn't be called, the index / new index metamethods enter into action when accessing an instance of Test = {
__newindex: function() print(1); end -- This is called on global `Test` table
}
setmetatable(Test, {
__newindex: function() print(2); end -- This is called on an *instance* of Test
})
Test.property = 1 -- Prints 1
t = Test.new() -- Assuming we have a constructor
t.property = 1 -- Prints 2 |
|
No, no, I understand all that, I don't expect |
|
So, to be 100% clear, in your test if you add this line: I didn't run this myself, but as I'm having totally the same code, I'm pretty sure the test will just fail, while it shouldn't. |
|
I understood, but it's a problem of how should i interpret an assignment to the "static" table (like DerivedExtensible): DerivedExtensible.property = 1
-- and `function DerivedExtensible:getProperty() end` which will be rewritten as:
DerivedExtensible.getProperty = function(self) endThey are exactly the same, and both will end up writing into the properties I'm considering completely removing the fallback metamethods and handle it completely inside the extensibleClass. Let's see what i can come up with. |
|
Have hacked a new version which should work on these edge cases, but i'm pretty sure there are other incompatibilities, especially with properties, will do more tests later on |
|
Ok, #139 works for my needs, I guess it should be fine for now, but if you would manage to improve it further, it would be cool. About
isn't it strange in this case that only |
|
It's a bit complicated because of the way lb3 manages registered classes and their metatables, but that was a problem with the static vs instance invocation. |
|
@kunitoki, a question. Derived = {}
setmetatable(Derived, getmetatable(Base))but then if I do function Derived:someMethod()
--
endthis method also gets defined on |
|
I don't think this is actually a possibility that have been thought so far, it might work but there we need to actually introduce some kind of internal registration for the C++ side. Applying a c++ bound metatable of a class exposed as a metatable of a non c++ bound lua "class" might introduce all sort of issues that i cannot predict right now. I will think about it if we have an easy way to allow it, but still making luabridge internals know about the lua class "deriving" from the c++ class... |
|
I see, well, I'd appreciate if you look into it, because I made a workaround for my needs that kinda wraps C++ class into Lua class (just a table), and, although it's fine for the most part, it still doesn't look very good. |
|
Hey @kunitoki! For a long time I haven't posted any new issues, but now I have to get back again to this long-suffering issue =) So, one thing I noticed recently with function Base:init()
print('Base:init()')
end
function Derived:init()
print('Derived:init()')
end
local base = Base()
base:init() -- will call Derived:init()And we assume here that it was To fix it, at first I didn't want to bother you, but wanted to use custom property container once again, since I use it anyway for custom instance properties, but this time I wanted to use it for static properties (like these methods). So I got back to using static auto staticProperties = newTable(luaState);
getGlobalNamespace(luaState)
.beginClass<Color>("Color", visibleMetatables)
.addStaticFunction(
"__index",
[](const LuaRef& classTable, const LuaRef& key) -> LuaRef
{
const auto& value = classTable.getMetatable()[key];
if (!value.isNil())
{
return value;
}
return staticProperties[key];
}
)
.addStaticFunction(
"__newindex",
[](const LuaRef& classTable, const LuaRef& key, const LuaRef& value)
{
staticProperties[key] = value;
return LuaRef(LuaState::get());
}
)But the problem I encountered with this approach is that now So I think it would be nice, and I might have suggested this before, to add About the initial problem. Currently I removed To conclude, personally for me having If you need me to provide a complete example with these issues illustrated, I can, for now was a bit busy to do that. |
|
Hey there ! I see, yes for static properties to work reliably we need to have |
In our project we need to extend classes bound from C++ with custom methods in Lua.
It may look like the following:
When this is tried without any additional C++ code modifications, it result in an error
No writable member 'doSomethingSpecial'.I did some experimentation based on this docs section
https://kunitoki.github.io/LuaBridge3/Manual#27---index-and-new-index-metamethods-fallback
and turns out I can handle calling custom method by using
addIndexMetaMethod, but to actually add that custom method I had to use.addStaticFunction("__newindex", ...), which I'm not even sure is correct to use, because it's not documented. As method is added for class binding table, not for instance,addNewIndexMetaMethodhas no use here.I can share my experimental code if it helps, but maybe correct solution to this problem is known already, so I'd like to ask about it first.
The text was updated successfully, but these errors were encountered: