Weird behaviour with std::optional as first struct member

[dbraeckelmann]

Glaze v2.4.4 CXX compiler is MSVC 19.39.33523.0 (Visual Studio 2022 v17.9.5) C++ Standard Library 14.39.33519 / Windows SDK 10.0.22621.0 on Windows 10.0.19045.

Having a std::optional as first field in structs seems to trigger some unexpected behaviour:

#include <optional>
#include <string>
#include <print>

#include "glaze/glaze.hpp"

struct target {
    std::optional<std::string> label {"label_optional"};
    std::string name {"name_string"};
};

struct data {
    target target {};
    std::string test {"test"};
};

int main() {
    data d;
    std::println("{}", glz::write<glz::opts{.skip_null_members = false, .prettify = true}>(d));
}

Output:

{

}

After swapping the order of the fields in target and leaving data as is, it works as intended:

struct target {
    std::string name {"name_string"};
    std::optional<std::string> label {"label_optional"};
};

Output:

{
   "target": {
      "name": "name_string",
      "label": "label_optional"
   },
   "test": "test"
}

Lastly, when swapping the fields in target and data both like this, there is a compilation error:

struct target {
    std::optional<std::string> label {"label_optional"};
    std::string name {"name_string"};
};

struct data {
    std::string test {"test"};
    target target {};
};

glaze-src\include\glaze\reflection\to_tuple.hpp(70,25): error C3448: the number of identifiers must match the number of array elements or members in a structured binding declaration

So having a std::optional as first field in structs seems to trigger some unexpected inconsistent behaviour.

[stephenberry]

I've seen the same behavior and don't have a logical reason at the moment. Will keep this issue alive until I understand why. Internally I've sometimes added a bool as the first element to make the struct reflectable without using a constructor.

[dbraeckelmann]

Interesting, do you have a list of known issues and weirdnesses? Would be really helpful to have some reference of where to be careful and what limitations are currently known and have to be worked around.

[stephenberry]

Agreed, pure reflection has some weird corner case limitations. I'll plan to add a known list of weirdness, but there aren't many I'm aware of and I try to fix them when I become aware. Still need to look into this more.

[stephenberry]

I can't seem to replicate your issue with MSVC. I've added a similar test cases to Glaze. Does the code you posted still fail to compile for you?

I've reported a perhaps associated bug to MSVC concerning counting members with std::optional: https://developercommunity.visualstudio.com/t/Bad-aggregate-initializable-handling-us/10664400

I've also created a test branch to look into this issue, but it is passing: #1021

[dbraeckelmann]

Hey Steven, with the latest MSVC 19.10 ("19.40.33808.0") it still shows the exact same behaviour, this only seems to be an issue when the structures are nested. Your testcase just has the one plain struct, that does work fine for me as well.

[stephenberry]

Ah, thanks. The nested case fails with GCC and Clang as well. I'm looking into it.

[stephenberry]

So, the problem is that the count_members function needs to guard against wrong counting for C style arrays by using a brace initialization wrapper (e.g. {Args{}}...). This breaks nested structure counting. If I remove the wrapping braces Args{}..., then nested cases work, such as the one in this issue. However, removing the {} breaks reflection for structs with C style arrays.

Normally I would just recommend not using C style arrays and use std::array. But, I know from where I work that C style arrays are common in hardware interfaces, and it's really nice to be able to reflect on these structs.

I'm working on trying to see if there is a workaround to handle all cases.

[stephenberry]

I found a simple solution that fixes this problem across MSVC, GCC, and Clang. The solution is to simply const qualify the member functions for the any_t. I've merged in the change in #1021

Let me know if you still run into issues, but I added a nested test that passes and I did a bunch of experimentation that passed.

[dbraeckelmann]

Yes, all permutations of my testcase now work as intended, thanks for fixing this!

[stephenberry]

Sweet! You're welcome, this makes me quite happy.

[dbraeckelmann]

I just attempted to update our codebase to the latest glaze and noticed that this change (added const qualifiers to the functions in any_t in to_tuple.hpp) breaks some custom serializers (does not compile any more with some meta errors, I can try to boil it down more, but seems to be just these custom template specializations):

template<typename T>
concept is_duration = requires {
    []<class Rep, class Period>(std::type_identity<std::chrono::duration<Rep, Period>>){}(std::type_identity<T>());
};

namespace glz::detail
{

template<is_duration T> struct from_json<T>
{
    template<auto Opts> static void op(T& duration, auto&&... args)
    {
        uint64_t i;
        read<json>::op<Opts>(i, args...);
        duration = T{i};
    }
};

template<is_duration T> struct to_json<T>
{
    template<auto Opts> static void op(const T& duration, auto&&... args) noexcept
    {
        write<json>::op<Opts>(std::to_string(duration.count()), args...);
    }
};

If I revert just 438d3e8408851c5216c6c81e59346065dc97087a and use the current release (v2.8.0), everything compiles again.

[stephenberry]

Thanks for reporting this. I've actually been wanting to add std::chrono support to Glaze. I'll experiment soon with potential fixes. I have some ideas, and I'm also working on getting MSVC and GCC to fix some compiler bugs.

[stephenberry]

So, the problem is that reverting https://github.com/stephenberry/glaze/commit/438d3e8408851c5216c6c81e59346065dc97087a breaks this original issue with std::optional broken with reflection as the first member. The addition of const fixed the std::optional problem as well as some others. Removing the initializer list construction from count_members fixes both cases, but breaks counting C style arrays.

Currently MSVC is not compliant with the C++ standard, and I have an open issue here: https://developercommunity.visualstudio.com/t/Bad-aggregate-initializable-handling-us/10664400

However, they are reticent to fix the bug as the wording around conversion operators may change soon. However, from reading the paper the wording should change in favor of supporting what Glaze wants. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2023/p2828r1.html

From the paper:

If the selected constructor is a copy or move constructor ([class.copy.ctor]) for T, the braced-init-list contains a single element, and the implicit conversion sequence ([over.best.ics.general]) for the constructor's first parameter would bind that parameter to the result object of a conversion function whose return type is T (ignoring cv-qualification), that conversion function is called to initialize the object.

This is what Glaze has, a single element in an initializer list whose return type is T, so MSVC and GCC ought to construct T and not fail as they currently do.

I'll continue to work with MSVC and GCC, but it's hard to find solutions when they aren't standard compliant.

There is actually another solution to counting members with requires and structured bindings, but MSVC, GCC, and Clang all have compiler errors here that I plan to report, and I don't expect this solution to be viable in the near term.

[dbraeckelmann]

Haha working on the cutting (or in this case more bleeding) edge of compilers truly is an experience in its own 😃

For now I can live with manually reverting 438d3e8408851c5216c6c81e59346065dc97087a until some better solution is available or MSVC fixes their bug.

[stephenberry]

Cool, I'm going to keep #1066 alive as I work on the issue. It is a tricky one.