teslamotors / fixed-containers

C++ Fixed Containers
MIT License
352 stars 31 forks source link

Problem with "Implicit Structured Binding due to the fields being public is disabled" #94

Closed abeimler closed 4 months ago

abeimler commented 4 months ago

Hello, I was updating my fork with the newest fixed-containers and got an error, cuz "Implicit Structured Binding due to the fields being public is disabled".

I was not sure what constrain was failing ... I'm using glaze by @stephenberry, seems like glaze is not playing well with fixed-containers.

glaze/core/common.hpp

      template <class T>
      concept tuple_t = requires(T t) {
         std::tuple_size<T>::value;      ///< error here
         glz::get<0>(t);
      } && !meta_value_t<T> && !range<T>;

Error:

<source>: In instantiation of 'struct std::tuple_size<fixed_containers::FixedVector<int, 8> >':
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/core/common.hpp:529:30:   required by substitution of 'template<class T>  requires (glaze_array_t<typename std::decay<_Tp>::type>) || (tuple_t<typename std::decay<_Tp>::type>) struct glz::detail::to_json<T> [with T = fixed_containers::FixedVector<int, 8>]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/json/write.hpp:30:60:   required from 'static void glz::detail::write<10>::op(T&&, Ctx&&, B&&, IX&&) [with auto Opts = glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}; T = fixed_containers::FixedVector<int, 8>&; Ctx = glz::context&; B = std::__cxx11::basic_string<char>&; IX = long unsigned int&]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/json/write.hpp:1197:40:   required from 'glz::detail::to_json<Foo>::op_base<glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}, Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&>(Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&)::<lambda(auto:726)> [with auto:726 = std::integral_constant<long unsigned int, 0>]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/util/for_each.hpp:30:68:   required from 'glz::for_each<1, detail::to_json<Foo>::op_base<glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}, Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&>(Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&)::<lambda(auto:726)> >(detail::to_json<Foo>::op_base<glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}, Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&>(Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&)::<lambda(auto:726)>&&)::<lambda(auto:24&& ...)> [with auto:24 = {std::integral_constant<long unsigned int, 0>}]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/util/for_each.hpp:14:17:   required from 'glz::indexer<>(std::index_sequence<0>)::<lambda(auto:23&&)> [with auto:23 = glz::for_each<1, detail::to_json<Foo>::op_base<glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}, Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&>(Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&)::<lambda(auto:726)> >(detail::to_json<Foo>::op_base<glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}, Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&>(Foo&, glz::context&, std::__cxx11::basic_string<char>&, long unsigned int&)::<lambda(auto:726)>&&)::<lambda(auto:24&& ...)>]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/util/for_each.hpp:30:26:   [ skipping 2 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/json/write.hpp:1077:52:   required from 'static void glz::detail::to_json<T>::op(V&&, auto:719&&, auto:720&&, auto:721&&) [with auto Options = glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}; V = Foo&; auto:719 = glz::context&; auto:720 = std::__cxx11::basic_string<char>&; auto:721 = long unsigned int&; T = Foo]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/json/write.hpp:41:66:   required from 'static void glz::detail::write<10>::op(T&&, Ctx&&, B&&, IX&&) [with auto Opts = glz::opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}; T = Foo&; Ctx = glz::context&; B = std::__cxx11::basic_string<char>&; IX = long unsigned int&]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/core/write.hpp:41:52:   required from 'void glz::write(T&&, Buffer&, auto:160&&) [with opts Opts = opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}; T = Foo&; Buffer = std::__cxx11::basic_string<char>; auto:160 = context&]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/core/write.hpp:51:18:   required from 'void glz::write(T&&, Buffer&) [with opts Opts = opts{10, false, true, true, true, false, ' ', 3, false, true, false, false, false, 0, false, false, false, false, false, false, true, false, false, false, false, true}; T = Foo&; Buffer = std::__cxx11::basic_string<char>]'
/opt/compiler-explorer/libs/glaze/trunk/include/glaze/json/write.hpp:1239:20:   required from 'auto glz::write_json(T&&) [with T = Foo&]'
<source>:3038:39:   required from here
<source>:3019:37: error: static assertion failed: Implicit Structured Binding due to the fields being public is disabled
 3019 |     static_assert(fixed_containers::AlwaysFalseV<T, decltype(MAXIMUM_SIZE), CheckingType>,
      |                   ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:3019:37: note: constraints not satisfied
<source>:39:9:   required by the constraints of 'template<class ...> concept fixed_containers::AlwaysFalseV'
<source>:39:24: note: the expression 'false' evaluated to 'false'
   39 | concept AlwaysFalseV = false;
      |                        ^~~~~
Compiler returned: 1

https://godbolt.org/z/ns6ns9Kqo

It happens when I try to write an fixed-container (like EnumMap or FixedVector) via glz::write_json.

#include <glaze/glaze.hpp>

using MyVector = fixed_containers::FixedVector<int, 8>;

struct Foo {
    MyVector bars;
};

int main() {
    Foo foo;
    std::string json = glz::write_json(foo);
    return json.size();
}

Is there a way to "enable" fixed-containers for Serialization ?

Bobobalink commented 4 months ago

Maybe you can write custom glaze wrappers for the fixed_containers types? https://github.com/stephenberry/glaze/blob/main/docs/wrappers.md

It isn't actually correct to reflect into the structures inside a FixedVector for example, because you would "see" the uninitialized slots of the backing array. It's also cleaner to serialize/deserialize into a variable length json array

abeimler commented 4 months ago

Maybe you can write custom glaze wrappers for the fixed_containers types? https://github.com/stephenberry/glaze/blob/main/docs/wrappers.md

I didn't really need to write an wrapper before the "Implicit Structured Binding"-Update. But I tried .. didn't ready work =/

Concepts are used to allow various containers and even user containers if they match standard library interfaces.

Without the compiler tuple-check, everything works as expected.

It's also cleaner to serialize/deserialize into a variable length json array

yea, I agree but for now I only writing some data out. For now all my data is limited.

EDIT:

I think what happen is, glaze is checking via concept for the type and end up been checking for a tuple:

// Specializations
namespace std
{
template <typename T,
          std::size_t MAXIMUM_SIZE,
          fixed_containers::customize::SequenceContainerChecking CheckingType>
struct tuple_size<fixed_containers::FixedVector<T, MAXIMUM_SIZE, CheckingType>>
  : std::integral_constant<std::size_t, 0>
{
    // no value

    //static_assert(fixed_containers::AlwaysFalseV<T, decltype(MAXIMUM_SIZE), CheckingType>,
    //              "Implicit Structured Binding due to the fields being public is disabled");
};
}  // namespace std
      template <class T>
      concept tuple_t = requires(T t) {
         std::tuple_size<T>::value;
         glz::get<0>(t);
      } && !meta_value_t<T> && !range<T>;
Bobobalink commented 4 months ago

Hmm, it is true that glaze shouldn't have to reflect into things that are std::ranges, so it shouldn't need to use a structured binding on it. Maybe we should not have a static_assert here so that concepts and SFINAE work properly.

alexkaratarakis commented 4 months ago

Thanks for reporting! Firstly, here is the reason why this check even exists: fixed_containers have public fields for the sole purpose of making their instances (not types, which work regardless) usable as template parameters (similar to using array instances in templates). The public fields should not be accessed directly and they are named in a way that explicitly expresses this. However, implicit structured binding allows accessing them in a way that doesn't immediately look undesirable, so it should be disabled.

As reported in this thread, the current way of preventing that has the unintended consequence of failing when doing valid metaprogramming, like the one glaze does to check whether this type is a tuple. However, setting the tuple_size to 0 is enough to prevent usage of structured bindings - the static_assert is not necessary. Removing in https://github.com/teslamotors/fixed-containers/pull/95 for all fixed_containers. I verified that glaze will treat FixedVector as a range and print only the valid elements after this fix.

abeimler commented 4 months ago

Maybe it can be some sort of Warning or enabled via Macro/constexpr for "linting" at compile-time.

#ifdef FC_ENABLE_STRICT_MODE
static inline constexpr auto EnableStrictMode = true;
#else
static inline constexpr auto EnableStrictMode = false;
#endif
 static_assert(fixed_containers::EnableStrictMode /*|| fixed_containers::AlwaysFalseV<K, V, CheckingType>*/,
                  "Implicit Structured Binding due to the fields being public is disabled")
alexkaratarakis commented 4 months ago

Yeah, could enhance the check like that if need be. For now, I merged the removal of the static_assert, so you are unblocked as of 0b2c183b830ea8ebf7ea6a1b3fecc7f8a12536db.