[libc++] Fix complexity guarantee in ranges::clamp

[ldionne]

This patch prevents us from calling the projection more than 3 times in std::clamp, as required by the Standard.

Fixes #64717

[llvmbot]

@llvm/pr-subscribers-libcxx

Changes

This patch prevents us from calling the projection more than 3 times in std::clamp, as required by the Standard.

Fixes #64717

---

Full diff: https://github.com/llvm/llvm-project/pull/68413.diff

2 Files Affected:

• (modified) libcxx/include/__algorithm/ranges_clamp.h (+3-2)
• (modified) libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp (+29-20)

diff --git a/libcxx/include/__algorithm/ranges_clamp.h b/libcxx/include/__algorithm/ranges_clamp.h
index 9613f7f37720a6c..e6c86207254a19f 100644
--- a/libcxx/include/__algorithm/ranges_clamp.h
+++ b/libcxx/include/__algorithm/ranges_clamp.h
@@ -37,9 +37,10 @@ struct __fn {
     _LIBCPP_ASSERT_UNCATEGORIZED(!bool(std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __low))),
                                  "Bad bounds passed to std::ranges::clamp");
 
-    if (std::invoke(__comp, std::invoke(__proj, __value), std::invoke(__proj, __low)))
+    auto&& __projected = std::invoke(__proj, __value);
+    if (std::invoke(__comp, std::forward<decltype(__projected)>(__projected), std::invoke(__proj, __low)))
       return __low;
-    else if (std::invoke(__comp, std::invoke(__proj, __high), std::invoke(__proj, __value)))
+    else if (std::invoke(__comp, std::invoke(__proj, __high), std::forward<decltype(__projected)>(__projected)))
       return __high;
     else
       return __value;
diff --git a/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp b/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
index 036552f75de4eb4..35ef55a91e243d4 100644
--- a/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp
@@ -38,6 +38,16 @@ static_assert(!HasClamp<NoComp>);
 static_assert(!HasClamp<int, NoComp>);
 static_assert(!HasClamp<int, std::ranges::less, CreateNoComp>);
 
+struct EnsureValueCategoryComp {
+  constexpr bool operator()(const int&& x, const int&& y) const { return x < y; }
+  constexpr bool operator()(const int&& x, int& y) const { return x < y; }
+  constexpr bool operator()(int& x, const int&& y) const { return x < y; }
+  constexpr bool operator()(int& x, int& y) const { return x < y; }
+  constexpr bool operator()(std::same_as<const int&> auto&& x, std::same_as<const int&> auto&& y) const {
+    return x < y;
+  }
+};
+
 constexpr bool test() {
   { // low < val < high
     int val = 2;
@@ -71,6 +81,7 @@ constexpr bool test() {
 
       constexpr const int& lvalue_proj() const { return i; }
       constexpr int prvalue_proj() const { return i; }
+      constexpr bool operator==(S const& other) const { return i == other.i; }
     };
 
     struct Comp {
@@ -82,31 +93,29 @@ constexpr bool test() {
     auto low = S{20};
     auto high = S{30};
     // Check that the value category of the projection return type is preserved.
-    assert(&std::ranges::clamp(val, low, high, Comp{}, &S::lvalue_proj) == &low);
-    assert(&std::ranges::clamp(val, high, low, Comp{}, &S::prvalue_proj) == &low);
+    assert(std::ranges::clamp(val, low, high, Comp{}, &S::lvalue_proj) == low);
+    assert(std::ranges::clamp(val, high, low, Comp{}, &S::prvalue_proj) == low);
   }
 
-  { // Check that the implementation doesn't cause double moves (which could result from calling the projection on
-    // `value` once and then forwarding the result into the comparator).
-    struct CheckDoubleMove {
-      int i;
-      bool moved = false;
-
-      constexpr explicit CheckDoubleMove(int set_i) : i(set_i) {}
-      constexpr CheckDoubleMove(const CheckDoubleMove&) = default;
-      constexpr CheckDoubleMove(CheckDoubleMove&& rhs) noexcept : i(rhs.i) {
-        assert(!rhs.moved);
-        rhs.moved = true;
-      }
+  { // Ensure that we respect the value category of the projection when calling the comparator.
+    // This additional example was provided by Tim Song in https://github.com/microsoft/STL/issues/3970#issuecomment-1685120958.
+    struct MoveProj {
+      constexpr int const&& operator()(int const& x) const { return std::move(x); }
     };
 
-    auto val = CheckDoubleMove{20};
-    auto low = CheckDoubleMove{10};
-    auto high = CheckDoubleMove{30};
+    static_assert(std::indirect_strict_weak_order<EnsureValueCategoryComp, std::projected<const int*, MoveProj>>);
 
-    auto moving_comp = [](CheckDoubleMove lhs, CheckDoubleMove rhs) { return lhs.i < rhs.i; };
-    auto prvalue_proj = [](const CheckDoubleMove& x) -> CheckDoubleMove { return x; };
-    assert(&std::ranges::clamp(val, low, high, moving_comp, prvalue_proj) == &val);
+    assert(std::ranges::clamp(0, 1, 2, EnsureValueCategoryComp{}, MoveProj{}) == 1);
+  }
+
+  { // Make sure we don't call the projection more than three times per [alg.clamp], see #64717
+    int counter              = 0;
+    auto projection_function = [&counter](const int value) -> int {
+      counter++;
+      return value;
+    };
+    assert(std::ranges::clamp(3, 2, 4, std::ranges::less{}, projection_function) == 3);
+    assert(counter <= 3);
   }
 
   return true;

[EricWF]

I would much rather we simply pass the projection down to another function, which ensures the lifetime of the projection, even in weird cases.

[ldionne]

I don't understand. We're extending the lifetime here with rvalue references, is that what you would want to make more explicit? FWIW, this is how other implementations do it too, and lifetime extending with rvalue references is a fairly common pattern, so if that's what your concern is about my preference would be to keep it simple.

[EricWF]

OK, so if a user passes a projection which happens to implicitly create the projected object in the call to the projection, For example:

std::string const& project_string(std::string const&); // pass "abc"

We currently do accept code that does this, because the returned object is never used outside of the expression which created it. We can keep this behavior simply by creating a __clamp_impl function that takes the projected values as argument and then returns the result. Ex return __clamp_impl( __low, __high, _Comp __comp, std::invoke(__proj, __value), std::invoke(__proj, __low), std::invoke(__proj, __high)); // now the projections live the entire time

[ldionne]

Ok, so you are thinking about the following case?

char const* low = "1";
char const* high = "3";
char const* val = "2";
auto projection = [](std::string const& s) -> std::string const& { return s; };
std::ranges::clamp(val, low, high, std::less{}, projection);

Here what you're saying is that the s argument of the projection is created implicitly when we call std::invoke(__proj, __something), and if we end the full-expression before we are done using that result, we'll be using a dangling ref.

That seems to be correct according to the lifetime extension docs on cppreference:

There are following exceptions to this lifetime rule:

• a temporary bound to a reference parameter in a function call exists until the end of the full expression containing that function call: if the function returns a reference, which outlives the full expression, it becomes a dangling reference.

I think the "problem" I have with that is that it means we could technically never call any user-provided function (projection or else) and hold on to the result of that, since this case could happen anywhere. Every time we call a user-provided function with some arguments, there could be some implicit conversion happening and then we are vulnerable to this dangling reference issue. Am I right? If so, and if we want to guard against this, then we would in theory write all of our code which is potentially vulnerable to this issue in a single full expression. I'm not sure this is either reasonable or expected. WDYT?

[timsong-cpp]

This is always dangling no matter how you do it. This isn't __proj(__something), it's std::invoke(__proj, __something). No temporary is created for the call to std::invoke itself, that just binds a bunch of references. The full-expression with the temporary is somewhere inside the std::invoke machinery, and the temporary is destroyed by the time std::invoke returns.

[ldionne]

Ahhhh, yes, indeed. So then I don't see a way that we can make this "work" even if we wanted to.

[EricWF]

OK, so if a user passes a projection which happens to implicitly create the projected object in the call to the projection, For example:

std::string const& project_string(std::string const&); // pass "abc"

We currently do accept code that does this, because the returned object is never used outside of the expression which created it. We can keep this behavior simply by creating a __clamp_impl function that takes the projected values as argument and then returns the result. Ex return __clamp_impl( __low, __high, _Comp __comp, std::invoke(__proj, __value), std::invoke(__proj, __low), std::invoke(__proj, __high)); // now the projections live the entire time