From 6d43aad742c7ea28303cf2799528188938e7ce32 Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Thu, 28 Sep 2023 12:58:51 -0400
Subject: [PATCH] span: Make Span template deduction guides work in SFINAE
 context

Also add test to make sure this doesn't get broken in the future.

This was breaking vector<bool> serialization in multiprocess code because
template current deduction guides would make it appear like vector<bool> could
be converted to a span, but then the actual conversion to span would fail.
---
 src/Makefile.test.include |  1 +
 src/span.h                | 21 +++++++++--
 src/test/span_tests.cpp   | 73 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 src/test/span_tests.cpp

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index b610dabd075..33ff9688b2f 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -144,6 +144,7 @@ BITCOIN_TESTS =\
   test/sigopcount_tests.cpp \
   test/skiplist_tests.cpp \
   test/sock_tests.cpp \
+  test/span_tests.cpp \
   test/streams_tests.cpp \
   test/sync_tests.cpp \
   test/system_tests.cpp \
diff --git a/src/span.h b/src/span.h
index 7209d21a585..2e8da27cde2 100644
--- a/src/span.h
+++ b/src/span.h
@@ -222,15 +222,32 @@ public:
     template <typename O> friend class Span;
 };
 
+// Return result of calling .data() method on type T. This is used to be able to
+// write template deduction guides for the single-parameter Span constructor
+// below that will work if the value that is passed has a .data() method, and if
+// the data method does not return a void pointer.
+//
+// It is important to check for the void type specifically below, so the
+// deduction guides can be used in SFINAE contexts to check whether objects can
+// be converted to spans. If the deduction guides did not explicitly check for
+// void, and an object was passed that returned void* from data (like
+// std::vector<bool>), the template deduction would succeed, but the Span<void>
+// object instantiation would fail, resulting in a hard error, rather than a
+// SFINAE error.
+// https://stackoverflow.com/questions/68759148/sfinae-to-detect-the-explicitness-of-a-ctad-deduction-guide
+// https://stackoverflow.com/questions/16568986/what-happens-when-you-call-data-on-a-stdvectorbool
+template<typename T>
+using DataResult = std::remove_pointer_t<decltype(std::declval<T&>().data())>;
+
 // Deduction guides for Span
 // For the pointer/size based and iterator based constructor:
 template <typename T, typename EndOrSize> Span(T*, EndOrSize) -> Span<T>;
 // For the array constructor:
 template <typename T, std::size_t N> Span(T (&)[N]) -> Span<T>;
 // For the temporaries/rvalue references constructor, only supporting const output.
-template <typename T> Span(T&&) -> Span<std::enable_if_t<!std::is_lvalue_reference_v<T>, const std::remove_pointer_t<decltype(std::declval<T&&>().data())>>>;
+template <typename T> Span(T&&) -> Span<std::enable_if_t<!std::is_lvalue_reference_v<T> && !std::is_void_v<DataResult<T&&>>, const DataResult<T&&>>>;
 // For (lvalue) references, supporting mutable output.
-template <typename T> Span(T&) -> Span<std::remove_pointer_t<decltype(std::declval<T&>().data())>>;
+template <typename T> Span(T&) -> Span<std::enable_if_t<!std::is_void_v<DataResult<T&>>, DataResult<T&>>>;
 
 /** Pop the last element off a span, and return a reference to that element. */
 template <typename T>
diff --git a/src/test/span_tests.cpp b/src/test/span_tests.cpp
new file mode 100644
index 00000000000..f6cac10b09b
--- /dev/null
+++ b/src/test/span_tests.cpp
@@ -0,0 +1,73 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <span.h>
+
+#include <boost/test/unit_test.hpp>
+#include <array>
+#include <set>
+#include <vector>
+
+namespace {
+struct Ignore
+{
+    template<typename T> Ignore(T&&) {}
+};
+template<typename T>
+bool Spannable(T&& value, decltype(Span{value})* enable = nullptr)
+{
+    return true;
+}
+bool Spannable(Ignore)
+{
+    return false;
+}
+
+#if defined(__clang__)
+#    pragma clang diagnostic push
+#    pragma clang diagnostic ignored "-Wunneeded-member-function"
+#    pragma clang diagnostic ignored "-Wunused-member-function"
+#endif
+struct SpannableYes
+{
+    int* data();
+    size_t size();
+};
+struct SpannableNo
+{
+    void* data();
+    size_t size();
+};
+#if defined(__clang__)
+#    pragma clang diagnostic pop
+#endif
+} // namespace
+
+BOOST_AUTO_TEST_SUITE(span_tests)
+
+// Make sure template Span template deduction guides accurately enable calls to
+// Span constructor overloads that work, and disable calls to constructor overloads that
+// don't work. This makes it is possible to use the Span constructor in a SFINAE
+// contexts like in the Spannable function above to detect whether types are or
+// aren't compatible with Spans at compile time.
+//
+// Previously there was a bug where writing a SFINAE check for vector<bool> was
+// not possible, because in libstdc++ vector<bool> has a data() memeber
+// returning void*, and the Span template guide ignored the data() return value,
+// so the template substitution would succeed, but the constructor would fail,
+// resulting in a fatal compile error, rather than a SFINAE error that could be
+// handled.
+BOOST_AUTO_TEST_CASE(span_constructor_sfinae)
+{
+    BOOST_CHECK(Spannable(std::vector<int>{}));
+    BOOST_CHECK(!Spannable(std::set<int>{}));
+    BOOST_CHECK(!Spannable(std::vector<bool>{}));
+    BOOST_CHECK(Spannable(std::array<int, 3>{}));
+    BOOST_CHECK(Spannable(Span<int>{}));
+    BOOST_CHECK(Spannable("char array"));
+    BOOST_CHECK(Spannable(SpannableYes{}));
+    BOOST_CHECK(!Spannable(SpannableNo{}));
+}
+
+BOOST_AUTO_TEST_SUITE_END()