Add the notion of "checked iterators" to WIL (#422)

* Debug iterators for vector_range_nothrow

* Switch to an assert-like macro

* Checks for iterable_iterator_nothrow

* Forgot the namespace

* Revert to previous begin behavior

* Tests plus minor bug fixing

* clang-format

* Comment

* copy paste

include/wil/common.h

@@ -306,6 +306,35 @@ static_assert(WIL_EXCEPTION_MODE <= 2, "Invalid exception mode");
 #error Must enable exceptions when WIL_EXCEPTION_MODE == 1
 #endif
 
+/// @cond
+#ifndef WIL_ITERATOR_DEBUG_LEVEL
+// NOTE: See the definition of 'RESULT_DEBUG' for commentary on the use of 'WIL_KERNEL_MODE' below
+#if (DBG || defined(DEBUG) || defined(_DEBUG)) && (defined(WIL_KERNEL_MODE) || !defined(NDEBUG))
+#define WIL_ITERATOR_DEBUG_LEVEL 2
+#else
+#define WIL_ITERATOR_DEBUG_LEVEL 0
+#endif
+#endif
+
+#if (WIL_ITERATOR_DEBUG_LEVEL < 0) || (WIL_ITERATOR_DEBUG_LEVEL > 2)
+#error Invalid value for 'WIL_ITERATOR_DEBUG_LEVEL'; valid values are 0-2
+#endif
+
+// To allow code with mis-matching iterator debug levels to link together without fear of ODR issues, we place iterators whose
+// definitions differ based on the definition of WIL_ITERATOR_DEBUG_LEVEL in different namespaces
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+#define __WI_ITR_NAMESPACE WI_PASTE(itr, WIL_ITERATOR_DEBUG_LEVEL)
+#define __WI_ITR_NAMESPACE_BEGIN \
+    inline namespace __WI_ITR_NAMESPACE \
+    {
+#define __WI_ITR_NAMESPACE_END }
+#else
+#define __WI_ITR_NAMESPACE
+#define __WI_ITR_NAMESPACE_BEGIN
+#define __WI_ITR_NAMESPACE_END
+#endif
+/// @endcond
+
 // block for documentation only
 #if defined(WIL_DOXYGEN)
 /** This define can be explicitly set to disable exception usage within wil.
@@ -329,6 +358,18 @@ Three exception modes are available:
     enabled.
 2)  This locks the binary to libraries built without exceptions. */
 #define WIL_EXCEPTION_MODE
+
+/**This define controls the degree of runtime checking for various iterator types defined by WIL.
+This option roughly follows the behavior of the MSVC STL's `_ITERATOR_DEBUG_LEVEL` define, with similar available values. The
+primary difference (besides being two disjoint values) is that `WIL_ITERATOR_DEBUG_LEVEL` will raise a failfast exception when a
+check fails as opposed to the invalid parameter handler that the STL invokes. There are three definitions allowed:
+0)  This will disable all additional runtime checks for the various iterator types. This is the default when building as 'Release'
+1)  This enables checks only for unsafe iterator use. This includes things like attempting to increment an iterator past the end,
+    dereference an end iterator, dereference invalidated iterators, etc.
+2)  This enables all checks enabled by level 1 plus some additional checks to try and catch invalid iterator use. The specific
+    checks enabled by this level will vary between iterator types. This is the default when building as 'Debug'
+*/
+#define WIL_ITERATOR_DEBUG_LEVEL 0
 #endif
 
 /// @cond

include/wil/result_macros.h

@@ -62,9 +62,8 @@
 #define __WI_ASSERT_FAIL_ANNOTATION(msg) __annotation(L"Debug", L"AssertFail", msg)
 #endif
 
-#define WI_ASSERT(condition) \
-    (__WI_ANALYSIS_ASSUME(condition), \
-     ((!(condition)) ? (__WI_ASSERT_FAIL_ANNOTATION(L"" #condition), DbgRaiseAssertionFailure(), FALSE) : TRUE))
+#define WI_ASSERT_FAIL(msg) __WI_ASSERT_FAIL_ANNOTATION(L"" msg), DbgRaiseAssertionFailure()
+#define WI_ASSERT(condition) (__WI_ANALYSIS_ASSUME(condition), ((!(condition)) ? (WI_ASSERT_FAIL(#condition), FALSE) : TRUE))
 #define WI_ASSERT_MSG(condition, msg) \
     (__WI_ANALYSIS_ASSUME(condition), ((!(condition)) ? (__WI_ASSERT_FAIL_ANNOTATION(L##msg), DbgRaiseAssertionFailure(), FALSE) : TRUE))
 #define WI_ASSERT_NOASSUME WI_ASSERT
@@ -73,6 +72,7 @@
 #define WI_VERIFY_MSG WI_ASSERT_MSG
 #define WI_VERIFY_SUCCEEDED(condition) WI_ASSERT(SUCCEEDED(condition))
 #else
+#define WI_ASSERT_FAIL(msg)
 #define WI_ASSERT(condition) (__WI_ANALYSIS_ASSUME(condition), 0)
 #define WI_ASSERT_MSG(condition, msg) (__WI_ANALYSIS_ASSUME(condition), 0)
 #define WI_ASSERT_NOASSUME(condition) ((void)0)
@@ -1260,6 +1260,26 @@ WI_ODR_PRAGMA("WIL_FreeMemory", "0")
         } \
     } while ((void)0, 0)
 
+// Like 'FAIL_FAST_IF', but raises an assertion failure first for easier debugging
+#define FAIL_FAST_ASSERT(condition) \
+    do \
+    { \
+        if (!wil::verify_bool(condition)) \
+        { \
+            WI_ASSERT_FAIL(#condition); \
+            __RFF_FN(FailFast_Unexpected)(__RFF_INFO_ONLY(#condition)) \
+        } \
+    } while (0, 0)
+#define FAIL_FAST_ASSERT_MSG(condition, msg) \
+    do \
+    { \
+        if (!wil::verify_bool(condition)) \
+        { \
+            WI_ASSERT_FAIL(msg); \
+            __RFF_FN(FailFast_UnexpectedMsg)(__RFF_INFO(#condition) __WI_CHECK_MSG_FMT(msg)); \
+        } \
+    } while (0, 0)
+
 //*****************************************************************************
 // Macros to throw exceptions on failure
 //*****************************************************************************

include/wil/winrt.h

@@ -769,6 +769,8 @@ class vector_range
 
 #pragma region error code based IVector<>/IVectorView<>
 
+__WI_ITR_NAMESPACE_BEGIN
+
 template <typename VectorType>
 class vector_range_nothrow
 {
@@ -807,32 +809,39 @@ class vector_range_nothrow
         typedef const TSmart& reference;
 
         vector_iterator_nothrow() = delete;
-        vector_iterator_nothrow(vector_range_nothrow<VectorType>* range, unsigned int pos) : m_range(range), m_i(pos)
+        vector_iterator_nothrow(vector_range_nothrow<VectorType>* range, unsigned int pos) :
+            m_range(range),
+            m_i(pos)
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            ,
+            m_version(range->m_version)
+#endif
         {
         }
 
         WI_NODISCARD reference operator*() const
         {
-            return m_range->m_currentElement;
+            return *this->operator->();
         }
 
         WI_NODISCARD pointer operator->() const
         {
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            FAIL_FAST_ASSERT_MSG(m_version == m_range->m_version, "Dereferencing an out-of-date vector_iterator_nothrow");
+            FAIL_FAST_ASSERT_MSG(SUCCEEDED(*m_range->m_result), "Dereferencing a vector_iterator_nothrow in a failed state");
+            FAIL_FAST_ASSERT_MSG(m_i < m_range->m_size, "Dereferencing an 'end' iterator");
+#endif
             return wistd::addressof(m_range->m_currentElement);
         }
 
         vector_iterator_nothrow& operator++()
         {
-            ++m_i;
-            m_range->get_at_current(m_i);
-            return *this;
+            return *this += 1;
         }
 
         vector_iterator_nothrow& operator--()
         {
-            --m_i;
-            m_range->get_at_current(m_i);
-            return *this;
+            return *this += -1;
         }
 
         vector_iterator_nothrow operator++(int)
@@ -851,16 +860,27 @@ class vector_range_nothrow
 
         vector_iterator_nothrow& operator+=(int n)
         {
+#if WIL_ITERATOR_DEBUG_LEVEL == 2
+            // This is _technically_ safe because we are not reading the out-of-date cached value, however having two active
+            // copies of iterators is generally a sign of problematic code with a bug waiting to happen
+            FAIL_FAST_ASSERT_MSG(
+                m_version == m_range->m_version, "Incrementing/decrementing an out-of-date copy of a vector_iterator_nothrow");
+#endif
             m_i += n;
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            // NOTE: 'm_i' is unsigned, hence the single check for increment/decrement
+            FAIL_FAST_ASSERT_MSG(m_i <= m_range->m_size, "Incrementing/decrementing a vector_iterator_nothrow out of range");
+#endif
             m_range->get_at_current(m_i);
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            m_version = m_range->m_version;
+#endif
             return *this;
         }
 
         vector_iterator_nothrow& operator-=(int n)
         {
-            m_i -= n;
-            m_range->get_at_current(m_i);
-            return *this;
+            return *this += -n;
         }
 
         WI_NODISCARD bool operator==(vector_iterator_nothrow const& other) const
@@ -876,10 +896,17 @@ class vector_range_nothrow
     private:
         vector_range_nothrow<VectorType>* m_range;
         unsigned int m_i = 0;
+
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+        // For checked iterator support; must match the version in 'm_range'
+        int m_version = 0;
+#endif
     };
 
     vector_iterator_nothrow begin()
     {
+        // NOTE: Calling 'begin()' more than once is explicitly allowed as a way to reset the range, however because state is
+        // shared between all iterators, this invalidates previously created iterators
         get_at_current(0);
         return vector_iterator_nothrow(this, 0);
     }
@@ -899,6 +926,10 @@ class vector_range_nothrow
         if (SUCCEEDED(*m_result) && (i < m_size))
         {
             *m_result = m_v->GetAt(i, m_currentElement.ReleaseAndGetAddressOf());
+
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            ++m_version;
+#endif
         }
     }
 
@@ -911,8 +942,15 @@ class vector_range_nothrow
     HRESULT* m_result;
     HRESULT m_resultStorage = S_OK; // for the case where the caller does not provide the location to store the result
     TSmart m_currentElement;
+
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+    // For checked iterator support
+    int m_version = 0;
+#endif
 };
 
+__WI_ITR_NAMESPACE_END
+
 #pragma endregion
 
 #pragma region exception and fail fast based IIterable<>
@@ -966,9 +1004,12 @@ class iterable_range
 
         iterable_iterator& operator=(const iterable_iterator& other)
         {
-            m_iterator = other.m_iterator;
-            m_i = other.m_i;
-            err_policy::HResult(other.m_element.CopyTo(m_element.ReleaseAndGetAddressOf()));
+            if (this != wistd::addressof(other))
+            {
+                m_iterator = other.m_iterator;
+                m_i = other.m_i;
+                err_policy::HResult(other.m_element.CopyTo(m_element.ReleaseAndGetAddressOf()));
+            }
             return *this;
         }
 
@@ -1074,6 +1115,9 @@ auto to_vector(VectorType* src)
 #endif
 
 #pragma region error code base IIterable<>
+
+__WI_ITR_NAMESPACE_BEGIN
+
 template <typename T>
 class iterable_range_nothrow
 {
@@ -1089,6 +1133,10 @@ class iterable_range_nothrow
     iterable_range_nothrow(iterable_range_nothrow&& other) WI_NOEXCEPT : m_iterator(wistd::move(other.m_iterator)),
                                                                          m_element(wistd::move(other.m_element)),
                                                                          m_resultStorage(other.m_resultStorage)
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+        ,
+                                                                         m_index(other.m_index)
+#endif
     {
         if (other.m_result == &other.m_resultStorage)
         {
@@ -1151,16 +1199,26 @@ class iterable_range_nothrow
 
         WI_NODISCARD reference operator*() const WI_NOEXCEPT
         {
-            return m_range->m_element;
+            return *this->operator->();
         }
 
         WI_NODISCARD pointer operator->() const WI_NOEXCEPT
         {
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            FAIL_FAST_ASSERT_MSG(SUCCEEDED(*m_range->m_result), "Dereferencing an iterable_iterator_nothrow in a failed state");
+            FAIL_FAST_ASSERT_MSG(m_i >= 0, "Dereferencing an 'end' iterator");
+            FAIL_FAST_ASSERT_MSG(m_i == m_range->m_index, "Dereferencing an out-of-date iterable_iterator_nothrow");
+#endif
             return wistd::addressof(m_range->m_element);
         }
 
         iterable_iterator_nothrow& operator++()
         {
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            // Failing this check is always bad because the iterator object we hold always advances forward
+            FAIL_FAST_ASSERT_MSG(m_i >= 0, "Incrementing an end iterator");
+            FAIL_FAST_ASSERT_MSG(m_i == m_range->m_index, "Incrementing an out-of-date copy of an iterable_iterator_nothrow");
+#endif
             boolean hasCurrent;
             *m_range->m_result = m_range->m_iterator->MoveNext(&hasCurrent);
             if (SUCCEEDED(*m_range->m_result) && hasCurrent)
@@ -1179,12 +1237,15 @@ class iterable_range_nothrow
             {
                 m_i = -1;
             }
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+            m_range->m_index = m_i;
+#endif
             return *this;
         }
 
-        iterable_range_nothrow operator++(int)
+        iterable_iterator_nothrow operator++(int)
         {
-            iterable_range_nothrow old(*this);
+            iterable_iterator_nothrow old(*this);
             ++*this;
             return old;
         }
@@ -1196,6 +1257,11 @@ class iterable_range_nothrow
 
     iterable_iterator_nothrow begin()
     {
+#if WIL_ITERATOR_DEBUG_LEVEL == 2
+        // The IIterator we hold only advances forward; we can't reset it back to the beginning. Calling this method more than
+        // once will very likely lead to unexpected behavior.
+        FAIL_FAST_ASSERT_MSG(m_index == 0, "Calling begin() on an already advanced iterable_range_nothrow");
+#endif
         return iterable_iterator_nothrow(this, this->m_iterator ? 0 : -1);
     }
 
@@ -1212,8 +1278,15 @@ class iterable_range_nothrow
     TSmart m_element;
     HRESULT* m_result;
     HRESULT m_resultStorage = S_OK;
+
+#if WIL_ITERATOR_DEBUG_LEVEL > 0
+    // For checked iterator support
+    int m_index = 0;
+#endif
 };
 
+__WI_ITR_NAMESPACE_END
+
 #pragma endregion
 
 #ifdef WIL_ENABLE_EXCEPTIONS