Using aggregate types with `StateValue` broke

[chrhaase]

I have code like this in my project and on the master branch this works well.
Just checked out next and now it won't compile.

Do you have an idea why and how it can be fixed?

struct AggregateType
{
    juce::String a;
    juce::String b;

    friend bool operator== (const AggregateType& lhs, const AggregateType& rhs) { return lhs.a == rhs.a
                                                                                         && lhs.b == rhs.b; }
    friend bool operator!= (const AggregateType& lhs, const AggregateType& rhs) { return ! (lhs == rhs); }
};

chowdsp::StateValue<AggregateType> aggregate;

Here's the error message from AppleClang:

In file included from /Users/chrhaase/dev/ChowDSP/chowdsp_utils/tests/plugin_tests/chowdsp_plugin_state_test/NonParamTest.cpp:2:
In file included from /Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/plugin/chowdsp_plugin_state/chowdsp_plugin_state.h:36:
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/plugin/chowdsp_plugin_state/Backend/chowdsp_StateValue.h:76:16: error: no viable conversion from returned value of type 'element_type' (aka 'AggregateType') to function return type 'nlohmann::json' (aka 'basic_json<>')
   76 |         return get();
      |                ^~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/tests/plugin_tests/chowdsp_plugin_state_test/NonParamTest.cpp:25:44: note: in instantiation of member function 'chowdsp::StateValue<AggregateType>::serialize_json' requested here
   25 |         chowdsp::StateValue<AggregateType> aggregate_val { "aggregate", { "foo", "bar" } };
      |                                            ^
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:19879:5: note: candidate constructor not viable: no known conversion from 'element_type' (aka 'AggregateType') to 'const value_t' (aka 'const nlohmann::detail::value_t') for 1st argument
 19879 |     basic_json(const value_t v)
       |     ^          ~~~~~~~~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:19887:5: note: candidate constructor not viable: no known conversion from 'element_type' (aka 'AggregateType') to 'std::nullptr_t' for 1st argument
 19887 |     basic_json(std::nullptr_t = nullptr) noexcept // NOLINT(bugprone-exception-escape)
       |     ^          ~~~~~~~~~~~~~~~~~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:19966:5: note: candidate constructor not viable: no known conversion from 'element_type' (aka 'AggregateType') to 'initializer_list_t' (aka 'initializer_list<detail::json_ref<basic_json<std::map, std::vector, string, bool, long long, unsigned long long, double, std::allocator, nlohmann::adl_serializer, vector<unsigned char, allocator<unsigned char>>>>>') for 1st argument
 19966 |     basic_json(initializer_list_t init,
       |     ^          ~~~~~~~~~~~~~~~~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:20208:5: note: candidate constructor not viable: no known conversion from 'element_type' (aka 'AggregateType') to 'const basic_json<> &' for 1st argument
 20208 |     basic_json(const basic_json& other)
       |     ^          ~~~~~~~~~~~~~~~~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:20276:5: note: candidate constructor not viable: no known conversion from 'element_type' (aka 'AggregateType') to 'basic_json<> &&' for 1st argument
 20276 |     basic_json(basic_json&& other) noexcept
       |     ^          ~~~~~~~~~~~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:19899:5: note: candidate template ignored: requirement 'detail::is_compatible_type<nlohmann::basic_json<std::map, std::vector, std::string, bool, long long, unsigned long long, double, std::allocator, nlohmann::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char>>>, AggregateType>::value' was not satisfied [with CompatibleType = element_type, U = detail::uncvref_t<element_type>]
 19899 |     basic_json(CompatibleType && val) noexcept(noexcept( // NOLINT(bugprone-forwarding-reference-overload,bugprone-exception-escape)
       |     ^
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:19913:5: note: candidate template ignored: requirement 'detail::is_basic_json<AggregateType>::value' was not satisfied [with BasicJsonType = element_type]
 19913 |     basic_json(const BasicJsonType& val)
       |     ^
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:20204:5: note: candidate template ignored: substitution failure [with JsonRef = element_type]: no type named 'value_type' in 'AggregateType'
 20203 |                                  std::is_same<typename JsonRef::value_type, basic_json>>::value, int> = 0 >
       |                                                                 ~~~~~~~~~~
 20204 |     basic_json(const JsonRef& ref) : basic_json(ref.moved_or_copied()) {}
       |     ^
In file included from /Users/chrhaase/dev/ChowDSP/chowdsp_utils/tests/plugin_tests/chowdsp_plugin_state_test/NonParamTest.cpp:2:
In file included from /Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/plugin/chowdsp_plugin_state/chowdsp_plugin_state.h:36:
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/plugin/chowdsp_plugin_state/Backend/chowdsp_StateValue.h:82:23: error: no matching member function for call to 'get'
   82 |         set (deserial.get<element_type>());
      |              ~~~~~~~~~^~~~~~~~~~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/tests/plugin_tests/chowdsp_plugin_state_test/NonParamTest.cpp:25:44: note: in instantiation of member function 'chowdsp::StateValue<AggregateType>::deserialize_json' requested here
   25 |         chowdsp::StateValue<AggregateType> aggregate_val { "aggregate", { "foo", "bar" } };
      |                                            ^
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:20805:10: note: candidate template ignored: substitution failure [with ValueTypeCV = element_type, ValueType = detail::uncvref_t<element_type>]: no matching member function for call to 'get_impl'
 20805 |     auto get() const noexcept(
       |          ^
 20806 |     noexcept(std::declval<const basic_json_t&>().template get_impl<ValueType>(detail::priority_tag<4> {})))
 20807 |     -> decltype(std::declval<const basic_json_t&>().template get_impl<ValueType>(detail::priority_tag<4> {}))
       |                                                              ~~~~~~~~
/Users/chrhaase/dev/ChowDSP/chowdsp_utils/modules/common/chowdsp_json/third_party/nlohmann/json.hpp:20846:10: note: candidate template ignored: substitution failure [with PointerType = element_type]: no matching member function for call to 'get_ptr'
 20846 |     auto get() noexcept -> decltype(std::declval<basic_json_t&>().template get_ptr<PointerType>())
       |          ^                                                                 ~~~~~~~

[jatinchowdhury18]

Hi Christian,

Thanks for testing this and catching it before I merged it to master. This change is kind of an offshoot of some other changes that I made in an effort to make the serialization system more robust. The issue with serializing aggregate types as it was previously implemented was that it would be pretty easy for the user accidentally break compatibility by changing the order of struct members. So for example, if version 1 of your amp/plugin had:

struct AggregateType
{
    juce::String a;
    juce::String b;
};

And then in version 2 you changed it to:

struct AggregateType
{
    juce::String a;
    int x;
    juce::String b;
};

Then deserializing a state saved with version 1 of the app/plugin would break, potentially in a silent way.

So the "new" way to do this sort of thing is to manually define your serialization scheme via a custom template specialization of chowdsp::StateValue<T>, for example:

template <>
struct chowdsp::StateValue<AggregateType> : StateValueBase
{
    StateValue (std::string_view valueName, const AggregateType& default_value)
        : StateValueBase { valueName },
          default_val { default_value },
          val { default_value }
    {
        reset();
    }

    auto get() const noexcept
    {
        return val;
    }

    void set (const AggregateType& v)
    {
        if (v == val)
            return;

        val = v;
        changeBroadcaster();
    }

    void reset() override { set (default_val); }

    [[nodiscard]] nlohmann::json serialize_json() const override
    {
        const auto current_value = get();
        return {
            { "a_value", current_value.a },
            { "b_value", current_value.b },
        };
    }

    void deserialize_json (const nlohmann::json& deserial) override
    {
        set (AggregateType {
            .a = deserial.value ("a_value", juce::String { "default string" }),
            .b = deserial.value ("b_value", juce::String { "default string" }),
        });
    }

    AggregateType default_val {};
    AggregateType val {};
};

That said, given that this is a breaking change, do you have some suggestions for a good way to document it and/or a better error message that you'd like to see for this kind of thing? There's also like 6 API points that the implemented needs to provide, which seems like a lot (constructor, get(), set(), reset(), serialize(), and deserialize()), so maybe there's a way to simplify that with a mixin or a base class or something?

As an example, I've got a PR that adds a test case for this type of thing to the test suite.

[chrhaase]

The issue with serializing aggregate types as it was previously implemented was that it would be pretty easy for the user accidentally break compatibility by changing the order of struct members.

Thanks for clearing that up. Makes sense!

Wouldn't it be simpler to just tell nlohmann how to parse the custom type? So something like this:

namespace nlohmann
{

template <>
struct adl_serializer<AggregateType>
{
    static void to_json (json& j, const AggregateType& a)
    {
        j = { { "a_value", a.a }, { "b_value", a.b } };
    }

    static void from_json (const json& j, AggregateType& a)
    {
        a.a = j.value ("a_value", juce::String ("default string"));
        a.b = j.value ("b_value", juce::String ("default string"));
    }
};

} // namespace nlohmann

[jatinchowdhury18]

Oh good call! That's a much simpler approach. There's probably a couple of cases where the full template specialization is needed, but those are probably more "edge cases".

I've updated the PR to use the ADL serializer.

[chrhaase]

Great! And probably, yes.
Regarding documentation, I'm not sure how the error message could be improved.
I like the BREAKING-CHANGES.md file they have in JUCE.
But a notice in the change log could also do the trick.

[jatinchowdhury18]

I put an "all caps" notice in the changelog for now... hopefully that will catch the attention of everyone who needs to know :).