[libc++] Prevent calling the projection more than three times

[pandaninjas]

Resolves #64717

[ldionne]

Thanks for looking into this! I have some comments but this is a great start!

[ldionne]

There is actually a problem here. We can potentially do a double-move. Imagine we have the following comparator:

struct Foo {
  std::string s;
};

// taking by value is important here
auto comparator = [](std::string a, std::string b) {
  return std::atoi(a.c_str()) < std::atoi(b.c_str());
};

auto projection = [](Foo const& foo) {
  return foo.s;
};

Foo foo{"12"};
Foo high{"10"};
Foo low{"1"};
std::ranges::clamp(foo, low, high, comparator, projection);

Here, we end up calling std::invoke(comp, forward<...>(projected), low), it returns false and then we call std::invoke(comp, forward<...>(projected), high). We end up moving projected twice into the invoke calls, so the second time we get an empty string.

Is that right? If so, this would need a test too.

[ldionne]

Ok, so actually it turns out that MSVC does the same thing here and they went through the exact same questioning as us, see microsoft/STL#1898 (comment).

However, I played around and I wasn't able to craft a case where passing an lvalue would fail (despite what @timsong-cpp said in that review). Unless we are able to come up with one, I think it would be reasonable to pass a lvalue.

[timsong-cpp]

microsoft/STL#3970 (comment)

[ldionne]

Ah, thanks a lot! Like I said in the other comment, @pandaninjas let's add a test for that and go back to std::forward. Then I think this patch will be good to go.

[ldionne]

What a tricky change! The ones that seem simple are sometimes the ones that end up being the most involved :)

[pandaninjas]

Any idea why the CI is failing? It mentions line 86 not being constexpr because of assert, but it doesn't seem like I've done anything which should affect that...

[ldionne]

Any idea why the CI is failing? It mentions line 86 not being constexpr because of assert, but it doesn't seem like I've done anything which should affect that...

I looked into it a bit and I think the CI is failing because of the std::forward problem. The assertion on line 86 fails because we don't respect the value category returned by the projection, which is basically that problem.

[ldionne]

Ah, thanks a lot! Like I said in the other comment, @pandaninjas let's add a test for that and go back to std::forward. Then I think this patch will be good to go.

[ldionne]

We don't want to use std::forward here, we want to use it in the argument to std::invoke.

[pandaninjas]

It seems like tests are still failing because there is a double move.

[frederick-vs-ja]

It seems that we should remove this block because it turns out that ranges::clamp needs double moves under some circumstances.

llvm-project/libcxx/test/std/algorithms/alg.sorting/alg.clamp/ranges.clamp.pass.cpp

Lines 89 to 110 in 4c1c96e

	{ // 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;
	}
	};
	auto val = CheckDoubleMove{20};
	auto low = CheckDoubleMove{10};
	auto high = CheckDoubleMove{30};
	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);
	}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ldionne]

This seems to be a bit stuck. I checked out the patch locally and that was helpful to understand some of the issues. I think the final patch should be:

commit c667f036c4228b9cf0172c311537ab96ef61fcf5
Author: PandaNinjas <[email protected]>
Date:   Wed Sep 13 17:38:17 2023 -0700

    [libc++] Prevent calling the projection more than three times
    
    Resolves #64717

diff --git a/libcxx/include/__algorithm/ranges_clamp.h b/libcxx/include/__algorithm/ranges_clamp.h
index 9613f7f37720..e6c86207254a 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 036552f75de4..35ef55a91e24 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;

Can you update the PR accordingly? Also, in the future I believe that following the instructions in https://libcxx.llvm.org/TestingLibcxx.html#usage would make it a lot easier to see issues in testing before running them through the CI. Most of the issues I fixed in the patch locally were pretty immediately visible when running the tests locally.

[ldionne]

Picked up in #68413.