Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dimula73 opened this issue Oct 12, 2022 · 6 comments · May be fixed by #165
Open

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

dimula73 opened this issue Oct 12, 2022 · 6 comments · May be fixed by #165

Comments

@dimula73
Copy link
Contributor

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
Copy link
Contributor Author

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

@dimula73
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@arximboldi
Copy link
Owner

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.

dimula73 added a commit to dimula73/lager that referenced this issue Nov 21, 2022
Basically, we shouldn't calculate the getter when we know that the
value will be dropped.

The patch partially fixes arximboldi#160, because now the getter is not called
at all during the set operation. But the double-move can still
happen when both, getter and setter calls are performed.
dimula73 added a commit to dimula73/lager that referenced this issue Dec 22, 2022
The patch ensures the following requirements:

1) When `view(lens, whole)` is called:

   * the setter lambda is neither called nor constructed.
     It avoid unnecessary copies of the 'whole' object.

   * the `whole` object is directly forwarded into the getter lambda,
     without any copies.

3) When `set(lens, whole, part)` is called:

   * no getter is called at the toplevel lens (which
     value was previously discarded).

   * on the lower lenses, the getter receives an 'lvalue'
     reference to the `whole` object, and setter receives an
     'rvalue' reference to the `whole` object. It ensures
     that there is no double-move problem.

Fixes arximboldi#160
dimula73 added a commit to dimula73/lager that referenced this issue Dec 22, 2022
The patch ensures the following requirements:

1) When `view(lens, whole)` is called:

   * the setter lambda is neither called nor constructed.
     It avoid unnecessary copies of the 'whole' object.

   * the `whole` object is directly forwarded into the getter lambda,
     without any copies.

3) When `set(lens, whole, part)` is called:

   * no getter is called at the toplevel lens (which
     value was previously discarded).

   * on the lower lenses, the getter receives an 'lvalue'
     reference to the `whole` object, and setter receives an
     'rvalue' reference to the `whole` object. It ensures
     that there is no double-move problem.

Fixes arximboldi#160
kdesysadmin pushed a commit to KDE/krita that referenced this issue Aug 1, 2024
The getter argument should be const-ref for now

See this bug-report:
arximboldi/lager#160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants