billyquith / ponder

C++ reflection library with Lua binding, and JSON and XML serialisation.
http://billyquith.github.io/ponder/
Other
640 stars 93 forks source link

UserObject does not enforce dynamic type safety when converting within class hierarchy. #47

Open j-zeppenfeld opened 8 years ago

j-zeppenfeld commented 8 years ago

I've been experimenting with ponder to decide whether to use it for an upcoming project, and have run into a major problem: missing dynamic type safety when converting types within a class hierarchy.

Given a derived class (Derived) inheriting from a base class (Base), constructing a ponder::UserObject of class Base allows it nonetheless to be gotten as a Derived.

#include <ponder/classbuilder.hpp>
#include <ponder/pondertype.hpp>

#include <iostream>

class Base {
public:
    Base() { std::cout << "Base():" << this << std::endl; }
    virtual ~Base() { std::cout << "~Base():" << this << std::endl; }
};

PONDER_TYPE(Base)

class Derived : public Base {
public:
    Derived() {
        std::cout << "Derived():" << this << std::endl;
    }
    Derived(Derived const &other) {
        std::cout << "Derived(Derived const &" << &other << "):" << this << std::endl;
    }
};

PONDER_TYPE(Derived)

int main() {
    ponder::Class::declare<Base>("Base")
        .constructor<>()
        .constructor<Base const &>();

    ponder::Class::declare<Derived>("Derived")
        .constructor<>()
        .constructor<Derived const &>()
        .base<Base>();

    ponder::Class const &metaclass = ponder::classByName("Base");
    ponder::UserObject obj = metaclass.construct();

    // ISSUE: The following line calls Derived's copy constructor but passing the Base instance.
    Derived derived = obj.get<Derived>();

    metaclass.destroy(obj);
}
billyquith commented 8 years ago

I guess this could be just seen as a user bug.

obj.get<TargetType>() static casts to the target type. UserObject is a runtime type, which is opaque, type-wise, to the user. This is legal C++, but allows a code bug.

To use polymorphism with Ponder, the PONDER_POLYMORPHIC() macro should be added to the classes.

I guess there are several ways to approach this if we want to make it type-safe:

What are your expectations?

[EDIT] corrected name of polymorphic macro for current mainline.

j-zeppenfeld commented 8 years ago

I would have expected obj.get<T>() to throw a ClassUnrelated exception if the stored object cannot be implicitly converted to type T. For any conversions that would require an explicit cast in the base language I would expect to find an explicit cast (and be it just a cast rather than get method) in ponder. The language would indicate a violation with a compile-time error, ponder could obviously only do so at run-time, ideally with an exception.

What would be the currently accepted method to check if a UserObject or Value can be converted to a certain type within a class hierarchy? Value::isCompatible doesn't seem to work either.

billyquith commented 8 years ago

What would be the currently accepted method to check if a UserObject or Value can be converted to a certain type within a class hierarchy? Value::isCompatible doesn't seem to work either.

I don't think there is a way of doing that currently. Ponder is a fork of CAMP. I replaced Boost with C++11 as an exercise in learning some C++11. The API is documented but there aren't really any usage examples, so I'm sort of figuring it out as I go along.

Value::isCompatible doesn't seem to work either

This is for testing whether you can do a Value (variant) conversion, not if a UserObject is castable.

Could add methods to copy things, thus avoiding the casting problem?

virtual Base* Base::clone() = 0;

You're welcome to submit a patch, although I think we're talking about emulating what the compiler would do at compiler-time, at runtime, and with multiple inheritance etc, it gets a little more complicated. I assume something like "safe cast", downcast, and upcast need adding. If you find other libraries with better solutions I'd welcome feedback.

I'm going to add this as an enhancement. It is pretty low priority for me. Other work in progress (in various branches).