gen::dereference should perfectly-forward unwrapped values

Summary:
[Folly] `gen::dereference` should perfectly-forward unwrapped values.

The problem comes in when the wrapped value is not actually a pointer, but is actually an rvalue-ref to some other kind of wrapper type with `Inner&& operator*() &&`. In such cases, the compiler emits a type mismatch error that it cannot cast `Inner` to `Inner&&`, with the errors originating in `Dereference::foreach` and `Dereference::apply`.

Fixes a couple other missing-forwarding and extra-forwarding bugs.

Reviewed By: ddrcoder

Differential Revision: D3776617

fbshipit-source-id: 6926fc18244a572846b22d428bd407d37fb20aa1

diff --git a/folly/gen/Base-inl.h b/folly/gen/Base-inl.h
index 11308b3ea7d71fdd95472247ed04dabe69f7a945..8be8e01d2efb718b345c70d395c841ff5bc09ae8 100644 (file)
--- a/folly/gen/Base-inl.h
+++ b/folly/gen/Base-inl.h
@@ -1567,7 +1567,7 @@ class Dereference : public Operator<Dereference> {
     void foreach(Body&& body) const {
       source_.foreach([&](Value value) {
         if (value) {
-          return body(*value);
+          return body(*std::forward<Value>(value));
         }
       });
     }
@@ -1576,7 +1576,7 @@ class Dereference : public Operator<Dereference> {
     bool apply(Handler&& handler) const {
       return source_.apply([&](Value value) -> bool {
         if (value) {
-          return handler(*value);
+          return handler(*std::forward<Value>(value));
         }
         return true;
       });
@@ -1627,14 +1627,14 @@ class Indirect : public Operator<Indirect> {
     template <class Body>
     void foreach(Body&& body) const {
       source_.foreach([&](Value value) {
-        return body(&value);
+        return body(&std::forward<Value>(value));
       });
     }
 
     template <class Handler>
     bool apply(Handler&& handler) const {
       return source_.apply([&](Value value) -> bool {
-        return handler(&value);
+        return handler(&std::forward<Value>(value));
       });
     }
 
@@ -1964,6 +1964,11 @@ class Min : public Operator<Min<Selector, Comparer>> {
   Selector selector_;
   Comparer comparer_;
 
+  template <typename T>
+  const T& asConst(const T& t) const {
+    return t;
+  }
+
  public:
   Min() = default;
 
@@ -1984,9 +1989,9 @@ class Min : public Operator<Min<Selector, Comparer>> {
     Optional<StorageType> min;
     Optional<Key> minKey;
     source | [&](Value v) {
-      Key key = selector_(std::forward<Value>(v));
+      Key key = selector_(asConst(v)); // so that selector_ cannot mutate v
       if (!minKey.hasValue() || comparer_(key, minKey.value())) {
-        minKey = key;
+        minKey = std::move(key);
         min = std::forward<Value>(v);
       }
     };

diff --git a/folly/gen/test/BaseTest.cpp b/folly/gen/test/BaseTest.cpp
index fa5bcecbd0607d17d1a17a638f0cc8758576008a..66b987e445d0c9fb65960db9037f10996121181d 100644 (file)
--- a/folly/gen/test/BaseTest.cpp
+++ b/folly/gen/test/BaseTest.cpp
@@ -24,9 +24,10 @@
 #include <folly/FBVector.h>
 #include <folly/MapUtil.h>
 #include <folly/Memory.h>
+#include <folly/String.h>
 #include <folly/dynamic.h>
-#include <folly/gen/Base.h>
 #include <folly/experimental/TestUtil.h>
+#include <folly/gen/Base.h>
 
 using namespace folly::gen;
 using namespace folly;
@@ -1113,6 +1114,45 @@ TEST(Gen, Dereference) {
   }
 }
 
+namespace {
+struct DereferenceWrapper {
+  string data;
+  string& operator*() & {
+    return data;
+  }
+  string&& operator*() && {
+    return std::move(data);
+  }
+  explicit operator bool() {
+    return true;
+  }
+};
+bool operator==(const DereferenceWrapper& a, const DereferenceWrapper& b) {
+  return a.data == b.data;
+}
+void PrintTo(const DereferenceWrapper& a, std::ostream* o) {
+  *o << "Wrapper{\"" << cEscape<string>(a.data) << "\"}";
+}
+}
+
+TEST(Gen, DereferenceWithLValueRef) {
+  auto original = vector<DereferenceWrapper>{{"foo"}, {"bar"}};
+  auto copy = original;
+  auto expected = vector<string>{"foo", "bar"};
+  auto actual = from(original) | dereference | as<vector>();
+  EXPECT_EQ(expected, actual);
+  EXPECT_EQ(copy, original);
+}
+
+TEST(Gen, DereferenceWithRValueRef) {
+  auto original = vector<DereferenceWrapper>{{"foo"}, {"bar"}};
+  auto empty = vector<DereferenceWrapper>{{}, {}};
+  auto expected = vector<string>{"foo", "bar"};
+  auto actual = from(original) | move | dereference | as<vector>();
+  EXPECT_EQ(expected, actual);
+  EXPECT_EQ(empty, original);
+}
+
 TEST(Gen, Indirect) {
   vector<int> vs{1};
   EXPECT_EQ(&vs[0], from(vs) | indirect | first | unwrap);