Use ordered set for enumerating map property keys

Summary:
We want two properties of the key set:
1. Unique — each property only needs to be enumerated once
2. Deterministic order — when we print or iterate using `(map.items ...)` we want the order to be deterministic. Otherwise, different runs of whisker could produce different outputs.

`std::set` fits both of these requirements.

Reviewed By: rmakheja

Differential Revision: D68527462

fbshipit-source-id: 3bd51dd21315d5a5d1ad65afccbc598825645aec

thrift/compiler/whisker/dsl.cc

@@ -68,18 +68,17 @@ object::ptr map_like::lookup_property(std::string_view identifier) const {
       });
 }
 
-std::optional<std::vector<std::string>> map_like::keys() const {
-  using result = std::optional<std::vector<std::string>>;
+std::optional<std::set<std::string>> map_like::keys() const {
+  using result = std::optional<std::set<std::string>>;
   return whisker::detail::variant_match(
       which_,
       [&](const native_object::map_like::ptr& m) -> result {
         return m->keys();
       },
       [&](const managed_ptr<map>& m) -> result {
-        std::vector<std::string> keys;
-        keys.reserve(m->size());
+        std::set<std::string> keys;
         for (const auto& [key, _] : *m) {
-          keys.push_back(key);
+          keys.insert(key);
         }
         return keys;
       });

thrift/compiler/whisker/dsl.h

@@ -73,7 +73,7 @@ static_assert(std::is_copy_constructible_v<array_like>);
 class map_like final : public native_object::map_like {
  public:
   object::ptr lookup_property(std::string_view identifier) const final;
-  std::optional<std::vector<std::string>> keys() const final;
+  std::optional<std::set<std::string>> keys() const final;
 
   /**
    * Tries to marshal the provided object into an map-like object, if the

thrift/compiler/whisker/mstch_compat.cc

@@ -112,11 +112,10 @@ class mstch_map_proxy final
     return nullptr;
   }
 
-  std::optional<std::vector<std::string>> keys() const override {
-    std::vector<std::string> property_names;
-    property_names.reserve(proxied_.size());
+  std::optional<std::set<std::string>> keys() const override {
+    std::set<std::string> property_names;
     for (const auto& [key, _] : proxied_) {
-      property_names.push_back(key);
+      property_names.insert(key);
     }
     return property_names;
   }
@@ -162,7 +161,7 @@ class mstch_object_proxy
         [](object::ptr o) -> object::ptr { return o; });
   }
 
-  std::optional<std::vector<std::string>> keys() const override {
+  std::optional<std::set<std::string>> keys() const override {
     return proxied_->property_names();
   }
 

thrift/compiler/whisker/mstch_compat.h

@@ -54,10 +54,10 @@ class object_t {
     return methods_.find(name) != methods_.end();
   }
 
-  std::vector<std::string> property_names() const {
-    std::vector<std::string> result;
+  std::set<std::string> property_names() const {
+    std::set<std::string> result;
     for (const auto& [name, _] : methods_) {
-      result.push_back(name);
+      result.insert(name);
     }
     return result;
   }

thrift/compiler/whisker/object.cc

@@ -194,25 +194,14 @@ bool array_eq(
   return true;
 }
 
-namespace {
-std::optional<std::unordered_set<std::string>> unique_keys(
-    std::optional<std::vector<std::string>> keys) {
-  if (!keys.has_value()) {
-    return std::nullopt;
-  }
-  return std::unordered_set<std::string>(
-      std::make_move_iterator(keys->begin()),
-      std::make_move_iterator(keys->end()));
-}
-} // namespace
 bool map_eq(const map& lhs, const map& rhs) {
   return lhs == rhs;
 }
 bool map_eq(const map& lhs, const native_object::map_like::ptr& rhs) {
   if (rhs == nullptr) {
     return false;
   }
-  auto rhs_keys = unique_keys(rhs->keys());
+  auto rhs_keys = rhs->keys();
   if (!rhs_keys.has_value()) {
     // Not enumerable
     return false;
@@ -244,8 +233,8 @@ bool map_eq(
     return false;
   }
 
-  auto lhs_keys = unique_keys(lhs->keys());
-  auto rhs_keys = unique_keys(rhs->keys());
+  auto lhs_keys = lhs->keys();
+  auto rhs_keys = rhs->keys();
   const bool keys_equal =
       lhs_keys.has_value() && rhs_keys.has_value() && *lhs_keys == *rhs_keys;
   if (!keys_equal) {
@@ -264,7 +253,7 @@ bool map_eq(
 
 void native_object::map_like::default_print_to(
     std::string_view name,
-    std::vector<std::string> property_names,
+    const std::set<std::string>& property_names,
     tree_printer::scope scope,
     const object_print_options& options) const {
   assert(scope.semantic_depth() <= options.max_depth);
@@ -430,11 +419,10 @@ object make::proxy(const object::ptr& source) {
             }
             return nullptr;
           }
-          std::optional<std::vector<std::string>> keys() const final {
-            std::vector<std::string> property_names;
-            property_names.reserve(map_->size());
+          std::optional<std::set<std::string>> keys() const final {
+            std::set<std::string> property_names;
             for (const auto& [key, _] : *map_) {
-              property_names.push_back(key);
+              property_names.insert(key);
             }
             return property_names;
           }

thrift/compiler/whisker/object.h

@@ -24,6 +24,7 @@
 #include <limits>
 #include <map>
 #include <memory>
+#include <set>
 #include <string>
 #include <string_view>
 #include <typeinfo>
@@ -162,16 +163,16 @@ class native_object {
         std::string_view identifier) const = 0;
 
     /**
-     * Returns an ordered list of finitely enumerable property names of this
+     * Returns an ordered set of finitely enumerable property names of this
      * map-like object.
      *
      * If property names are not enumerable (i.e. dynamically generated), then
      * this returns the empty optional.
      *
-     * For each name returned in this list, lookup_property must not return
+     * For each name returned in this set, lookup_property must not return
      * nullptr.
      */
-    virtual std::optional<std::vector<std::string>> keys() const {
+    virtual std::optional<std::set<std::string>> keys() const {
       return std::nullopt;
     }
 
@@ -187,7 +188,7 @@ class native_object {
      */
     void default_print_to(
         std::string_view name,
-        std::vector<std::string> property_names,
+        const std::set<std::string>& property_names,
         tree_printer::scope,
         const object_print_options&) const;
   };

thrift/compiler/whisker/standard_library.cc

@@ -20,6 +20,8 @@
 
 #include <fmt/core.h>
 
+#include <iterator>
+
 namespace w = whisker::make;
 
 namespace whisker {
@@ -189,8 +191,11 @@ map::value_type create_map_functions() {
           throw ctx.make_error(
               "map-like object does not have enumerable properties.");
         }
-        auto view =
-            w::make_native_object<items_view>(std::move(m), std::move(*keys));
+        auto view = w::make_native_object<items_view>(
+            std::move(m),
+            std::vector<std::string>(
+                std::make_move_iterator(keys->begin()),
+                std::make_move_iterator(keys->end())));
         return manage_owned<object>(std::move(view));
       });
 

thrift/compiler/whisker/test/object_test.cc

@@ -147,7 +147,7 @@ TEST(ObjectTest, native_object_equality) {
   struct always_zero_map : native_object,
                            native_object::map_like,
                            std::enable_shared_from_this<always_zero_map> {
-    explicit always_zero_map(std::vector<std::string> keys)
+    explicit always_zero_map(std::set<std::string> keys)
         : keys_(std::move(keys)) {}
 
     native_object::map_like::ptr as_map_like() const override {
@@ -158,9 +158,7 @@ TEST(ObjectTest, native_object_equality) {
       return manage_owned<object>(w::i64(0));
     }
 
-    std::optional<std::vector<std::string>> keys() const override {
-      return keys_;
-    }
+    std::optional<std::set<std::string>> keys() const override { return keys_; }
 
     void print_to(
         tree_printer::scope scope,
@@ -169,7 +167,7 @@ TEST(ObjectTest, native_object_equality) {
     }
 
    private:
-    std::vector<std::string> keys_;
+    std::set<std::string> keys_;
   };
 
   struct always_zero_array : native_object,
@@ -198,11 +196,11 @@ TEST(ObjectTest, native_object_equality) {
   };
 
   native_object::ptr m1 =
-      std::make_shared<always_zero_map>(std::vector<std::string>{"foo"});
+      std::make_shared<always_zero_map>(std::set<std::string>{"foo"});
   native_object::ptr m2 =
-      std::make_shared<always_zero_map>(std::vector<std::string>{"foo"});
+      std::make_shared<always_zero_map>(std::set<std::string>{"foo"});
   native_object::ptr m3 =
-      std::make_shared<always_zero_map>(std::vector<std::string>{"foo", "bar"});
+      std::make_shared<always_zero_map>(std::set<std::string>{"foo", "bar"});
   map raw_m3{{"foo", w::i64(0)}, {"bar", w::i64(0)}};
 
   object o1 = w::native_object(m1);

thrift/compiler/whisker/test/render_test_helpers.h

@@ -83,11 +83,10 @@ class RenderTest : public testing::Test {
       return nullptr;
     }
 
-    std::optional<std::vector<std::string>> keys() const override {
-      std::vector<std::string> keys;
-      keys.reserve(values_.size());
+    std::optional<std::set<std::string>> keys() const override {
+      std::set<std::string> keys;
       for (const auto& [key, _] : values_) {
-        keys.push_back(key);
+        keys.insert(key);
       }
       return keys;
     }