fix std::filesystem::path regression

[theodelrieu]

Antiquated type traits performed an incorrect and insufficient check.

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

Fixes #3070

• Changes are described in the pull request, or an existing issue is referenced.
• The test suite compiles and runs without error.
• Code coverage is 100%. Test cases can be added by editing the test suite.
• The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

[coveralls]

Coverage remained the same at 100.0% when pulling 68825f1 on theodelrieu:fix/filesystem_path into 4b1cb9e on nlohmann:develop.

[theodelrieu]

Digging into why Windows fails, I found something interesting.

This code causes a SIGSEGV with 3.10.2:

std::filesystem::path text_path("/tmp/text.txt");
json j(text_path);

@TheAifam5 Have you ever constructed a JSON value with std::filesystem::path? Or only converting a JSON value to it?

Before 3.10.3, this worked by accident, as std::filesystem::path constructor allows any input:

Any of the character types char, char8_t, (since C++20)char16_t, char32_t, wchar_t is allowed, and the method of conversion to the native character set depends on the character type used by source

However, converting from a path to json never worked on Windows, as the conversion operator returns a std::wstring!

So this is not a compatible string type for the library, so it tries to convert it to an array, and it crashes since std::filesystem::path is a peculiar beast (see other issues linked to it: google/googletest#521).

We could introduce C++17-and-higher to_json/from_json for std::filesystem::path, but I don't know if the library should handle UTF-8 conversions.

Any idea @nlohmann?

[TheAifam5]

@theodelrieu let me see the Bear project, will report to you shortly.

EDIT 1:
Using your branch helps to remove some error messages, now only one is left (see log 1 below).
The line which produces that error: j["paths_to_include"] = rhs.paths_to_include; which adds a list of std::list<std::filesystem::path> to the nlohmann::json - answering your question, Bear constructs a JSON value from the list of paths. j is the json object, paths_to_include - std::list<std::filesystem::path>.

LOG 1:

[ 75%] Building CXX object citnames/CMakeFiles/citnames_json_a.dir/source/Configuration.cc.o
In file included from /home/theaifam5/Documents/GitHub/Bear/source/citnames/source/Configuration.cc:26:
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp: In instantiation of ‘constexpr const bool nlohmann::detail::is_compatible_array_type_impl<nlohmann::basic_json<>, std::filesystem::__cxx11::path, void>::value’:
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:4898:48:   required by substitution of ‘template<class BasicJsonType, class CompatibleArrayType, typename std::enable_if<((((nlohmann::detail::is_compatible_array_type<BasicJsonType, CompatibleArrayType>::value && (! nlohmann::detail::is_compatible_object_type<BasicJsonType, CompatibleObjectType>::value)) && (! nlohmann::detail::is_compatible_string_type<BasicJsonType, CompatibleStringType>::value)) && (! std::is_same<typename BasicJsonType::binary_t, CompatibleArrayType>::value)) && (! nlohmann::detail::is_basic_json<T>::value)), int>::type <anonymous> > void nlohmann::detail::to_json(BasicJsonType&, const CompatibleArrayType&) [with BasicJsonType = nlohmann::basic_json<>; CompatibleArrayType = std::filesystem::__cxx11::path; typename std::enable_if<((((nlohmann::detail::is_compatible_array_type<BasicJsonType, CompatibleArrayType>::value && (! nlohmann::detail::is_compatible_object_type<BasicJsonType, CompatibleObjectType>::value)) && (! nlohmann::detail::is_compatible_string_type<BasicJsonType, CompatibleStringType>::value)) && (! std::is_same<typename BasicJsonType::binary_t, CompatibleArrayType>::value)) && (! nlohmann::detail::is_basic_json<T>::value)), int>::type <anonymous> = <missing>]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:4981:24:   required by substitution of ‘template<class BasicJsonType, class T> decltype ((nlohmann::detail::to_json(j, forward<T>(val)), void())) nlohmann::detail::to_json_fn::operator()(BasicJsonType&, T&&) const [with BasicJsonType = nlohmann::basic_json<>; T = std::filesystem::__cxx11::path]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:5059:36:   required by substitution of ‘template<class BasicJsonType, class TargetType> static decltype ((nlohmann::{anonymous}::to_json(j, forward<TargetType>(val)), void())) nlohmann::adl_serializer<std::filesystem::__cxx11::path, void>::to_json<BasicJsonType, TargetType>(BasicJsonType&, TargetType&&) [with BasicJsonType = nlohmann::basic_json<>; TargetType = std::filesystem::__cxx11::path]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:3560:45:   required by substitution of ‘template<class T, class ... Args> using to_json_function = decltype (T::to_json((declval<Args>)()...)) [with T = nlohmann::adl_serializer<std::filesystem::__cxx11::path, void>; Args = {nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long int, long unsigned int, double, std::allocator, nlohmann::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >&, std::filesystem::__cxx11::path}]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:2286:7:   [ skipping 10 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:5059:36:   required by substitution of ‘template<class BasicJsonType, class TargetType> static decltype ((nlohmann::{anonymous}::to_json(j, forward<TargetType>(val)), void())) nlohmann::adl_serializer<std::__cxx11::list<std::filesystem::__cxx11::path>, void>::to_json<BasicJsonType, TargetType>(BasicJsonType&, TargetType&&) [with BasicJsonType = nlohmann::basic_json<>; TargetType = std::__cxx11::list<std::filesystem::__cxx11::path>]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:2286:7:   recursively required by substitution of ‘template<class Default, template<class ...> class Op, class ... Args> struct nlohmann::detail::detector<Default, typename nlohmann::detail::make_void<Op<Args ...> >::type, Op, Args ...> [with Default = nlohmann::detail::nonesuch; Op = nlohmann::detail::to_json_function; Args = {nlohmann::adl_serializer<std::__cxx11::list<std::filesystem::__cxx11::path, std::allocator<std::filesystem::__cxx11::path> >, void>, nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long int, long unsigned int, double, std::allocator, nlohmann::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >&, std::__cxx11::list<std::filesystem::__cxx11::path, std::allocator<std::filesystem::__cxx11::path> >}]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:2286:7:   required by substitution of ‘template<class Expected, template<class ...> class Op, class ... Args> using is_detected_exact = std::is_same<Expected, typename nlohmann::detail::detector<nlohmann::detail::nonesuch, void, Op, Args ...>::type> [with Expected = void; Op = nlohmann::detail::to_json_function; Args = {nlohmann::adl_serializer<std::__cxx11::list<std::filesystem::__cxx11::path, std::allocator<std::filesystem::__cxx11::path> >, void>, nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, long int, long unsigned int, double, std::allocator, nlohmann::adl_serializer, std::vector<unsigned char, std::allocator<unsigned char> > >&, std::__cxx11::list<std::filesystem::__cxx11::path, std::allocator<std::filesystem::__cxx11::path> >}]’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:3619:13:   required from ‘constexpr const bool nlohmann::detail::has_to_json<nlohmann::basic_json<>, std::__cxx11::list<std::filesystem::__cxx11::path>, void>::value’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:3897:53:   required from ‘constexpr const bool nlohmann::detail::is_compatible_type_impl<nlohmann::basic_json<>, std::__cxx11::list<std::filesystem::__cxx11::path>, void>::value’
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:19033:53:   required by substitution of ‘template<class CompatibleType, class U, typename std::enable_if<((! nlohmann::detail::is_basic_json<T>::value) && nlohmann::detail::is_compatible_type<nlohmann::basic_json<>, U>::value), int>::type <anonymous> > nlohmann::basic_json<>::basic_json(CompatibleType&&) [with CompatibleType = const std::__cxx11::list<std::filesystem::__cxx11::path>&; U = std::__cxx11::list<std::filesystem::__cxx11::path>; typename std::enable_if<((! nlohmann::detail::is_basic_json<T>::value) && nlohmann::detail::is_compatible_type<nlohmann::basic_json<>, U>::value), int>::type <anonymous> = <missing>]’
/home/theaifam5/Documents/GitHub/Bear/source/citnames/source/Configuration.cc:58:41:   required from here
/home/theaifam5/Documents/GitHub/Bear/build/subprojects/Install/nlohmann_json_dependency/include/nlohmann/json.hpp:3813:46: error: incomplete type ‘nlohmann::detail::is_constructible<nlohmann::basic_json<>, std::filesystem::__cxx11::path>’ used in nested name specifier
 3813 |         range_value_t<CompatibleArrayType>>::value;
      |                                              ^~~~~

[theodelrieu]

@TheAifam5 Thanks, does that code work on Windows with 3.10.2? It should cause a stack overflow.

[theodelrieu]

The main problem is that std::filesystem::path::value_type is std::filesystem::path itself. Which is the cause of several stack overflows in different projects (e.g. GoogleTest that I linked earlier).

On Windows, the operator string_type returns a std::wstring, which is not convertible to json::string_t, thus it tries to see if it can be converted to an array type. Which implies that the value_type must be convertible to json. Which it is, as it's std::filesystem::path again!

Then, when performing the runtime conversion, it will just call to_json recursively and crash.

[TheAifam5]

I‘m on Linux. I have no access to Windows

[theodelrieu]

Well, it seems the problem was never spotted because there is no Windows support for bear: rizsotto/Bear#303 (comment)

I'm confident that path has never worked with json on Windows. Now what can we do about it?

The library only supports UTF-8 (see https://github.com/nlohmann/json#character-encoding) so I think we can add C++17 overloads of to/from_json for path, using string() and call it a day.

EDIT: I don't know what happens when basic_json<> is specialized to have std::wstring as a string type... @nlohmann?

[theodelrieu]

Should we constrain the overloads on whether or not path is convertible/constructible to/from std::string?

[nlohmann]

Should we constrain the overloads on whether or not path is convertible/constructible to/from std::string?

You mean: whether the overload is only used in cases where a std::wstring is returned?

[theodelrieu]

You mean: whether the overload is only used in cases where a std::wstring is returned?

path::string() always returns a std::string, so my question is more like: Do we want to support other StringType?

If a user has basic_json<..., ..., std::wstring, ...> for example.

[nlohmann]

EDIT: I don't know what happens when basic_json<> is specialized to have std::wstring as a string type... @nlohmann?

Never tried it, and I don't expect it to work...

[nlohmann]

You mean: whether the overload is only used in cases where a std::wstring is returned?

path::string() always returns a std::string, so my question is more like: Do we want to support other StringType?

If a user has basic_json<..., ..., std::wstring, ...> for example.

Using different string types is currently hardly tested, because the library uses a lot of the std::basic_string API. I would be fine by supporting std::filesystem::path like this as I doubt this will be the only thing that breaks with a different string type.

[nlohmann]

Looks good to me.

[nlohmann]

Will this actually fix #3070?

[theodelrieu]

Yes, it will properly support std::filesystem::path on all platforms.

[nlohmann]

Thanks a lot!

[nlohmann]

While trying to fix #3090 with #3101, I begin to wonder whether it was a good idea to use std::filesystem in the library in the first place. Compiler support is much worse than I expected, and already detecting which header to use is difficult. In addition, linker flags may be needed to use std::filesystem on some systems.

I am currently thinking about guarding the conversion from/to std::filesystem::path with a preprocessor macro so users have the control whether they actually need it.

@theodelrieu What do you think?

[gregmarr]

I don't think the code using it will be instantiated unless it's actually used, so it shouldn't trigger linker flags if it's not used.

[nlohmann]

I think you're right. I fixed all but one compiler: #3101 (comment)