Fix Debug assert in IntervalTree's is_valid() with til::point keys

oss/interval_tree/MAINTAINER_README.md

@@ -1,17 +1,32 @@
-# Notes for Future Maintainers
-
-This was originally imported by @PankajBhojwani in September 2020.
-
-The provenance information (where it came from and which commit) is stored in the file `cgmanifest.json` in the same directory as this readme.
-Please update the provenance information in that file when ingesting an updated version of the dependent library.
-That provenance file is automatically read and inventoried by Microsoft systems to ensure compliance with appropriate governance standards.
-
-## What should be done to update this in the future?
-
-1. Go to the ekg/intervaltree repository on GitHub.
-2. Take the file IntervalTree.h wholesale and drop it into the directory here.
-3. Don't change anything about it.
-4. Validate that the license in the root of the repository didn't change and update it if so. It is sitting in the same directory as this readme.
-   If it changed dramatically, ensure that it is still compatible with our license scheme. Also update the NOTICE file in the root of our repository to declare the third-party usage.
-5. Submit the pull.
-
+# Notes for Future Maintainers
+
+This was originally imported by @PankajBhojwani in September 2020.
+
+The provenance information (where it came from and which commit) is stored in the file `cgmanifest.json` in the same directory as this readme.
+Please update the provenance information in that file when ingesting an updated version of the dependent library.
+That provenance file is automatically read and inventoried by Microsoft systems to ensure compliance with appropriate governance standards.
+
+## Local deviation from upstream
+
+**`IntervalTree.h` here is _not_ a byte-for-byte copy of upstream.** Do not assume it is.
+`Scalar` is compared against `Scalar{}` instead of `0`, and `center` is default-initialized
+rather than set to `0`, so that a user-defined struct such as `til::point` can be used as a
+key. This came in with the original import in #7691 and corresponds to the still-open
+upstream pull request <https://github.com/ekg/intervaltree/pull/31>. Without it this file
+does not compile against `til::point` at all.
+
+Note that `is_valid()` assumes `std::numeric_limits<Scalar>` is meaningful. We satisfy that
+by specializing `std::numeric_limits` for the `til` coordinate types rather than by patching
+this file; see `src/inc/til/point.h` and #20486.
+
+## What should be done to update this in the future?
+
+1. Go to the ekg/intervaltree repository on GitHub.
+2. Take the file IntervalTree.h wholesale and drop it into the directory here.
+3. Don't change anything about it, **except** that you must re-apply the local deviation
+   listed above. Taking upstream wholesale without it will break the build, because
+   `til::point` cannot be compared against `0`.
+4. Validate that the license in the root of the repository didn't change and update it if so. It is sitting in the same directory as this readme.
+   If it changed dramatically, ensure that it is still compatible with our license scheme. Also update the NOTICE file in the root of our repository to declare the third-party usage.
+5. Submit the pull.
+

src/inc/LibraryIncludes.h

@@ -30,6 +30,7 @@
 #include <fstream>
 #include <functional>
 #include <iterator>
+#include <limits>
 #include <list>
 #include <map>
 #include <memory_resource>

src/inc/til/point.h

@@ -374,6 +374,32 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
     };
 }
 
+// til::point is ordered (lexicographically, by y then x), so the extremes are
+// well defined. Generic code that needs a "smaller/larger than anything" seed
+// value reaches for std::numeric_limits, and the unspecialized primary template
+// silently answers til::point{} for both max() and min() -- see GH#20486.
+template<>
+class std::numeric_limits<til::point>
+{
+public:
+    static constexpr bool is_specialized = true;
+
+    static constexpr til::point min() noexcept
+    {
+        return { til::CoordTypeMin, til::CoordTypeMin };
+    }
+
+    static constexpr til::point max() noexcept
+    {
+        return { til::CoordTypeMax, til::CoordTypeMax };
+    }
+
+    static constexpr til::point lowest() noexcept
+    {
+        return min();
+    }
+};
+
 #ifdef __WEX_COMMON_H__
 namespace WEX::TestExecution
 {

src/inc/til/size.h

@@ -206,6 +206,30 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
     }
 };
 
+// See the equivalent specialization in point.h. Without this, generic code that
+// seeds an accumulator with std::numeric_limits gets til::size{} for both ends.
+template<>
+class std::numeric_limits<til::size>
+{
+public:
+    static constexpr bool is_specialized = true;
+
+    static constexpr til::size min() noexcept
+    {
+        return { til::CoordTypeMin, til::CoordTypeMin };
+    }
+
+    static constexpr til::size max() noexcept
+    {
+        return { til::CoordTypeMax, til::CoordTypeMax };
+    }
+
+    static constexpr til::size lowest() noexcept
+    {
+        return min();
+    }
+};
+
 #ifdef __WEX_COMMON_H__
 namespace WEX::TestExecution
 {

src/til/ut_til/IntervalTreeTests.cpp

@@ -0,0 +1,75 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+#include "precomp.h"
+
+#include "til/point.h"
+
+using namespace WEX::Common;
+using namespace WEX::Logging;
+using namespace WEX::TestExecution;
+
+// These tests cover our use of oss/interval_tree with a non-arithmetic Scalar.
+// See oss/interval_tree/MAINTAINER_README.md for the local deviations involved.
+class IntervalTreeTests
+{
+    TEST_CLASS(IntervalTreeTests);
+
+    using PointTree = interval_tree::IntervalTree<til::point, size_t>;
+
+    // The tree only splits into subtrees once it holds at least 64 intervals (its
+    // default minimum bucket size), so anything below that never exercised the
+    // subtree constraints.
+    static PointTree _makeTree(size_t count)
+    {
+        PointTree::interval_vector intervals;
+        intervals.reserve(count);
+        for (size_t i = 0; i < count; ++i)
+        {
+            const auto y = gsl::narrow<til::CoordType>(i);
+            intervals.push_back(PointTree::interval({ 0, y }, { 10, y }, i));
+        }
+        return PointTree{ std::move(intervals) };
+    }
+
+    // GH#20486: is_valid() seeds its bounds accumulators from
+    // std::numeric_limits<Scalar>. Without a specialization the primary template
+    // answers til::point{0,0} for both ends, which is not a usable sentinel.
+    TEST_METHOD(NumericLimitsAreUsableSentinels)
+    {
+        VERIFY_IS_TRUE(std::numeric_limits<til::point>::is_specialized);
+        VERIFY_IS_TRUE(std::numeric_limits<til::point>::min() < til::point{});
+        VERIFY_IS_TRUE(std::numeric_limits<til::point>::max() > til::point{});
+        VERIFY_ARE_EQUAL(std::numeric_limits<til::point>::min(), std::numeric_limits<til::point>::lowest());
+
+        VERIFY_IS_TRUE(std::numeric_limits<til::size>::is_specialized);
+        VERIFY_ARE_EQUAL(til::CoordTypeMax, std::numeric_limits<til::size>::max().width);
+        VERIFY_ARE_EQUAL(til::CoordTypeMin, std::numeric_limits<til::size>::min().height);
+    }
+
+    // Before GH#20486 this asserted in Debug builds for any tree with 64 or more
+    // intervals -- i.e. whenever 64+ URLs were autodetected on screen.
+    TEST_METHOD(IsValidWithSubtrees)
+    {
+        for (const size_t count : { 1u, 63u, 64u, 65u, 512u })
+        {
+            const auto tree = _makeTree(count);
+            VERIFY_IS_TRUE(tree.is_valid().first, NoThrowString().Format(L"%zu intervals", count));
+        }
+    }
+
+    // A tree that reports itself valid must also still find what it contains.
+    TEST_METHOD(FindsIntervalsAcrossSubtrees)
+    {
+        static constexpr size_t count = 200;
+        const auto tree = _makeTree(count);
+
+        for (size_t i = 0; i < count; ++i)
+        {
+            const auto y = gsl::narrow<til::CoordType>(i);
+            const auto results = tree.findOverlapping({ 5, y }, { 5, y });
+            VERIFY_ARE_EQUAL(1u, results.size(), NoThrowString().Format(L"row %zu", i));
+            VERIFY_ARE_EQUAL(i, results.front().value);
+        }
+    }
+};

src/til/ut_til/sources

@@ -20,6 +20,7 @@ SOURCES = \
     EnumSetTests.cpp \
     EnvTests.cpp \
     HashTests.cpp \
+    IntervalTreeTests.cpp \
     MathTests.cpp \
     mutex.cpp \
     OperatorTests.cpp \

src/til/ut_til/til.unit.tests.vcxproj

@@ -22,6 +22,7 @@
     <ClCompile Include="FlatSetTests.cpp" />
     <ClCompile Include="GenerationalTests.cpp" />
     <ClCompile Include="HashTests.cpp" />
+    <ClCompile Include="IntervalTreeTests.cpp" />
     <ClCompile Include="MathTests.cpp" />
     <ClCompile Include="mutex.cpp" />
     <ClCompile Include="OperatorTests.cpp" />

src/til/ut_til/til.unit.tests.vcxproj.filters

@@ -12,9 +12,11 @@
     <ClCompile Include="ColorTests.cpp" />
     <ClCompile Include="EnumSetTests.cpp" />
     <ClCompile Include="HashTests.cpp" />
+    <ClCompile Include="IntervalTreeTests.cpp" />
     <ClCompile Include="MathTests.cpp" />
     <ClCompile Include="mutex.cpp" />
     <ClCompile Include="OperatorTests.cpp" />
+    <ClCompile Include="IntervalTreeTests.cpp" />
     <ClCompile Include="PointTests.cpp" />
     <ClCompile Include="RectangleTests.cpp" />
     <ClCompile Include="ReplaceTests.cpp" />