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

lager::with() and lager::reader<tuple<...>> behave in a non-symmetrical way #172

Open
dimula73 opened this issue Dec 27, 2022 · 8 comments

Comments

@dimula73
Copy link
Contributor

I have a feeling like there is some bug in mapping readers holding a tuple. It seems to be impossible to map-unzip them into a normal function. Here is a code example:

#include <zug/tuplify.hpp>
#include <zug/transducer/zip.hpp>
#include <zug/transducer/unzip.hpp>

TEST_CASE("tuple unfolding")
{
    auto callback = [] ([[maybe_unused]] int x, [[maybe_unused]] std::string y) {
        return std::to_string(x) + "-" + y;
    };

    auto callback_tuple = [] ([[maybe_unused]] std::tuple<int, std::string> x) {
        return std::to_string(std::get<0>(x)) + "-" + std::get<1>(x);
    };


    state<int, automatic_tag> value1{7};
    state<std::string, automatic_tag> value2{"value"};


    reader<std::string> converted_value{with(value1, value2).map(callback)};
    reader<std::string> converted_value2{with(value1, value2).xform(zug::zip).map(callback_tuple)};

    CHECK(converted_value.get() == "7-value");
    CHECK(converted_value2.get() == "7-value");

    reader<std::tuple<int, std::string>> tuplified{with(value1, value2)};


    // BUG: Fails to compile!
    // reader<std::string> converted_from_tuple{tuplified.xform(zug::unzip).map(callback)};

    reader<std::string> converted_from_tuple2{tuplified.map(callback_tuple)};

    //CHECK(converted_from_tuple.get() == "7-value");
    CHECK(converted_from_tuple2.get() == "7-value");
}

It is possible to map a with-expression to both, tuple-style and unfolded callback, but it is impossible to map a tuple-containing reader to it.

Full unittest is in this commit: ba8fe64

@arximboldi
Copy link
Owner

arximboldi commented Dec 27, 2022

They are not meant to be symmetric. lager::with(...) does not return a reader, but a combined expression of the two things... it zips automatically at the end of the chain when converting to a reader. A reader of tuples does not auto-unzip. We could add that but it could be surprising. We could have that lager::with() zip automatically at the beginning of the "chain" but that would make many common use-cases more inconvenient. I think this probably will be a won't-fix, unless you have a good argument to change it?

@dimula73
Copy link
Contributor Author

Hi, @arximboldi!

The problem is not that lager::with(...) is too permissive, but that lager::reader<tuple<...>> is too restrictive and (seems to be) non-unpackable. I haven't managed to find any way to unpack it, it packs itself back on any my attempt, even when requesting it explicitly like that:

reader<std::string> converted_from_tuple{tuplified.xform(zug::unzip).map(callback)};

In the real code here we use lager::with(...) and lager::reader<tuple<...>> somewhat interchangeably. Basically, when one has a lens that does some "heavy" computations (e.g. this lens), one wants to cache the result somewhere and put it into a reader.

It is also impossible to bind to a lager::with(...) expression (which is understandable), so one needs to put it into a reader first. So, when the same expression is a public "signal" and a source of a subordinate value at the same time, it becomes troublesome to use it.

PS:
Btw, to overcome the unpacking issue on watching/binding I implemented a (hackish-looking?) wrapper:

namespace kismpl {
/**
 * Convert a given functor f accepting multiple arguments into
 * a function that accepts a tuple with the same number of
 * elements
 */
constexpr auto unzip_wrapper = [] (auto f) {
    return
        [=] (auto &&x) {
            return std::apply(f, std::forward<decltype(x)>(x));
        };
};

}

TEST_CASE("tuple unfolding when binding")
{
    auto callback = [] ([[maybe_unused]] int x, [[maybe_unused]] std::string y) {
        std::cout << "unpacked callback" << std::endl;
    };

    auto callback_tuple = [] ([[maybe_unused]] std::tuple<int, std::string> x) {
        std::cout << "packed callback" << std::endl;
    };


    state<int, automatic_tag> value1{7};
    state<std::string, automatic_tag> value2{"value"};

    reader<std::tuple<int, std::string>> converted_value{with(value1, value2)};

    converted_value.bind(kismpl::unzip_wrapper(callback));
    converted_value.bind(callback_tuple);
}

Is it possible to implement the same with the standard transducers? Or, perhaps, we could include this wrapper into lager?

@arximboldi
Copy link
Owner

Hmmm, interesting, something like this should work:

auto callback = [](int x, string y) { return y; };
reader<tuple<int, string>> tuplified = ...;
reader<std::string> converted_from_tuple{tuplified.xform(zug::unzip).map(callback)};

In what way does this fail?

@dimula73
Copy link
Contributor Author

Here is the full error message:

https://invent.kde.org/-/snippets/2458

The line at C:/dev/env-4-lager/lager-tree/lager/test/cursor.cpp:259:46 is:

reader<std::string> converted_from_tuple{tuplified.xform(zug::unzip).map(callback)};

(from the branch linked above)

@arximboldi
Copy link
Owner

Hmm interesting, that looks indeed like a bug!

@arximboldi
Copy link
Owner

Maybe the bug is in zug itself... It would be interesting to try isolate that in an unit test...

@dimula73
Copy link
Contributor Author

dimula73 commented Jan 4, 2023

There is a unittest in this branch: ba8fe64

Or you mean a unittest for zug?

@arximboldi
Copy link
Owner

Hmmm, after some thought, maybe the bug is not in zug... Probably the problem is that .map() works both via transducer composition, but also as-if it had realized the intermediate cursor. No need to try reproduce in zug until we further investigate here.

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

No branches or pull requests

2 participants