Use of destructed object in lager::lenses::getset

[dimula73]

Hi!

I have found a weird issue in lager::lenses::getset. It looks like both lambdas accept std::forward<decltype(p)>(p), which causes the object to be moved twice. To make this happen, both getter and setter functions should accept a normal object, not a reference.

template <typename Getter, typename Setter>
auto getset(Getter&& getter, Setter&& setter)
{
    return zug::comp([=](auto&& f) {
        return [&, f = LAGER_FWD(f)](auto&& p) {
            // here we call a move constructor for decltype(p) and besically expire p
            return f(getter(std::forward<decltype(p)>(p)))([&](auto&& x) {

               // here we try to do that again, while the object has already been expired
                return setter(std::forward<decltype(p)>(p),
                              std::forward<decltype(x)>(x));
            });
        };
    });
}

Example code:

struct Data : public boost::equality_comparable<Data>
{
    QString id = "default_id";
    QString curve = "default_value";

    Data() = default;
    Data(const Data&) = default;
    Data(Data&&) = default;
    Data& operator=(const Data&) = default;
    Data& operator=(Data&&) = default;

    bool operator==(const Data&rhs) { return id == rhs.id && curve == rhs.curve; }
};

auto testCurveLensNoTuple = lager::lenses::getset(
    [](Data data) -> QString {
        qDebug() << "get" << data.curve;
        return data.curve;
    },
    [](Data data, QString curve) {
        qDebug() << "set" << data.curve << data.id;
        Q_ASSERT(!data.curve.isEmpty());
        data.curve = curve;
        return data;
    });

void test()
{
    lager::state<Data, lager::automatic_tag> state;

    // this line asserts!
    lager::cursor<QString> activeCurve = state.zoom(testCurveLensNoTuple);
    
    // this line works fine
    // lager::cursor<QString> activeCurve = state.zoom(lager::lenses::attr(&Data::curve));

    qDebug() << ppVar(activeCurve.get()) << ppVar(state->curve) << ppVar(state->id);

    activeCurve.set(DEFAULT_CURVE_STRING);

    qDebug() << ppVar(activeCurve.get()) << ppVar(state->curve) << ppVar(state->id);
}

I'm not sure what is the best way to fix that. The simplest solution could be to make a copy of the object in lager::lenses::getset, which doesn't look like the best solution. Perhaps it is possible to implement getset routine without lambdas and avoud this extra copy?

[dimula73]

Looking at the code it seems like lager::lenses::attr should also have this issue, though I cannot reproduce that for some reason.

[dimula73]

There is also an issue that as long as getter has side-effects (e.g. expires the temporary) it is called during the set call, though the result is discarded.

[arximboldi]

That is an excellent catch. I think one solution is to not move into the getter, which would anyway normally use const& for the data. There is a potential unnecessary copy in some set via an update of the projected value but not sure it matters in most practical scenarios.

[dimula73]

Hi, @arximboldi!

My main concern right now is that 'get()' part of the lens is executed during the 'set()' operation. And I don't know how to fix that properly. I tried some deferred calculation approach, but it breaks unittests in multiple places.

[dimula73]

Here is my proof-of-concept implementation, though it breaks lenses::attr severely :(
#161

[arximboldi]

Hey @dimula73 ! Sorry I haven't got back in that PR, I need to look into it more deeply keep getting caught in other stuff.