Skip to content

Commit

Permalink
fix std::filesystem::path regression (#3073)
Browse files Browse the repository at this point in the history 
* meta: rework is_compatible/is_constructible_string_type

These type traits performed an incorrect and insufficient check.

Converting to a std::filesystem::path used to work by accident thanks to
these brittle constraints, but the clean-up performed in #3020 broke them.

* support std::filesystem::path

Fixes #3070
  • Loading branch information
@theodelrieu
theodelrieu authored Oct 14, 2021
1 parent 4b1cb9e commit 0e694b4
Show file tree
Hide file tree
Showing 5 changed files with 161 additions and 119 deletions.
18 changes: 17 additions & 1 deletion include/nlohmann/detail/conversions/from_json.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
#include <nlohmann/detail/meta/type_traits.hpp>
#include <nlohmann/detail/value_t.hpp>

#ifdef JSON_HAS_CPP_17
#include <filesystem>
#endif

namespace nlohmann
{
namespace detail
Expand Down Expand Up @@ -169,7 +173,7 @@ void from_json(const BasicJsonType& j, std::valarray<T>& l)
}

template<typename BasicJsonType, typename T, std::size_t N>
auto from_json(const BasicJsonType& j, T (&arr)[N]) // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays)
auto from_json(const BasicJsonType& j, T (&arr)[N]) // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays)
-> decltype(j.template get<T>(), void())
{
for (std::size_t i = 0; i < N; ++i)
Expand Down Expand Up @@ -444,6 +448,18 @@ void from_json(const BasicJsonType& j, std::unordered_map<Key, Value, Hash, KeyE
}
}

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType>
void from_json(const BasicJsonType& j, std::filesystem::path& p)
{
if (JSON_HEDLEY_UNLIKELY(!j.is_string()))
{
JSON_THROW(type_error::create(302, "type must be string, but is " + std::string(j.type_name()), j));
}
p = *j.template get_ptr<const typename BasicJsonType::string_t*>();
}
#endif

struct from_json_fn
{
template<typename BasicJsonType, typename T>
Expand Down
13 changes: 13 additions & 0 deletions include/nlohmann/detail/conversions/to_json.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@
#include <valarray> // valarray
#include <vector> // vector

#include <nlohmann/detail/macro_scope.hpp>
#include <nlohmann/detail/iterators/iteration_proxy.hpp>
#include <nlohmann/detail/meta/cpp_future.hpp>
#include <nlohmann/detail/meta/type_traits.hpp>
#include <nlohmann/detail/value_t.hpp>

#ifdef JSON_HAS_CPP_17
#include <filesystem>
#endif

namespace nlohmann
{
namespace detail
Expand Down Expand Up @@ -386,6 +391,14 @@ void to_json(BasicJsonType& j, const T& t)
to_json_tuple_impl(j, t, make_index_sequence<std::tuple_size<T>::value> {});
}

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType>
void to_json(BasicJsonType& j, const std::filesystem::path& p)
{
j = p.string();
}
#endif

struct to_json_fn
{
template<typename BasicJsonType, typename T>
Expand Down
39 changes: 11 additions & 28 deletions include/nlohmann/detail/meta/type_traits.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,44 +309,21 @@ struct is_constructible_object_type
: is_constructible_object_type_impl<BasicJsonType,
ConstructibleObjectType> {};

template<typename BasicJsonType, typename CompatibleStringType,
typename = void>
struct is_compatible_string_type_impl : std::false_type {};

template<typename BasicJsonType, typename CompatibleStringType>
struct is_compatible_string_type_impl <
BasicJsonType, CompatibleStringType,
enable_if_t<is_detected_convertible<typename BasicJsonType::string_t::value_type,
range_value_t,
CompatibleStringType>::value >>
struct is_compatible_string_type
{
static constexpr auto value =
is_constructible<typename BasicJsonType::string_t, CompatibleStringType>::value;
};

template<typename BasicJsonType, typename ConstructibleStringType>
struct is_compatible_string_type
: is_compatible_string_type_impl<BasicJsonType, ConstructibleStringType> {};

template<typename BasicJsonType, typename ConstructibleStringType,
typename = void>
struct is_constructible_string_type_impl : std::false_type {};

template<typename BasicJsonType, typename ConstructibleStringType>
struct is_constructible_string_type_impl <
BasicJsonType, ConstructibleStringType,
enable_if_t<is_detected_exact<typename BasicJsonType::string_t::value_type,
value_type_t, ConstructibleStringType>::value >>
struct is_constructible_string_type
{
static constexpr auto value =
is_constructible<ConstructibleStringType,
typename BasicJsonType::string_t>::value;
};

template<typename BasicJsonType, typename ConstructibleStringType>
struct is_constructible_string_type
: is_constructible_string_type_impl<BasicJsonType, ConstructibleStringType> {};

template<typename BasicJsonType, typename CompatibleArrayType, typename = void>
struct is_compatible_array_type_impl : std::false_type {};

Expand All @@ -355,7 +332,10 @@ struct is_compatible_array_type_impl <
BasicJsonType, CompatibleArrayType,
enable_if_t <
is_detected<iterator_t, CompatibleArrayType>::value&&
is_iterator_traits<iterator_traits<detected_t<iterator_t, CompatibleArrayType>>>::value >>
is_iterator_traits<iterator_traits<detected_t<iterator_t, CompatibleArrayType>>>::value&&
// special case for types like std::filesystem::path whose iterator's value_type are themselves
// c.f. https://github.com/nlohmann/json/pull/3073
!std::is_same<CompatibleArrayType, detected_t<range_value_t, CompatibleArrayType>>::value >>
{
static constexpr bool value =
is_constructible<BasicJsonType,
Expand Down Expand Up @@ -388,8 +368,11 @@ struct is_constructible_array_type_impl <
is_detected<iterator_t, ConstructibleArrayType>::value&&
is_iterator_traits<iterator_traits<detected_t<iterator_t, ConstructibleArrayType>>>::value&&
is_detected<range_value_t, ConstructibleArrayType>::value&&
is_complete_type <
detected_t<range_value_t, ConstructibleArrayType >>::value >>
// special case for types like std::filesystem::path whose iterator's value_type are themselves
// c.f. https://github.com/nlohmann/json/pull/3073
!std::is_same<ConstructibleArrayType, detected_t<range_value_t, ConstructibleArrayType>>::value&&
is_complete_type <
detected_t<range_value_t, ConstructibleArrayType >>::value >>
{
using value_type = range_value_t<ConstructibleArrayType>;

Expand Down
71 changes: 42 additions & 29 deletions single_include/nlohmann/json.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3783,44 +3783,21 @@ struct is_constructible_object_type
: is_constructible_object_type_impl<BasicJsonType,
ConstructibleObjectType> {};

template<typename BasicJsonType, typename CompatibleStringType,
typename = void>
struct is_compatible_string_type_impl : std::false_type {};

template<typename BasicJsonType, typename CompatibleStringType>
struct is_compatible_string_type_impl <
BasicJsonType, CompatibleStringType,
enable_if_t<is_detected_convertible<typename BasicJsonType::string_t::value_type,
range_value_t,
CompatibleStringType>::value >>
struct is_compatible_string_type
{
static constexpr auto value =
is_constructible<typename BasicJsonType::string_t, CompatibleStringType>::value;
};

template<typename BasicJsonType, typename ConstructibleStringType>
struct is_compatible_string_type
: is_compatible_string_type_impl<BasicJsonType, ConstructibleStringType> {};

template<typename BasicJsonType, typename ConstructibleStringType,
typename = void>
struct is_constructible_string_type_impl : std::false_type {};

template<typename BasicJsonType, typename ConstructibleStringType>
struct is_constructible_string_type_impl <
BasicJsonType, ConstructibleStringType,
enable_if_t<is_detected_exact<typename BasicJsonType::string_t::value_type,
value_type_t, ConstructibleStringType>::value >>
struct is_constructible_string_type
{
static constexpr auto value =
is_constructible<ConstructibleStringType,
typename BasicJsonType::string_t>::value;
};

template<typename BasicJsonType, typename ConstructibleStringType>
struct is_constructible_string_type
: is_constructible_string_type_impl<BasicJsonType, ConstructibleStringType> {};

template<typename BasicJsonType, typename CompatibleArrayType, typename = void>
struct is_compatible_array_type_impl : std::false_type {};

Expand All @@ -3829,7 +3806,10 @@ struct is_compatible_array_type_impl <
BasicJsonType, CompatibleArrayType,
enable_if_t <
is_detected<iterator_t, CompatibleArrayType>::value&&
is_iterator_traits<iterator_traits<detected_t<iterator_t, CompatibleArrayType>>>::value >>
is_iterator_traits<iterator_traits<detected_t<iterator_t, CompatibleArrayType>>>::value&&
// special case for types like std::filesystem::path whose iterator's value_type are themselves
// c.f. https://github.com/nlohmann/json/pull/3073
!std::is_same<CompatibleArrayType, detected_t<range_value_t, CompatibleArrayType>>::value >>
{
static constexpr bool value =
is_constructible<BasicJsonType,
Expand Down Expand Up @@ -3862,8 +3842,11 @@ struct is_constructible_array_type_impl <
is_detected<iterator_t, ConstructibleArrayType>::value&&
is_iterator_traits<iterator_traits<detected_t<iterator_t, ConstructibleArrayType>>>::value&&
is_detected<range_value_t, ConstructibleArrayType>::value&&
is_complete_type <
detected_t<range_value_t, ConstructibleArrayType >>::value >>
// special case for types like std::filesystem::path whose iterator's value_type are themselves
// c.f. https://github.com/nlohmann/json/pull/3073
!std::is_same<ConstructibleArrayType, detected_t<range_value_t, ConstructibleArrayType>>::value&&
is_complete_type <
detected_t<range_value_t, ConstructibleArrayType >>::value >>
{
using value_type = range_value_t<ConstructibleArrayType>;

Expand Down Expand Up @@ -3967,6 +3950,10 @@ T conditional_static_cast(U value)
// #include <nlohmann/detail/value_t.hpp>


#ifdef JSON_HAS_CPP_17
#include <filesystem>
#endif

namespace nlohmann
{
namespace detail
Expand Down Expand Up @@ -4117,7 +4104,7 @@ void from_json(const BasicJsonType& j, std::valarray<T>& l)
}

template<typename BasicJsonType, typename T, std::size_t N>
auto from_json(const BasicJsonType& j, T (&arr)[N]) // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays)
auto from_json(const BasicJsonType& j, T (&arr)[N]) // NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays)
-> decltype(j.template get<T>(), void())
{
for (std::size_t i = 0; i < N; ++i)
Expand Down Expand Up @@ -4392,6 +4379,18 @@ void from_json(const BasicJsonType& j, std::unordered_map<Key, Value, Hash, KeyE
}
}

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType>
void from_json(const BasicJsonType& j, std::filesystem::path& p)
{
if (JSON_HEDLEY_UNLIKELY(!j.is_string()))
{
JSON_THROW(type_error::create(302, "type must be string, but is " + std::string(j.type_name()), j));
}
p = *j.template get_ptr<const typename BasicJsonType::string_t*>();
}
#endif

struct from_json_fn
{
template<typename BasicJsonType, typename T>
Expand Down Expand Up @@ -4425,6 +4424,8 @@ constexpr const auto& from_json = detail::static_const<detail::from_json_fn>::va
#include <valarray> // valarray
#include <vector> // vector

// #include <nlohmann/detail/macro_scope.hpp>

// #include <nlohmann/detail/iterators/iteration_proxy.hpp>


Expand Down Expand Up @@ -4625,6 +4626,10 @@ class tuple_element<N, ::nlohmann::detail::iteration_proxy_value<IteratorType >>
// #include <nlohmann/detail/value_t.hpp>


#ifdef JSON_HAS_CPP_17
#include <filesystem>
#endif

namespace nlohmann
{
namespace detail
Expand Down Expand Up @@ -4997,6 +5002,14 @@ void to_json(BasicJsonType& j, const T& t)
to_json_tuple_impl(j, t, make_index_sequence<std::tuple_size<T>::value> {});
}

#ifdef JSON_HAS_CPP_17
template<typename BasicJsonType>
void to_json(BasicJsonType& j, const std::filesystem::path& p)
{
j = p.string();
}
#endif

struct to_json_fn
{
template<typename BasicJsonType, typename T>
Expand Down
Loading

2 comments on commit 0e694b4

@Jehops
Copy link

@Jehops Jehops commented on 0e694b4 Oct 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix. I hope we can get this into the FreeBSD package, as a few consumers packages like bear are currently not building. Normally we could update our package independent of a new release here using

% git describe --tags 0e694b4060ed55df980eaaebc2398b0ff24530d4
v3.10.1-16-g0e694b406

But v3.10.1-16-g0e694b406 goes backwards relative to the current version in the FreeBSD ports tree, which is 3.10.3 and having versions go backwards causes problems.

Would it be possible to cut a new release?

@nlohmann
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have another fix in review right now and would then create the release 3.10.4. You can speed up this process by also having a look at https://github.com/nlohmann/json/pulls?q=is%3Apr+is%3Aopen+label%3A%22review+needed%22.

Please sign in to comment.