From e1d3d3a32682e840b3f8014980599488a7386cc9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Kr=C3=BCgler?= <daniel.kruegler@gmail.com>
Date: Sun, 10 Jul 2022 15:26:23 +0200
Subject: [PATCH] Exclude recursive ranges from the formatter specialization
 for ranges (#2974)

* 2954: Add test case

* Eliminate extra-test and merge it into existing std-test instead. Add conditionals for filesystem::path testing that does not run into the ambiguity problem.

* #2968: Introduce additional compile-time predicate to detect recursive ranges and reject them in formatter specialization for ranges. In addition, introduce additional wrapper traits for the individual logical operands of the complete range constraints

* #2968: Eliminate preprocessor condition that enables the formatter specialization for std::filesystem::path

* #2968: Eliminate preprocessor condition that enables the test for the formatter specialization for std::filesystem::path

* Use own bool_constant, which is available for all C++ versions

* Reintroduce previous workaround but restrict to VS 2015 for now

* Comma fix

* - Rename is_not_recursive_range to is_nonrecursive_range and add comment that explains it being depending on is_range being true
- Merge has_fallback_formatter_delayed into is_formattable_delayed and add comment that explains it being depending on is_not_recursive_range being true
- Replace disjunction in formatter specialization by has_fallback_formatter_delayed
- Get rid of unneeded detail:: prefixes within namespace detail
---
 include/fmt/ranges.h | 33 +++++++++++++++++++++++----------
 include/fmt/std.h    |  5 -----
 test/std-test.cc     | 11 +++--------
 3 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/fmt/ranges.h b/include/fmt/ranges.h
index 10429fc8..8b7c9aad 100644
--- a/include/fmt/ranges.h
+++ b/include/fmt/ranges.h
@@ -390,22 +390,35 @@ using range_formatter_type = conditional_t<
 template <typename R>
 using maybe_const_range =
     conditional_t<has_const_begin_end<R>::value, const R, R>;
+
+// is_nonrecursive_range depends on fmt::is_range<T, Char>::value == true.
+// It exists to ensure short-circuit evaluation in the constraint of the 
+// formatter specialization below. A similar approach is used in 
+// https://wg21.link/p2286.
+template <typename R>
+struct is_nonrecursive_range : bool_constant<
+    !std::is_same<uncvref_type<R>, R>::value> {};
+
+// is_formattable_delayed depends on is_nonrecursive_range<R>::value == true.
+// It exists to ensure short-circuit evaluation in the constraint of the 
+// formatter specialization below.
+template <typename R, typename Char>
+struct is_formattable_delayed : disjunction<
+    is_formattable<uncvref_type<maybe_const_range<R>>, Char>,
+    has_fallback_formatter<uncvref_type<maybe_const_range<R>>, Char>> {};
+
 }  // namespace detail
 
 template <typename R, typename Char>
 struct formatter<
     R, Char,
     enable_if_t<
-        conjunction<fmt::is_range<R, Char>
-// Workaround a bug in MSVC 2017 and earlier.
-#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
-        ,
-        disjunction<
-          is_formattable<detail::uncvref_type<detail::maybe_const_range<R>>,
-                         Char>,
-          detail::has_fallback_formatter<
-             detail::uncvref_type<detail::maybe_const_range<R>>, Char>
-        >
+        conjunction<fmt::is_range<R, Char>,
+          detail::is_nonrecursive_range<R>
+// Workaround a bug in MSVC 2015 and earlier.
+#if !FMT_MSC_VERSION || FMT_MSC_VERSION > 1900
+          ,
+          detail::is_formattable_delayed<R, Char>
 #endif
         >::value
         >> {
diff --git a/include/fmt/std.h b/include/fmt/std.h
index 227f4841..41d2b283 100644
--- a/include/fmt/std.h
+++ b/include/fmt/std.h
@@ -57,10 +57,6 @@ inline void write_escaped_path<std::filesystem::path::value_type>(
 
 }  // namespace detail
 
-#if !FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920
-// For MSVC 2017 and earlier using the partial specialization
-// would cause an ambiguity error, therefore we provide it only
-// conditionally.
 template <typename Char>
 struct formatter<std::filesystem::path, Char>
     : formatter<basic_string_view<Char>> {
@@ -73,7 +69,6 @@ struct formatter<std::filesystem::path, Char>
         basic_string_view<Char>(quoted.data(), quoted.size()), ctx);
   }
 };
-#endif
 FMT_END_NAMESPACE
 #endif
 
diff --git a/test/std-test.cc b/test/std-test.cc
index fc8f72a0..c22b3e38 100644
--- a/test/std-test.cc
+++ b/test/std-test.cc
@@ -14,10 +14,7 @@
 #include "gtest/gtest.h"
 
 TEST(std_test, path) {
-// Test ambiguity problem described in #2954. We need to exclude compilers
-// where the ambiguity problem cannot be solved for now.
-#if defined(__cpp_lib_filesystem) && \
-    (!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920)
+#ifdef __cpp_lib_filesystem
   EXPECT_EQ(fmt::format("{:8}", std::filesystem::path("foo")), "\"foo\"   ");
   EXPECT_EQ(fmt::format("{}", std::filesystem::path("foo\"bar.txt")),
             "\"foo\\\"bar.txt\"");
@@ -37,10 +34,8 @@ TEST(std_test, path) {
 }
 
 TEST(ranges_std_test, format_vector_path) {
-// Test ambiguity problem described in #2954. We need to exclude compilers
-// where the ambiguity problem cannot be solved for now.
-#if defined(__cpp_lib_filesystem) && \
-    (!FMT_MSC_VERSION || FMT_MSC_VERSION >= 1920)
+// Test ambiguity problem described in #2954.
+#ifdef __cpp_lib_filesystem
   auto p = std::filesystem::path("foo/bar.txt");
   auto c = std::vector<std::string>{"abc", "def"};
   EXPECT_EQ(fmt::format("path={}, range={}", p, c),